[libvirt] [PATCH v3 01/11] util: Introduce virHostGetDRMRenderNode helper

Erik Skultety posted 11 patches 7 years ago
[libvirt] [PATCH v3 01/11] util: Introduce virHostGetDRMRenderNode helper
Posted by Erik Skultety 7 years ago
This is the first step towards libvirt picking the first available
render node instead of QEMU. It also makes sense for us to be able to do
that, since we allow specifying the node directly for SPICE, so if
there's no render node specified by the user, we should pick the first
available one. The algorithm used for that is essentially the same as
the one QEMU uses.

Signed-off-by: Erik Skultety <eskultet@redhat.com>
---
 src/libvirt_private.syms |  1 +
 src/util/virutil.c       | 53 ++++++++++++++++++++++++++++++++++++++++
 src/util/virutil.h       |  2 ++
 3 files changed, 56 insertions(+)

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 5018a13e9c..c7f08f9620 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -3151,6 +3151,7 @@ virGetUserName;
 virGetUserRuntimeDirectory;
 virGetUserShell;
 virHexToBin;
+virHostGetDRMRenderNode;
 virHostHasIOMMU;
 virIndexToDiskName;
 virIsDevMapperDevice;
diff --git a/src/util/virutil.c b/src/util/virutil.c
index 974cffc2ee..da12a11e04 100644
--- a/src/util/virutil.c
+++ b/src/util/virutil.c
@@ -2145,3 +2145,56 @@ virHostHasIOMMU(void)
     VIR_DIR_CLOSE(iommuDir);
     return ret;
 }
+
+
+/**
+ * virHostGetDRMRenderNode:
+ *
+ * Picks the first DRM render node available. Missing DRI or missing DRM render
+ * nodes in the system results in an error.
+ *
+ * Returns an absolute path to the first render node available or NULL in case
+ * of an error with the error being reported.
+ * Caller is responsible for freeing the result string.
+ *
+ */
+char *
+virHostGetDRMRenderNode(void)
+{
+    char *ret = NULL;
+    DIR *driDir = NULL;
+    const char *driPath = "/dev/dri";
+    struct dirent *ent = NULL;
+    int dirErr = 0;
+    bool have_rendernode = false;
+
+    if (virDirOpen(&driDir, driPath) < 0)
+        return NULL;
+
+    while ((dirErr = virDirRead(driDir, &ent, driPath)) > 0) {
+        if (ent->d_type != DT_CHR)
+            continue;
+
+        if (STRPREFIX(ent->d_name, "renderD")) {
+            have_rendernode = true;
+            break;
+        }
+    }
+
+    if (dirErr < 0)
+        goto cleanup;
+
+    /* even if /dev/dri exists, there might be no renderDX nodes available */
+    if (!have_rendernode) {
+        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+                       _("No DRM render nodes available"));
+        goto cleanup;
+    }
+
+    if (virAsprintf(&ret, "%s/%s", driPath, ent->d_name) < 0)
+        goto cleanup;
+
+ cleanup:
+    VIR_DIR_CLOSE(driDir);
+    return ret;
+}
diff --git a/src/util/virutil.h b/src/util/virutil.h
index e0ab0da0f2..89bd21b148 100644
--- a/src/util/virutil.h
+++ b/src/util/virutil.h
@@ -222,6 +222,8 @@ unsigned long long virMemoryMaxValue(bool ulong) ATTRIBUTE_NOINLINE;
 
 bool virHostHasIOMMU(void);
 
