Re: [libvirt] [PATCH v2] qemu: Reject updating unsupported disk information

On 10.07.2015 08:20, Martin Kletzander wrote: > If one calls update-device with information that is not updatable, > libvirt reports success even though no data were updated. The example > used in the bug linked below uses updating device with <boot order='2'/> > which, in my opinion, is a valid thing to request from user's > perspective. Mainly since we properly error out if user wants to update > such data on a network device for example. > > And since there are many things that might happen (update-device on disk > basically knows just how to change removable media), check for what's > changing and moreover, since the function might be usable in other > drivers (updating only disk path is a valid possibility) let's abstract > it for any two disks. > > We can't possibly check for everything since for many fields our code > does not properly differentiate between default and unspecified values. > Even though this could be changed, I don't feel like it's worth the > complexity so it's not the aim of this patch. > > Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1007228 > Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com&gt; > --- > > Notes: > v2: > - Don't say 'NULL' when it should be 'unspecified' > - Don't blindly copy field name into the error message, but use space > instead of dot. That way it looks the same as in the XML provided. > - Check strings properly instead of addresses > > src/conf/domain_conf.c | 133 +++++++++++++++++++++++++++++++++++++++++++++++ > src/conf/domain_conf.h | 2 + > src/libvirt_private.syms | 1 + > src/qemu/qemu_driver.c | 3 ++ > 4 files changed, 139 insertions(+) > > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c > index 1f7862b00463..69e5df27c270 100644 > --- a/src/conf/domain_conf.c > +++ b/src/conf/domain_conf.c > @@ -5690,6 +5690,139 @@ virDomainDiskFindByBusAndDst(virDomainDefPtr def, > return NULL; > } > > + > +/* > + * Makes sure the @disk differs from @orig_disk only by the source > + * path and nothing else. Fields that are being checked and the > + * information whether they are nullable (may not be specified) or is > + * taken from the virDomainDiskDefFormat() code. > + */ > +bool > +virDomainDiskDiffersSourceOnly(virDomainDiskDefPtr disk, > + virDomainDiskDefPtr orig_disk) > +{ Nice work, but I think this should go somewhere into src/qemu/. I mean, some o these fields are not changeable as a domain is running. But some might be depending on the underlying hypervisor. For instance, xen might be able to change serial number of a HDD. On the other hand, that might be just a few corner cases I'm talking about, so it's up to you what you prefer more.