On Fri, Jul 17, 2015 at 04:58:46PM +0200, Michal Privoznik wrote:
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@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(+)ACKThis 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) {
Actually, if you don't specify <transient/> for a disk that is already
transient, this will fail to update the media. But you're right this
must not be string comparison. I changed this check to:
CHECK_EQ(transient, "transient", true);
Which is a bit more complicated, but shorter version of:
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
Attachment:
signature.asc
Description: PGP signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list