1. Patchew
  2. Libvirt
  3. View series

[libvirt] [PATCH v2] virt-aa-helper: Set the supported features

Shivaprasad G Bhat posted 1 patch 6 years, 2 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20180119145455.81804.1387.stgit@lep8c.aus.stglabs.ibm.com
There is a newer version of this series
src/security/virt-aa-helper.c |    8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
[libvirt] [PATCH v2] virt-aa-helper: Set the supported features
Posted by Shivaprasad G Bhat 6 years, 2 months ago 
The virt-aa-helper fails to parse the xmls with the memory/cpu
hotplug features or user assigned aliases. Set the features in
xmlopt->config for the parsing to succeed.

Signed-off-by: Shivaprasad G Bhat <sbhat@linux.vnet.ibm.com>
---
 src/security/virt-aa-helper.c |    8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c
index f7ccae0..29a459d 100644
--- a/src/security/virt-aa-helper.c
+++ b/src/security/virt-aa-helper.c
@@ -654,6 +654,11 @@ caps_mockup(vahControl * ctl, const char *xmlStr)
     return rc;
 }
 
+virDomainDefParserConfig virAAHelperDomainDefParserConfig = {
+    .features = VIR_DOMAIN_DEF_FEATURE_MEMORY_HOTPLUG |
+                VIR_DOMAIN_DEF_FEATURE_OFFLINE_VCPUPIN |
+                VIR_DOMAIN_DEF_FEATURE_INDIVIDUAL_VCPUS,
+};
 
 static int
 get_definition(vahControl * ctl, const char *xmlStr)
@@ -673,7 +678,8 @@ get_definition(vahControl * ctl, const char *xmlStr)
         goto exit;
     }
 
-    if (!(ctl->xmlopt = virDomainXMLOptionNew(NULL, NULL, NULL, NULL, NULL))) {
+    if (!(ctl->xmlopt = virDomainXMLOptionNew(&virAAHelperDomainDefParserConfig,
+                                              NULL, NULL, NULL, NULL))) {
         vah_error(ctl, 0, _("Failed to create XML config object"));
         goto exit;
     }

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] virt-aa-helper: Set the supported features
Posted by Christian Ehrhardt 6 years, 2 months ago 
On Fri, Jan 19, 2018 at 3:55 PM, Shivaprasad G Bhat
<sbhat@linux.vnet.ibm.com> wrote:
> The virt-aa-helper fails to parse the xmls with the memory/cpu
> hotplug features or user assigned aliases. Set the features in
> xmlopt->config for the parsing to succeed.
>
> Signed-off-by: Shivaprasad G Bhat <sbhat@linux.vnet.ibm.com>
> ---
>  src/security/virt-aa-helper.c |    8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c
> index f7ccae0..29a459d 100644
> --- a/src/security/virt-aa-helper.c
> +++ b/src/security/virt-aa-helper.c
> @@ -654,6 +654,11 @@ caps_mockup(vahControl * ctl, const char *xmlStr)
>      return rc;
>  }
>
> +virDomainDefParserConfig virAAHelperDomainDefParserConfig = {
> +    .features = VIR_DOMAIN_DEF_FEATURE_MEMORY_HOTPLUG |
> +                VIR_DOMAIN_DEF_FEATURE_OFFLINE_VCPUPIN |
> +                VIR_DOMAIN_DEF_FEATURE_INDIVIDUAL_VCPUS,
> +};

Sure we can't link against qemu_domain.c to get "the original"
virQEMUDriverDomainDefParserConfig.
But a comment here that the define is essentially taken there might
helpful to later on follow any updates there.

Also mentioning why exactly you dropped
VIR_DOMAIN_DEF_FEATURE_USER_ALIAS for the same reason.

Other than that - I built and tested the case you are reporting and
can confirm that it works.
Tested-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>

BTW - since the essential line to trigger this is something like:
  <maxMemory slots='16' unit='KiB'>10485760</maxMemory>
