conf: Validate device on update-device

[libvirt] [PATCH] conf: Validate device on update-device

Posted by Michal Privoznik 6 years, 7 months ago

https://bugzilla.redhat.com/show_bug.cgi?id=1439991

Whenever a device is being updated via
virDomainUpdateDeviceFlags() API, we parse the device XML and
ideally run some generic checks to validate the configuration
(e.g. if device defines per-device boot order but the domain has
os/boot element already). Well, that's the theory - due to a
missing check we've jumped early from that check function.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 src/conf/domain_conf.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index f7574d77b..0064a71f5 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -26026,7 +26026,8 @@ virDomainDefCompatibleDevice(virDomainDefPtr def,
 {
     virDomainDeviceInfoPtr info = virDomainDeviceGetInfo(dev);
 
-    if (action != VIR_DOMAIN_DEVICE_ACTION_ATTACH)
+    if (action != VIR_DOMAIN_DEVICE_ACTION_ATTACH &&
+        action != VIR_DOMAIN_DEVICE_ACTION_UPDATE)
         return 0;
 
     if (!virDomainDefHasUSB(def) &&
-- 
2.13.5

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

Re: [libvirt] [PATCH] conf: Validate device on update-device

Posted by Erik Skultety 6 years, 7 months ago

On Tue, Sep 05, 2017 at 04:37:23PM +0200, Michal Privoznik wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1439991
>
> Whenever a device is being updated via
> virDomainUpdateDeviceFlags() API, we parse the device XML and
> ideally run some generic checks to validate the configuration
> (e.g. if device defines per-device boot order but the domain has
> os/boot element already). Well, that's the theory - due to a
> missing check we've jumped early from that check function.
>
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
>  src/conf/domain_conf.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index f7574d77b..0064a71f5 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -26026,7 +26026,8 @@ virDomainDefCompatibleDevice(virDomainDefPtr def,
>  {
>      virDomainDeviceInfoPtr info = virDomainDeviceGetInfo(dev);
>
> -    if (action != VIR_DOMAIN_DEVICE_ACTION_ATTACH)
> +    if (action != VIR_DOMAIN_DEVICE_ACTION_ATTACH &&
> +        action != VIR_DOMAIN_DEVICE_ACTION_UPDATE)
>          return 0;

^Which means that now the 'compatibility' check is only skipped for _DETACH
(which, for _DETACH, has been the case since @1c13166134). Therefore I think
that codewise, even though you're change makes sense, it would be IMHO nicer to
just drop this condition all together and also drop the call to this validation
helper from the *Detach helper (with the appropriate tuning of the validator's
signature of course).

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

>
>      if (!virDomainDefHasUSB(def) &&
> --
> 2.13.5
>
> --
> 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] conf: Validate device on update-device

Posted by Michal Privoznik 6 years, 7 months ago

On 09/06/2017 10:45 AM, Erik Skultety wrote:
> On Tue, Sep 05, 2017 at 04:37:23PM +0200, Michal Privoznik wrote:
>> https://bugzilla.redhat.com/show_bug.cgi?id=1439991
>>
>> Whenever a device is being updated via
>> virDomainUpdateDeviceFlags() API, we parse the device XML and
>> ideally run some generic checks to validate the configuration
>> (e.g. if device defines per-device boot order but the domain has
>> os/boot element already). Well, that's the theory - due to a
>> missing check we've jumped early from that check function.
>>
>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>> ---
>>  src/conf/domain_conf.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>> index f7574d77b..0064a71f5 100644
>> --- a/src/conf/domain_conf.c
>> +++ b/src/conf/domain_conf.c
>> @@ -26026,7 +26026,8 @@ virDomainDefCompatibleDevice(virDomainDefPtr def,
>>  {
>>      virDomainDeviceInfoPtr info = virDomainDeviceGetInfo(dev);
>>
>> -    if (action != VIR_DOMAIN_DEVICE_ACTION_ATTACH)
>> +    if (action != VIR_DOMAIN_DEVICE_ACTION_ATTACH &&
>> +        action != VIR_DOMAIN_DEVICE_ACTION_UPDATE)
>>          return 0;
> 
> ^Which means that now the 'compatibility' check is only skipped for _DETACH
> (which, for _DETACH, has been the case since @1c13166134). Therefore I think
> that codewise, even though you're change makes sense, it would be IMHO nicer to
> just drop this condition all together and also drop the call to this validation
> helper from the *Detach helper (with the appropriate tuning of the validator's
> signature of course).

Well, I might prefer that we call validation function even for detach
since I think it's more future proof (even though it's no-op currently),
but I don't stand so strong behind it. So okay, I'll remove the argument
and the call from *Detach helpers.

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

Thanks,
Michal

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