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

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


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 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(+)
>>
>> 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 ;)
-------------- 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/54187c0e/attachment-0001.sig>


More information about the libvir-list mailing list