Adding that in the commit message wouldn't be so bad either (but not important)

>  static int
>  get_definition(vahControl * ctl, const char *xmlStr)
> @@ -673,7 +678,8 @@ get_definition(vahControl * ctl, const char *xmlStr)
>          goto exit;
>      }
>
> -    if (!(ctl->xmlopt = virDomainXMLOptionNew(NULL, NULL, NULL, NULL, NULL))) {
> +    if (!(ctl->xmlopt = virDomainXMLOptionNew(&virAAHelperDomainDefParserConfig,
> +                                              NULL, NULL, NULL, NULL))) {
>          vah_error(ctl, 0, _("Failed to create XML config object"));
>          goto exit;
>      }
>
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list



-- 
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: Set the supported features
Posted by Peter Krempa 6 years, 2 months ago 
On Fri, Feb 02, 2018 at 08:14:29 +0100, Christian Ehrhardt wrote:
> On Fri, Jan 19, 2018 at 3:55 PM, Shivaprasad G Bhat
> <sbhat@linux.vnet.ibm.com> wrote:
> > The virt-aa-helper fails to parse the xmls with the memory/cpu
> > hotplug features or user assigned aliases. Set the features in
> > xmlopt->config for the parsing to succeed.
> >
> > Signed-off-by: Shivaprasad G Bhat <sbhat@linux.vnet.ibm.com>
> > ---
> >  src/security/virt-aa-helper.c |    8 +++++++-
> >  1 file changed, 7 insertions(+), 1 deletion(-)
> >
> > diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c
> > index f7ccae0..29a459d 100644
> > --- a/src/security/virt-aa-helper.c
> > +++ b/src/security/virt-aa-helper.c
> > @@ -654,6 +654,11 @@ caps_mockup(vahControl * ctl, const char *xmlStr)
> >      return rc;
> >  }
> >
> > +virDomainDefParserConfig virAAHelperDomainDefParserConfig = {
> > +    .features = VIR_DOMAIN_DEF_FEATURE_MEMORY_HOTPLUG |
> > +                VIR_DOMAIN_DEF_FEATURE_OFFLINE_VCPUPIN |
> > +                VIR_DOMAIN_DEF_FEATURE_INDIVIDUAL_VCPUS,
> > +};
> 
> Sure we can't link against qemu_domain.c to get "the original"
> virQEMUDriverDomainDefParserConfig.
> But a comment here that the define is essentially taken there might
> helpful to later on follow any updates there.
> 
> Also mentioning why exactly you dropped
> VIR_DOMAIN_DEF_FEATURE_USER_ALIAS for the same reason.

Actually, we might want to add a parser flag (or feature bit) to ignore
all other features when parsing. The aa-helper binary does not really
want or need to validate all the stuff necessary here and it might also
create problems since the feature bits are specifically designed to
reject XMLs which have certain elements (if the bit is not enabled).

This means that not-defining VIR_DOMAIN_DEF_FEATURE_USER_ALIAS would
actually make the parser fail on any XML which has user aliases despite
the fact that the aa-helper does not actually care about them at all.

Having a way to disable that is preferred, since having two definitions
of this would actualy result into one of them being out of sync all the
time.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] virt-aa-helper: Set the supported features
Posted by Shivaprasad bhat 6 years, 2 months ago 
Thanks for the reply Peter, Christian

On Fri, Feb 2, 2018 at 3:21 PM, Peter Krempa <pkrempa@redhat.com> wrote:

