Re: [libvirt] [REPOST PATCHv2 1/2] qemu: Check for existing hostdev address for cold attach device

On 07/09/2018 10:32 AM, Michal Privoznik wrote: > On 07/06/2018 08:50 PM, John Ferlan wrote: >> Prior to the hostdev being inserted in the hostdevs list, >> add a check during qemuDomainAttachDeviceConfig to determine >> whether the new/incoming <hostdev ...> device is providing >> the same <address> as some existing hostdev on the list >> and if so fail the cold attach. >> >> This cannot be done during virDomainHostdevDefPostParse >> because that is called after virDomainDefParseXML would >> have inserted a hostdev onto the hostdevs list and thus >> would have a "conflict" with itself. Therefore, the post >> parse processing can only compare if the current hostdev >> address conflicts with a SCSI <disk> address. >> >> By comparison this is similar to the validation phase >> checks in virDomainDefCheckDuplicateDriveAddresses that >> occur during define/startup processing but are not run >> during config attach of a live guest. >> >> Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; >> --- >> src/qemu/qemu_driver.c | 6 ++++++ >> 1 file changed, 6 insertions(+) >> >> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c >> index 9a35e04a85..ef1abe3f68 100644 >> --- a/src/qemu/qemu_driver.c >> +++ b/src/qemu/qemu_driver.c >> @@ -8007,6 +8007,12 @@ qemuDomainAttachDeviceConfig(virDomainDefPtr vmdef, >> _("device is already in the domain configuration")); >> return -1; >> } >> + if (dev->data.hostdev->info->type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE && >> + virDomainDefHasDeviceAddress(vmdef, dev->data.hostdev->info)) { >> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", >> + _("a device with the same address already exists ")); >> + return -1; >> + } >> if (virDomainHostdevInsert(vmdef, hostdev)) >> return -1; >> dev->data.hostdev = NULL; >> > > I think hostdevs are not the only type of device suffering from this. In > fact, I've just tested disks and libvirt accepts attaching another disk > onto same <address/> happily. > > I wonder if this should go into virDomainDefCompatibleDevice() (now that > we have @action there ;-) ). > It's a case of myopia for the bug I'm working on as listed in patch2. What I did is no different than the VIR_DOMAIN_DEVICE_RNG check in the same function. Still, if I change this patch to add: if (action == VIR_DOMAIN_DEVICE_ACTION_ATTACH && !live && data.newInfo && data.newInfo->type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE && virDomainDefHasDeviceAddress(def, data.newInfo)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("a device with the same address already exists ")); return -1; } to virDomainDefCompatibleDevice and remove the (new)hostdev and (existing)rng checks, then I believe it covers the existing cases.