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@xxxxxxxxxx> >> --- >> 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@xxxxxxxxxx> Thanks, Michal -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