> On Fri, Feb 02, 2018 at 08:14:29 +0100, Christian Ehrhardt wrote:
> > On Fri, Jan 19, 2018 at 3:55 PM, Shivaprasad G Bhat
> > <sbhat@linux.vnet.ibm.com> wrote:
> > > The virt-aa-helper fails to parse the xmls with the memory/cpu
> > > hotplug features or user assigned aliases. Set the features in
> > > xmlopt->config for the parsing to succeed.
> > >
> > > Signed-off-by: Shivaprasad G Bhat <sbhat@linux.vnet.ibm.com>
> > > ---
> > >  src/security/virt-aa-helper.c |    8 +++++++-
> > >  1 file changed, 7 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/src/security/virt-aa-helper.c
> b/src/security/virt-aa-helper.c
> > > index f7ccae0..29a459d 100644
> > > --- a/src/security/virt-aa-helper.c
> > > +++ b/src/security/virt-aa-helper.c
> > > @@ -654,6 +654,11 @@ caps_mockup(vahControl * ctl, const char *xmlStr)
> > >      return rc;
> > >  }
> > >
> > > +virDomainDefParserConfig virAAHelperDomainDefParserConfig = {
> > > +    .features = VIR_DOMAIN_DEF_FEATURE_MEMORY_HOTPLUG |
> > > +                VIR_DOMAIN_DEF_FEATURE_OFFLINE_VCPUPIN |
> > > +                VIR_DOMAIN_DEF_FEATURE_INDIVIDUAL_VCPUS,
> > > +};
> >
> > Sure we can't link against qemu_domain.c to get "the original"
> > virQEMUDriverDomainDefParserConfig.
> > But a comment here that the define is essentially taken there might
> > helpful to later on follow any updates there.
> >
> > Also mentioning why exactly you dropped
> > VIR_DOMAIN_DEF_FEATURE_USER_ALIAS for the same reason.
>

The reason being the parsing code just ignores the user specified aliases
and doesn't fail the parsing if the VIR_DOMAIN_DEF_FEATURE_USER_ALIAS
feature flag is not set.


> Actually, we might want to add a parser flag (or feature bit) to ignore
> all other features when parsing. The aa-helper binary does not really
> want or need to validate all the stuff necessary here and it might also
> create problems since the feature bits are specifically designed to
> reject XMLs which have certain elements (if the bit is not enabled).
>

I agree. Having a new "skip" flag is better than this as that is future
proof
for more flags that might come.

Sending the next version in a while.

Thanks,
Shivaprasad



>
> This means that not-defining VIR_DOMAIN_DEF_FEATURE_USER_ALIAS would
> actually make the parser fail on any XML which has user aliases despite
> the fact that the aa-helper does not actually care about them at all.
>
> Having a way to disable that is preferred, since having two definitions
> of this would actualy result into one of them being out of sync all the
> time.
>
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] virt-aa-helper: Set the supported features
Posted by Peter Krempa 6 years, 2 months ago 
On Mon, Feb 05, 2018 at 18:39:24 +0530, Shivaprasad bhat wrote:
> Thanks for the reply Peter, Christian
> 
> On Fri, Feb 2, 2018 at 3:21 PM, Peter Krempa <pkrempa@redhat.com> wrote:
> 
> > On Fri, Feb 02, 2018 at 08:14:29 +0100, Christian Ehrhardt wrote:
> > > On Fri, Jan 19, 2018 at 3:55 PM, Shivaprasad G Bhat
> > > <sbhat@linux.vnet.ibm.com> wrote:
> > > > The virt-aa-helper fails to parse the xmls with the memory/cpu
> > > > hotplug features or user assigned aliases. Set the features in
> > > > xmlopt->config for the parsing to succeed.
> > > >
> > > > Signed-off-by: Shivaprasad G Bhat <sbhat@linux.vnet.ibm.com>
> > > > ---
> > > >  src/security/virt-aa-helper.c |    8 +++++++-
> > > >  1 file changed, 7 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/src/security/virt-aa-helper.c
> > b/src/security/virt-aa-helper.c
> > > > index f7ccae0..29a459d 100644
> > > > --- a/src/security/virt-aa-helper.c
> > > > +++ b/src/security/virt-aa-helper.c
> > > > @@ -654,6 +654,11 @@ caps_mockup(vahControl * ctl, const char *xmlStr)
> > > >      return rc;
> > > >  }
> > > >
> > > > +virDomainDefParserConfig virAAHelperDomainDefParserConfig = {
> > > > +    .features = VIR_DOMAIN_DEF_FEATURE_MEMORY_HOTPLUG |
> > > > +                VIR_DOMAIN_DEF_FEATURE_OFFLINE_VCPUPIN |
> > > > +                VIR_DOMAIN_DEF_FEATURE_INDIVIDUAL_VCPUS,
> > > > +};
> > >
> > > Sure we can't link against qemu_domain.c to get "the original"
> > > virQEMUDriverDomainDefParserConfig.
> > > But a comment here that the define is essentially taken there might
> > > helpful to later on follow any updates there.
> > >
> > > Also mentioning why exactly you dropped
> > > VIR_DOMAIN_DEF_FEATURE_USER_ALIAS for the same reason.
> >
> 
> The reason being the parsing code just ignores the user specified aliases
> and doesn't fail the parsing if the VIR_DOMAIN_DEF_FEATURE_USER_ALIAS
> feature flag is not set.

