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

Martin Kletzander mkletzan at redhat.com
Mon Jul 20 09:33:09 UTC 2015


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 at redhat.com>
>>> ---
>>>
>>> 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) {

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20150720/dd50fe22/attachment-0001.sig>


More information about the libvir-list mailing list