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

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(a)redhat.com&gt; >> Date: Tue, 29 Sep 2015 17:04:11 -0400 >> Subject: [PATCH] bug 1257844 >> >> Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; >> --- >> 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

John > Martin > > [1] > https://www.redhat.com/archives/libvir-list/2015-September/msg00698.html > [...]