Ah, right. Sorry about that, I've only remembered the rest of the flags
which usually end in a parse failure.

In such case, ACK even without any modification currently. We can figure
out the skipping of the parser features later.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] virt-aa-helper: Set the supported features
Posted by Shivaprasad bhat 6 years, 2 months ago 
On Mon, Feb 5, 2018 at 6:44 PM, Peter Krempa <pkrempa@redhat.com> wrote:

> On Mon, Feb 05, 2018 at 18:39:24 +0530, Shivaprasad bhat wrote:
> > Thanks for the reply Peter, Christian
> >
> > On Fri, Feb 2, 2018 at 3:21 PM, Peter Krempa <pkrempa@redhat.com> wrote:
> >
> > > On Fri, Feb 02, 2018 at 08:14:29 +0100, Christian Ehrhardt wrote:
> > > > On Fri, Jan 19, 2018 at 3:55 PM, Shivaprasad G Bhat
> > > > <sbhat@linux.vnet.ibm.com> wrote:
> > > > > The virt-aa-helper fails to parse the xmls with the memory/cpu
> > > > > hotplug features or user assigned aliases. Set the features in
> > > > > xmlopt->config for the parsing to succeed.
> > > > >
> > > > > Signed-off-by: Shivaprasad G Bhat <sbhat@linux.vnet.ibm.com>
> > > > > ---
> > > > >  src/security/virt-aa-helper.c |    8 +++++++-
> > > > >  1 file changed, 7 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/src/security/virt-aa-helper.c
> > > b/src/security/virt-aa-helper.c
> > > > > index f7ccae0..29a459d 100644
> > > > > --- a/src/security/virt-aa-helper.c
> > > > > +++ b/src/security/virt-aa-helper.c
> > > > > @@ -654,6 +654,11 @@ caps_mockup(vahControl * ctl, const char
> *xmlStr)
> > > > >      return rc;
> > > > >  }
> > > > >
> > > > > +virDomainDefParserConfig virAAHelperDomainDefParserConfig = {
> > > > > +    .features = VIR_DOMAIN_DEF_FEATURE_MEMORY_HOTPLUG |
> > > > > +                VIR_DOMAIN_DEF_FEATURE_OFFLINE_VCPUPIN |
> > > > > +                VIR_DOMAIN_DEF_FEATURE_INDIVIDUAL_VCPUS,
> > > > > +};
> > > >
> > > > Sure we can't link against qemu_domain.c to get "the original"
> > > > virQEMUDriverDomainDefParserConfig.
> > > > But a comment here that the define is essentially taken there might
> > > > helpful to later on follow any updates there.
> > > >
> > > > Also mentioning why exactly you dropped
> > > > VIR_DOMAIN_DEF_FEATURE_USER_ALIAS for the same reason.
> > >
> >
> > The reason being the parsing code just ignores the user specified aliases
> > and doesn't fail the parsing if the VIR_DOMAIN_DEF_FEATURE_USER_ALIAS
> > feature flag is not set.
>
> Ah, right. Sorry about that, I've only remembered the rest of the flags
> which usually end in a parse failure.
>
> In such case, ACK even without any modification currently. We can figure
> out the skipping of the parser features later.