+char *virHostGetDRMRenderNode(void);
+
 /**
  * VIR_ASSIGN_IS_OVERFLOW:
  * @rvalue: value that is checked (evaluated twice)
-- 
2.19.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 01/11] util: Introduce virHostGetDRMRenderNode helper
Posted by Daniel P. Berrangé 7 years ago
On Thu, Nov 29, 2018 at 03:20:11PM +0100, Erik Skultety wrote:
> This is the first step towards libvirt picking the first available
> render node instead of QEMU. It also makes sense for us to be able to do
> that, since we allow specifying the node directly for SPICE, so if
> there's no render node specified by the user, we should pick the first
> available one. The algorithm used for that is essentially the same as
> the one QEMU uses.
> 
> Signed-off-by: Erik Skultety <eskultet@redhat.com>
> ---
>  src/libvirt_private.syms |  1 +
>  src/util/virutil.c       | 53 ++++++++++++++++++++++++++++++++++++++++
>  src/util/virutil.h       |  2 ++
>  3 files changed, 56 insertions(+)
> 
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 5018a13e9c..c7f08f9620 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -3151,6 +3151,7 @@ virGetUserName;
>  virGetUserRuntimeDirectory;
>  virGetUserShell;
>  virHexToBin;
> +virHostGetDRMRenderNode;
>  virHostHasIOMMU;
>  virIndexToDiskName;
>  virIsDevMapperDevice;
> diff --git a/src/util/virutil.c b/src/util/virutil.c
> index 974cffc2ee..da12a11e04 100644
> --- a/src/util/virutil.c
> +++ b/src/util/virutil.c
> @@ -2145,3 +2145,56 @@ virHostHasIOMMU(void)
>      VIR_DIR_CLOSE(iommuDir);
>      return ret;
>  }
> +
> +
> +/**
> + * virHostGetDRMRenderNode:
> + *
> + * Picks the first DRM render node available. Missing DRI or missing DRM render
> + * nodes in the system results in an error.
> + *
> + * Returns an absolute path to the first render node available or NULL in case
> + * of an error with the error being reported.
> + * Caller is responsible for freeing the result string.
> + *
> + */
> +char *
> +virHostGetDRMRenderNode(void)
> +{
> +    char *ret = NULL;
> +    DIR *driDir = NULL;
> +    const char *driPath = "/dev/dri";
> +    struct dirent *ent = NULL;
> +    int dirErr = 0;
> +    bool have_rendernode = false;
> +
> +    if (virDirOpen(&driDir, driPath) < 0)
> +        return NULL;
> +
> +    while ((dirErr = virDirRead(driDir, &ent, driPath)) > 0) {
> +        if (ent->d_type != DT_CHR)
> +            continue;

The 'd_type' field is a Linux-ism, so this has broken the build

Is it even needed ? ie are then any files in /dev/dri that have
a prefix of "renderD" but which are not character devices ?

> +
> +        if (STRPREFIX(ent->d_name, "renderD")) {
> +            have_rendernode = true;
> +            break;
> +        }
> +    }

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 01/11] util: Introduce virHostGetDRMRenderNode helper
Posted by Erik Skultety 7 years ago
On Mon, Dec 03, 2018 at 03:18:47PM +0000, Daniel P. Berrangé wrote:
> On Thu, Nov 29, 2018 at 03:20:11PM +0100, Erik Skultety wrote:
> > This is the first step towards libvirt picking the first available
> > render node instead of QEMU. It also makes sense for us to be able to do
> > that, since we allow specifying the node directly for SPICE, so if
> > there's no render node specified by the user, we should pick the first
> > available one. The algorithm used for that is essentially the same as
> > the one QEMU uses.
> >
> > Signed-off-by: Erik Skultety <eskultet@redhat.com>
> > ---
> >  src/libvirt_private.syms |  1 +
> >  src/util/virutil.c       | 53 ++++++++++++++++++++++++++++++++++++++++
> >  src/util/virutil.h       |  2 ++
> >  3 files changed, 56 insertions(+)
> >
> > diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> > index 5018a13e9c..c7f08f9620 100644
> > --- a/src/libvirt_private.syms
> > +++ b/src/libvirt_private.syms
> > @@ -3151,6 +3151,7 @@ virGetUserName;
> >  virGetUserRuntimeDirectory;
> >  virGetUserShell;
> >  virHexToBin;
> > +virHostGetDRMRenderNode;
> >  virHostHasIOMMU;
> >  virIndexToDiskName;
> >  virIsDevMapperDevice;
> > diff --git a/src/util/virutil.c b/src/util/virutil.c
> > index 974cffc2ee..da12a11e04 100644
> > --- a/src/util/virutil.c
> > +++ b/src/util/virutil.c
> > @@ -2145,3 +2145,56 @@ virHostHasIOMMU(void)
> >      VIR_DIR_CLOSE(iommuDir);
> >      return ret;
> >  }
> > +
> > +
> > +/**
> > + * virHostGetDRMRenderNode:
> > + *
> > + * Picks the first DRM render node available. Missing DRI or missing DRM render
> > + * nodes in the system results in an error.
> > + *
> > + * Returns an absolute path to the first render node available or NULL in case
> > + * of an error with the error being reported.
> > + * Caller is responsible for freeing the result string.
> > + *
> > + */
> > +char *
> > +virHostGetDRMRenderNode(void)
> > +{
> > +    char *ret = NULL;
> > +    DIR *driDir = NULL;
> > +    const char *driPath = "/dev/dri";
> > +    struct dirent *ent = NULL;
> > +    int dirErr = 0;
> > +    bool have_rendernode = false;
> > +
> > +    if (virDirOpen(&driDir, driPath) < 0)
> > +        return NULL;
> > +
> > +    while ((dirErr = virDirRead(driDir, &ent, driPath)) > 0) {
> > +        if (ent->d_type != DT_CHR)
> > +            continue;
>
> The 'd_type' field is a Linux-ism, so this has broken the build
>
> Is it even needed ? ie are then any files in /dev/dri that have
> a prefix of "renderD" but which are not character devices ?

Can't say it's impossible, but I haven't bumped into a setup like that - I had
different permissions, I had DRI missing, I had renderD missing, but I haven't
seen a non-character DRI node...You're right in the thinking, the reason why I
put it in there is to spare a few cycles wasted on string comparison, so either
I can shrug my shoulders, remove the check, waste the cycles and be happy or I
can have a look whether gnulib can help us out here.

Erik

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 01/11] util: Introduce virHostGetDRMRenderNode helper
Posted by Daniel P. Berrangé 7 years ago
On Mon, Dec 03, 2018 at 04:24:18PM +0100, Erik Skultety wrote:
> On Mon, Dec 03, 2018 at 03:18:47PM +0000, Daniel P. Berrangé wrote:
> > On Thu, Nov 29, 2018 at 03:20:11PM +0100, Erik Skultety wrote:
> > > This is the first step towards libvirt picking the first available
> > > render node instead of QEMU. It also makes sense for us to be able to do
> > > that, since we allow specifying the node directly for SPICE, so if
> > > there's no render node specified by the user, we should pick the first
> > > available one. The algorithm used for that is essentially the same as
> > > the one QEMU uses.
> > >
> > > Signed-off-by: Erik Skultety <eskultet@redhat.com>
> > > ---
> > >  src/libvirt_private.syms |  1 +
> > >  src/util/virutil.c       | 53 ++++++++++++++++++++++++++++++++++++++++
> > >  src/util/virutil.h       |  2 ++
> > >  3 files changed, 56 insertions(+)
> > >
> > > diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> > > index 5018a13e9c..c7f08f9620 100644
> > > --- a/src/libvirt_private.syms
> > > +++ b/src/libvirt_private.syms
> > > @@ -3151,6 +3151,7 @@ virGetUserName;
> > >  virGetUserRuntimeDirectory;
> > >  virGetUserShell;
> > >  virHexToBin;
> > > +virHostGetDRMRenderNode;
> > >  virHostHasIOMMU;
> > >  virIndexToDiskName;
> > >  virIsDevMapperDevice;
> > > diff --git a/src/util/virutil.c b/src/util/virutil.c
> > > index 974cffc2ee..da12a11e04 100644
> > > --- a/src/util/virutil.c
> > > +++ b/src/util/virutil.c
> > > @@ -2145,3 +2145,56 @@ virHostHasIOMMU(void)
> > >      VIR_DIR_CLOSE(iommuDir);
> > >      return ret;
> > >  }
> > > +
> > > +
> > > +/**
> > > + * virHostGetDRMRenderNode:
> > > + *
> > > + * Picks the first DRM render node available. Missing DRI or missing DRM render
> > > + * nodes in the system results in an error.
> > > + *
> > > + * Returns an absolute path to the first render node available or NULL in case
> > > + * of an error with the error being reported.
> > > + * Caller is responsible for freeing the result string.
> > > + *
> > > + */
> > > +char *
> > > +virHostGetDRMRenderNode(void)
> > > +{
> > > +    char *ret = NULL;
> > > +    DIR *driDir = NULL;
> > > +    const char *driPath = "/dev/dri";
> > > +    struct dirent *ent = NULL;
> > > +    int dirErr = 0;
> > > +    bool have_rendernode = false;
> > > +
> > > +    if (virDirOpen(&driDir, driPath) < 0)
> > > +        return NULL;
> > > +
> > > +    while ((dirErr = virDirRead(driDir, &ent, driPath)) > 0) {
> > > +        if (ent->d_type != DT_CHR)
> > > +            continue;
> >
> > The 'd_type' field is a Linux-ism, so this has broken the build
> >
> > Is it even needed ? ie are then any files in /dev/dri that have
> > a prefix of "renderD" but which are not character devices ?
> 
> Can't say it's impossible, but I haven't bumped into a setup like that - I had
> different permissions, I had DRI missing, I had renderD missing, but I haven't
> seen a non-character DRI node...You're right in the thinking, the reason why I
> put it in there is to spare a few cycles wasted on string comparison, so either
> I can shrug my shoulders, remove the check, waste the cycles and be happy or I
> can have a look whether gnulib can help us out here.

gnulib can't fix this unfortunately. Without d_type being reported by the
kernel, the only way gnulib could "fix" it would be to call stat() on every
dirent result which would be horrifically inefficient for users who don't
care about the d_type.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 01/11] util: Introduce virHostGetDRMRenderNode helper
Posted by Ján Tomko 7 years ago
On Thu, Nov 29, 2018 at 03:20:11PM +0100, Erik Skultety wrote:
>This is the first step towards libvirt picking the first available
>render node instead of QEMU. It also makes sense for us to be able to do
>that, since we allow specifying the node directly for SPICE, so if
>there's no render node specified by the user, we should pick the first
>available one. The algorithm used for that is essentially the same as
>the one QEMU uses.
>
>Signed-off-by: Erik Skultety <eskultet@redhat.com>
>---
> src/libvirt_private.syms |  1 +
> src/util/virutil.c       | 53 ++++++++++++++++++++++++++++++++++++++++
> src/util/virutil.h       |  2 ++
> 3 files changed, 56 insertions(+)
>

Reviewed-by: Ján Tomko <jtomko@redhat.com>

Jano
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list