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

On 17.07.2015 16:21, 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(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(+) >> > > > ACK This was premature. You need to squash this in: diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 2448a37..6562157 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -5781,7 +5781,7 @@ virDomainDiskDiffersSourceOnly(virDomainDiskDefPtr disk, "blkdeviotune size_iops_sec", true); - if (disk->transient && STRNEQ(disk->transient, orig_disk->transient)) { + if (disk->transient != orig_disk->transient) {

virReportError(VIR_ERR_OPERATION_UNSUPPORTED, _("cannot modify field '%s' of the disk"), "transient"); disk->transient is a bool, not string. Michal