>
In that case, are you planning to push this patch to git ?

Or Should I rework on the V3 I sent already ?

Thanks,
Shivaprasad
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] virt-aa-helper: Set the supported features
Posted by Peter Krempa 6 years, 2 months ago 
On Tue, Feb 06, 2018 at 15:11:10 +0530, Shivaprasad bhat wrote:
> On Mon, Feb 5, 2018 at 6:44 PM, Peter Krempa <pkrempa@redhat.com> wrote:
> 
> > On Mon, Feb 05, 2018 at 18:39:24 +0530, Shivaprasad bhat wrote:
> > > Thanks for the reply Peter, Christian
> > >
> > > On Fri, Feb 2, 2018 at 3:21 PM, Peter Krempa <pkrempa@redhat.com> wrote:
> > >
> > > > On Fri, Feb 02, 2018 at 08:14:29 +0100, Christian Ehrhardt wrote:
> > > > > On Fri, Jan 19, 2018 at 3:55 PM, Shivaprasad G Bhat
> > > > > <sbhat@linux.vnet.ibm.com> wrote:
> > > > > > The virt-aa-helper fails to parse the xmls with the memory/cpu
> > > > > > hotplug features or user assigned aliases. Set the features in
> > > > > > xmlopt->config for the parsing to succeed.
> > > > > >
> > > > > > Signed-off-by: Shivaprasad G Bhat <sbhat@linux.vnet.ibm.com>
> > > > > > ---
> > > > > >  src/security/virt-aa-helper.c |    8 +++++++-
> > > > > >  1 file changed, 7 insertions(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/src/security/virt-aa-helper.c
> > > > b/src/security/virt-aa-helper.c
> > > > > > index f7ccae0..29a459d 100644
> > > > > > --- a/src/security/virt-aa-helper.c
> > > > > > +++ b/src/security/virt-aa-helper.c
> > > > > > @@ -654,6 +654,11 @@ caps_mockup(vahControl * ctl, const char
> > *xmlStr)
> > > > > >      return rc;
> > > > > >  }
> > > > > >
> > > > > > +virDomainDefParserConfig virAAHelperDomainDefParserConfig = {
> > > > > > +    .features = VIR_DOMAIN_DEF_FEATURE_MEMORY_HOTPLUG |
> > > > > > +                VIR_DOMAIN_DEF_FEATURE_OFFLINE_VCPUPIN |
> > > > > > +                VIR_DOMAIN_DEF_FEATURE_INDIVIDUAL_VCPUS,
> > > > > > +};
> > > > >
> > > > > Sure we can't link against qemu_domain.c to get "the original"
> > > > > virQEMUDriverDomainDefParserConfig.
> > > > > But a comment here that the define is essentially taken there might
> > > > > helpful to later on follow any updates there.
> > > > >
> > > > > Also mentioning why exactly you dropped
> > > > > VIR_DOMAIN_DEF_FEATURE_USER_ALIAS for the same reason.
> > > >
> > >
> > > The reason being the parsing code just ignores the user specified aliases
> > > and doesn't fail the parsing if the VIR_DOMAIN_DEF_FEATURE_USER_ALIAS
> > > feature flag is not set.
> >
> > Ah, right. Sorry about that, I've only remembered the rest of the flags
> > which usually end in a parse failure.
> >
> > In such case, ACK even without any modification currently. We can figure
> > out the skipping of the parser features later.
> 
> 
> >
> In that case, are you planning to push this patch to git ?

I think this is okay as long as you add a comment to the place wher the
VIR_DOMAIN_DEF_FEATURE_* enum is declared, that any new flag should be
considered to be added to the aa helper code as long as it prevents the
XML from being parsed.
--
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.