[libvirt] [PATCH] qemuDomainAttachDeviceLive: Check provided disk address

[Martin Kletzander]

On Wed, Sep 30, 2015 at 09:44:27AM -0400, John Ferlan wrote:
>
>
>On 09/30/2015 12:43 AM, Martin Kletzander wrote:
>> On Tue, Sep 29, 2015 at 05:27:58PM -0400, John Ferlan wrote:
>
>[...]
>
>>>
>>> NOTE: The change to the test is because the failure now occurs during
>>> parse rather than at run (e.g. earlier, where I think it should).
>>
>> I agree, and this sounds good, I'd have just two minor additions, see
>> below.
>>
>>> From a03a8aa073eb410d6b713e6f77572edcf0631263 Mon Sep 17 00:00:00 2001
>>> From: John Ferlan <jferlan at redhat.com>
>>> Date: Tue, 29 Sep 2015 17:04:11 -0400
>>> Subject: [PATCH] bug 1257844
>>>
>>> Signed-off-by: John Ferlan <jferlan at redhat.com>
>>> ---
>>> src/conf/domain_conf.c   | 43 +++++++++++++++++++++++++++++++++++++++++++
>>> tests/qemuxml2argvtest.c |  2 +-
>>> 2 files changed, 44 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>>> index 393ece7..21439c7 100644
>>> --- a/src/conf/domain_conf.c
>>> +++ b/src/conf/domain_conf.c
>>> @@ -3962,6 +3962,41 @@ virDomainDefPostParseInternal(virDomainDefPtr def,
>>> }
>>>
>>>
>>> +/* Check if a provided address is valid */
>>> +static bool
>>> +virDomainDeviceAddressTypeIsValid(virDomainDiskDefPtr disk)
>>
>> 1) The name suggests it checks the address type of any device, I'd
>>    somehow add a "Disk" in the name =)
>
>yeah - I was merely copying from Ruifeng's code... When I generate an
>official patch I would attribute the code appropriately...
>
>>
>> 2) Until anything similar to my proposal [1] is added, this would
>>    make daemon lose such domain on reload.
>>
>
>Hmm.. right... yuck - how easy one forgets about that.  Although it does
>seem you have some traction with the other series.  Or perhaps new logic
>that includes a flag that could get passed along to the PostParse
>function as well and checked in the "new" else condition from only the
>Live attach, update, detach paths (seems like overkill though).
>
>One thing still not resolved is that outside of virsh edit, one wouldn't
>be able to remove the device using virsh detach-device
>

I'd say it's OK that you have to re-define it.  You'd have to
re-define it anyway.  While on that note, I'd gladly welcome any
opinions in that thread about the invalid domain XML loading ;)

>John
>
>> Martin
>>
>> [1]
>> https://www.redhat.com/archives/libvir-list/2015-September/msg00698.html
>>
>
>[...]
-------------- 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/20150930/36057967/attachment-0001.sig>