1. Patchew
  2. Libvirt
  3. View series

[libvirt] [PATCH v2] virt-aa-helper: generate rules for gl enabled graphics devices

Christian Ehrhardt posted 1 patch 5 years, 2 months ago
Test syntax-check passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20190122103050.13526-1-christian.ehrhardt@canonical.com
src/security/virt-aa-helper.c | 12 ++++++++++++
tests/virt-aa-helper-test     |  6 ++++++
2 files changed, 18 insertions(+)
[libvirt] [PATCH v2] virt-aa-helper: generate rules for gl enabled graphics devices
Posted by Christian Ehrhardt 5 years, 2 months ago 
This adds the virt-aa-helper support for gl enabled graphics devices to
generate rules for the needed rendernode paths.

Example in domain xml:
<graphics type='spice'>
  <gl enable='yes' rendernode='/dev/dri/bar'/>
</graphics>

results in:
  "/dev/dri/bar" rw,

Special cases are:
- multiple devices with rendernodes -> all are added
- non explicit rendernodes -> follow recently added virHostGetDRMRenderNode
- rendernode without opengl (in egl-headless for example) -> still add
  the node

Updates in V2:
- avoid leaking rendernode
- logically group patch hunks
- drop base /dev/dri access
- simplify detection of the need for a default rendernode
- add tests in tests/virt-aa-helper-test
- rebased to recent master

Fixes: https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/1757085

Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
---
 src/security/virt-aa-helper.c | 12 ++++++++++++
 tests/virt-aa-helper-test     |  6 ++++++
 2 files changed, 18 insertions(+)

diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c
index 64a425671d..a84dd5a8c9 100644
--- a/src/security/virt-aa-helper.c
+++ b/src/security/virt-aa-helper.c
@@ -1062,6 +1062,18 @@ get_files(vahControl * ctl)
     for (i = 0; i < ctl->def->ngraphics; i++) {
         virDomainGraphicsDefPtr graphics = ctl->def->graphics[i];
         size_t n;
+        const char *rendernode = virDomainGraphicsGetRenderNode(graphics);
+
+        if (rendernode)
+            vah_add_file(&buf, rendernode, "rw");
+        else
+            if (virDomainGraphicsNeedsAutoRenderNode(graphics)) {
+                char * defaultRenderNode = virHostGetDRMRenderNode();
+                if (defaultRenderNode) {
+                    vah_add_file(&buf, defaultRenderNode, "rw");
+                    VIR_FREE(defaultRenderNode);
+            }
+        }
 
         for (n = 0; n < graphics->nListens; n++) {
             virDomainGraphicsListenDef listenObj = graphics->listens[n];
diff --git a/tests/virt-aa-helper-test b/tests/virt-aa-helper-test
index fb400574dc..6e674bfe5c 100755
--- a/tests/virt-aa-helper-test
+++ b/tests/virt-aa-helper-test
@@ -378,6 +378,12 @@ testme "0" "input dev passthrough" "-r -u $valid_uuid" "$test_xml" "$disk2.*rw,$
 sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e "s,<memory>524288</memory>,<memory>1048576</memory>,g" -e "s,</devices>,<memory model='nvdimm'><source><path>$disk2</path></source><target><size unit='KiB'>524288</size><node>0</node></target></memory></devices>,g" "$template_xml" > "$test_xml"
 testme "0" "nvdimm" "-r -u $valid_uuid" "$test_xml" "$disk2.*rw,$"
 
+sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e "s,</devices>,<graphics type='egl-headless'><gl rendernode='/dev/dri/testegl1'/></graphics></devices>,g" "$template_xml" > "$test_xml"
+testme "0" "dri egl" "-r -u $valid_uuid" "$test_xml" "/dev/dri/testegl1.*rw,$"
+
+sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e "s,</devices>,<graphics type='spice'><gl enable='yes' rendernode='/dev/dri/testegl2'/></graphics></devices>,g" "$template_xml" > "$test_xml"
+testme "0" "dri spice" "-r -u $valid_uuid" "$test_xml" "/dev/dri/testegl2.*rw,$"
+
 testme "0" "help" "-h"
 
 echo "" >$output
-- 
2.17.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] virt-aa-helper: generate rules for gl enabled graphics devices
Posted by Erik Skultety 5 years, 2 months ago 
On Tue, Jan 22, 2019 at 11:30:50AM +0100, Christian Ehrhardt wrote:
> This adds the virt-aa-helper support for gl enabled graphics devices to
> generate rules for the needed rendernode paths.
>
> Example in domain xml:
> <graphics type='spice'>
>   <gl enable='yes' rendernode='/dev/dri/bar'/>
> </graphics>
>
> results in:
>   "/dev/dri/bar" rw,
>
> Special cases are:
> - multiple devices with rendernodes -> all are added
> - non explicit rendernodes -> follow recently added virHostGetDRMRenderNode
> - rendernode without opengl (in egl-headless for example) -> still add
>   the node
>
> Updates in V2:
> - avoid leaking rendernode
> - logically group patch hunks
> - drop base /dev/dri access
> - simplify detection of the need for a default rendernode
> - add tests in tests/virt-aa-helper-test
> - rebased to recent master

^This is irrelevant to the git history, so should be dropped before pushing

>
> Fixes: https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/1757085
>
> Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
> ---
>  src/security/virt-aa-helper.c | 12 ++++++++++++
>  tests/virt-aa-helper-test     |  6 ++++++
>  2 files changed, 18 insertions(+)
>
> diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c
> index 64a425671d..a84dd5a8c9 100644
> --- a/src/security/virt-aa-helper.c
> +++ b/src/security/virt-aa-helper.c
> @@ -1062,6 +1062,18 @@ get_files(vahControl * ctl)
>      for (i = 0; i < ctl->def->ngraphics; i++) {
>          virDomainGraphicsDefPtr graphics = ctl->def->graphics[i];
>          size_t n;
> +        const char *rendernode = virDomainGraphicsGetRenderNode(graphics);
> +
> +        if (rendernode)
> +            vah_add_file(&buf, rendernode, "rw");
> +        else
> +            if (virDomainGraphicsNeedsAutoRenderNode(graphics)) {
> +                char * defaultRenderNode = virHostGetDRMRenderNode();
> +                if (defaultRenderNode) {
> +                    vah_add_file(&buf, defaultRenderNode, "rw");
> +                    VIR_FREE(defaultRenderNode);
> +            }
> +        }

Minor guideline nitpick about curly braces around complex bodies, with the
following squashed in:

diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c
index a84dd5a8c9..78666db4f8 100644
--- a/src/security/virt-aa-helper.c
+++ b/src/security/virt-aa-helper.c
@@ -1064,14 +1064,15 @@ get_files(vahControl * ctl)
         size_t n;
         const char *rendernode = virDomainGraphicsGetRenderNode(graphics);

-        if (rendernode)
+        if (rendernode) {
             vah_add_file(&buf, rendernode, "rw");
-        else
+        } else {
             if (virDomainGraphicsNeedsAutoRenderNode(graphics)) {
-                char * defaultRenderNode = virHostGetDRMRenderNode();
+                char *defaultRenderNode = virHostGetDRMRenderNode();
+
                 if (defaultRenderNode) {
                     vah_add_file(&buf, defaultRenderNode, "rw");
                     VIR_FREE(defaultRenderNode);
+                }
             }
         }

Reviewed-by: Erik Skultety <eskultet@redhat.com>

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] virt-aa-helper: generate rules for gl enabled graphics devices
Posted by Christian Ehrhardt 5 years, 2 months ago 
On Tue, Jan 22, 2019 at 2:25 PM Erik Skultety <eskultet@redhat.com> wrote:
>
> On Tue, Jan 22, 2019 at 11:30:50AM +0100, Christian Ehrhardt wrote:
> > This adds the virt-aa-helper support for gl enabled graphics devices to
> > generate rules for the needed rendernode paths.
> >
> > Example in domain xml:
> > <graphics type='spice'>
> >   <gl enable='yes' rendernode='/dev/dri/bar'/>
> > </graphics>
> >
> > results in:
> >   "/dev/dri/bar" rw,
> >
> > Special cases are:
> > - multiple devices with rendernodes -> all are added
> > - non explicit rendernodes -> follow recently added virHostGetDRMRenderNode
> > - rendernode without opengl (in egl-headless for example) -> still add
> >   the node
> >
> > Updates in V2:
> > - avoid leaking rendernode
> > - logically group patch hunks
> > - drop base /dev/dri access
> > - simplify detection of the need for a default rendernode
> > - add tests in tests/virt-aa-helper-test
> > - rebased to recent master
>
> ^This is irrelevant to the git history, so should be dropped before pushing

I added it with an --amend on the submission as I had no 0/X patch to
mention it.
Will not be in the push once ack's are in.

> >
> > Fixes: https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/1757085
> >
> > Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
> > ---
> >  src/security/virt-aa-helper.c | 12 ++++++++++++
> >  tests/virt-aa-helper-test     |  6 ++++++
> >  2 files changed, 18 insertions(+)
> >
> > diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c
> > index 64a425671d..a84dd5a8c9 100644
> > --- a/src/security/virt-aa-helper.c
> > +++ b/src/security/virt-aa-helper.c
> > @@ -1062,6 +1062,18 @@ get_files(vahControl * ctl)
> >      for (i = 0; i < ctl->def->ngraphics; i++) {
> >          virDomainGraphicsDefPtr graphics = ctl->def->graphics[i];
> >          size_t n;
> > +        const char *rendernode = virDomainGraphicsGetRenderNode(graphics);
> > +
> > +        if (rendernode)
> > +            vah_add_file(&buf, rendernode, "rw");
> > +        else
> > +            if (virDomainGraphicsNeedsAutoRenderNode(graphics)) {
> > +                char * defaultRenderNode = virHostGetDRMRenderNode();
> > +                if (defaultRenderNode) {
> > +                    vah_add_file(&buf, defaultRenderNode, "rw");
> > +                    VIR_FREE(defaultRenderNode);
> > +            }
> > +        }
>
> Minor guideline nitpick about curly braces around complex bodies, with the
> following squashed in:

I was making syntaxcheck happy, but yours looks better - Integrated in
what would be pushed, thanks!

>
> diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c
> index a84dd5a8c9..78666db4f8 100644
> --- a/src/security/virt-aa-helper.c
> +++ b/src/security/virt-aa-helper.c
> @@ -1064,14 +1064,15 @@ get_files(vahControl * ctl)
>          size_t n;
>          const char *rendernode = virDomainGraphicsGetRenderNode(graphics);
>
> -        if (rendernode)
> +        if (rendernode) {
>              vah_add_file(&buf, rendernode, "rw");
> -        else
> +        } else {
>              if (virDomainGraphicsNeedsAutoRenderNode(graphics)) {
> -                char * defaultRenderNode = virHostGetDRMRenderNode();
> +                char *defaultRenderNode = virHostGetDRMRenderNode();
> +
>                  if (defaultRenderNode) {
>                      vah_add_file(&buf, defaultRenderNode, "rw");
>                      VIR_FREE(defaultRenderNode);
> +                }
>              }
>          }
>
> Reviewed-by: Erik Skultety <eskultet@redhat.com>



-- 
Christian Ehrhardt
Software Engineer, Ubuntu Server
Canonical Ltd

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] virt-aa-helper: generate rules for gl enabled graphics devices
Posted by Jamie Strandboge 5 years, 2 months ago 
On Tue, 22 Jan 2019, Christian Ehrhardt wrote:

> This adds the virt-aa-helper support for gl enabled graphics devices to
> generate rules for the needed rendernode paths.
> 
> Example in domain xml:
> <graphics type='spice'>
>   <gl enable='yes' rendernode='/dev/dri/bar'/>
> </graphics>
> 
> results in:
>   "/dev/dri/bar" rw,
> 
> Special cases are:
> - multiple devices with rendernodes -> all are added
> - non explicit rendernodes -> follow recently added virHostGetDRMRenderNode
> - rendernode without opengl (in egl-headless for example) -> still add
>   the node
> 
> Updates in V2:
> - avoid leaking rendernode
> - logically group patch hunks
> - drop base /dev/dri access
> - simplify detection of the need for a default rendernode
> - add tests in tests/virt-aa-helper-test
> - rebased to recent master
> 
> Fixes: https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/1757085
> 
> Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
> ---
>  src/security/virt-aa-helper.c | 12 ++++++++++++
>  tests/virt-aa-helper-test     |  6 ++++++
>  2 files changed, 18 insertions(+)
> 
> diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c
> index 64a425671d..a84dd5a8c9 100644
> --- a/src/security/virt-aa-helper.c
> +++ b/src/security/virt-aa-helper.c
> @@ -1062,6 +1062,18 @@ get_files(vahControl * ctl)
>      for (i = 0; i < ctl->def->ngraphics; i++) {
>          virDomainGraphicsDefPtr graphics = ctl->def->graphics[i];
>          size_t n;
> +        const char *rendernode = virDomainGraphicsGetRenderNode(graphics);
> +
> +        if (rendernode)
> +            vah_add_file(&buf, rendernode, "rw");
> +        else
> +            if (virDomainGraphicsNeedsAutoRenderNode(graphics)) {
> +                char * defaultRenderNode = virHostGetDRMRenderNode();
> +                if (defaultRenderNode) {
> +                    vah_add_file(&buf, defaultRenderNode, "rw");
> +                    VIR_FREE(defaultRenderNode);
> +            }
> +        }
>  
>          for (n = 0; n < graphics->nListens; n++) {
>              virDomainGraphicsListenDef listenObj = graphics->listens[n];
> diff --git a/tests/virt-aa-helper-test b/tests/virt-aa-helper-test
> index fb400574dc..6e674bfe5c 100755
> --- a/tests/virt-aa-helper-test
> +++ b/tests/virt-aa-helper-test
> @@ -378,6 +378,12 @@ testme "0" "input dev passthrough" "-r -u $valid_uuid" "$test_xml" "$disk2.*rw,$
>  sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e "s,<memory>524288</memory>,<memory>1048576</memory>,g" -e "s,</devices>,<memory model='nvdimm'><source><path>$disk2</path></source><target><size unit='KiB'>524288</size><node>0</node></target></memory></devices>,g" "$template_xml" > "$test_xml"
>  testme "0" "nvdimm" "-r -u $valid_uuid" "$test_xml" "$disk2.*rw,$"
>  
> +sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e "s,</devices>,<graphics type='egl-headless'><gl rendernode='/dev/dri/testegl1'/></graphics></devices>,g" "$template_xml" > "$test_xml"
> +testme "0" "dri egl" "-r -u $valid_uuid" "$test_xml" "/dev/dri/testegl1.*rw,$"
> +
> +sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e "s,</devices>,<graphics type='spice'><gl enable='yes' rendernode='/dev/dri/testegl2'/></graphics></devices>,g" "$template_xml" > "$test_xml"
> +testme "0" "dri spice" "-r -u $valid_uuid" "$test_xml" "/dev/dri/testegl2.*rw,$"
> +
>  testme "0" "help" "-h"
>  
>  echo "" >$output
> -- 
> 2.17.1

LGTM. Thanks for the v2 and the tests :)

-- 
Jamie Strandboge             | http://www.canonical.com
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] virt-aa-helper: generate rules for gl enabled graphics devices
Posted by Christian Ehrhardt 5 years, 2 months ago 
On Tue, Jan 22, 2019 at 9:10 PM Jamie Strandboge <jamie@canonical.com> wrote:
>
> On Tue, 22 Jan 2019, Christian Ehrhardt wrote:
>
> > This adds the virt-aa-helper support for gl enabled graphics devices to
> > generate rules for the needed rendernode paths.
> >
> > Example in domain xml:
> > <graphics type='spice'>
> >   <gl enable='yes' rendernode='/dev/dri/bar'/>
> > </graphics>
> >
> > results in:
> >   "/dev/dri/bar" rw,
> >
> > Special cases are:
> > - multiple devices with rendernodes -> all are added
> > - non explicit rendernodes -> follow recently added virHostGetDRMRenderNode
> > - rendernode without opengl (in egl-headless for example) -> still add
> >   the node
> >
> > Updates in V2:
> > - avoid leaking rendernode
> > - logically group patch hunks
> > - drop base /dev/dri access
> > - simplify detection of the need for a default rendernode
> > - add tests in tests/virt-aa-helper-test
> > - rebased to recent master
> >
> > Fixes: https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/1757085
> >
> > Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
> > ---
> >  src/security/virt-aa-helper.c | 12 ++++++++++++
> >  tests/virt-aa-helper-test     |  6 ++++++
> >  2 files changed, 18 insertions(+)
> >
> > diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c
> > index 64a425671d..a84dd5a8c9 100644
> > --- a/src/security/virt-aa-helper.c
> > +++ b/src/security/virt-aa-helper.c
> > @@ -1062,6 +1062,18 @@ get_files(vahControl * ctl)
> >      for (i = 0; i < ctl->def->ngraphics; i++) {
> >          virDomainGraphicsDefPtr graphics = ctl->def->graphics[i];
> >          size_t n;
> > +        const char *rendernode = virDomainGraphicsGetRenderNode(graphics);
> > +
> > +        if (rendernode)
> > +            vah_add_file(&buf, rendernode, "rw");
> > +        else
> > +            if (virDomainGraphicsNeedsAutoRenderNode(graphics)) {
> > +                char * defaultRenderNode = virHostGetDRMRenderNode();
> > +                if (defaultRenderNode) {
> > +                    vah_add_file(&buf, defaultRenderNode, "rw");
> > +                    VIR_FREE(defaultRenderNode);
> > +            }
> > +        }
> >
> >          for (n = 0; n < graphics->nListens; n++) {
> >              virDomainGraphicsListenDef listenObj = graphics->listens[n];
> > diff --git a/tests/virt-aa-helper-test b/tests/virt-aa-helper-test
> > index fb400574dc..6e674bfe5c 100755
> > --- a/tests/virt-aa-helper-test
> > +++ b/tests/virt-aa-helper-test
> > @@ -378,6 +378,12 @@ testme "0" "input dev passthrough" "-r -u $valid_uuid" "$test_xml" "$disk2.*rw,$
> >  sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e "s,<memory>524288</memory>,<memory>1048576</memory>,g" -e "s,</devices>,<memory model='nvdimm'><source><path>$disk2</path></source><target><size unit='KiB'>524288</size><node>0</node></target></memory></devices>,g" "$template_xml" > "$test_xml"
> >  testme "0" "nvdimm" "-r -u $valid_uuid" "$test_xml" "$disk2.*rw,$"
> >
> > +sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e "s,</devices>,<graphics type='egl-headless'><gl rendernode='/dev/dri/testegl1'/></graphics></devices>,g" "$template_xml" > "$test_xml"
> > +testme "0" "dri egl" "-r -u $valid_uuid" "$test_xml" "/dev/dri/testegl1.*rw,$"
> > +
> > +sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e "s,</devices>,<graphics type='spice'><gl enable='yes' rendernode='/dev/dri/testegl2'/></graphics></devices>,g" "$template_xml" > "$test_xml"
> > +testme "0" "dri spice" "-r -u $valid_uuid" "$test_xml" "/dev/dri/testegl2.*rw,$"
> > +
> >  testme "0" "help" "-h"
> >
> >  echo "" >$output
> > --
> > 2.17.1
>
> LGTM. Thanks for the v2 and the tests :)

Thanks Erik, Michal and Jamie for your feedback!
I pushed the commit to master with your acks/reviews added and the
improvement of Erik squashed into it.

>
> --
> Jamie Strandboge             | http://www.canonical.com



-- 
Christian Ehrhardt
Software Engineer, Ubuntu Server
Canonical Ltd

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

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.