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

[Michal Privoznik]

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 at 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 at redhat.com>

Thanks,
Michal