On Fri, Jul 17, 2015 at 04:21:29PM +0200, Michal Privoznik wrote:
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@xxxxxxxxxx> --- 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.
Well, I'd prefer it to be here for now. And that's also why I didn't call the function virDomainDiskUpdatable() as that has nothing to do with updating a disk, it's just a "coincidence" that qemu uses it for that purpose ;)
Attachment:
signature.asc
Description: PGP signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list