On 06/11/2018 10:42 AM, Ján Tomko wrote: > On Wed, Jun 06, 2018 at 10:44:10AM -0400, John Ferlan wrote: >> https://bugzilla.redhat.com/show_bug.cgi?id=1559867 >> >> Add a check if the incoming <hostdev ...> with specified <address> >> already exists and if so fail the attach. >> > > Looking at the reproducer steps, the bug talks about libvirt > auto-generating a duplicate address, not user-specified input. > Oh, right - perhaps should have been clearer what's happening with the <hostdev> addition. While/when not provided, the post parse processing for a hostdev will automagically generate the address... of course in the code I was looking at that <address> was there and essentially copied the check from VIR_DOMAIN_DEVICE_RNG and that's what was on my mind when I wrote the terse commit message. >> Signed-off-by: John Ferlan <jferlan@xxxxxxxxxx> >> --- >> 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 38ea865ce3..7b4cdbcdcf 100644 >> --- a/src/qemu/qemu_driver.c >> +++ b/src/qemu/qemu_driver.c >> @@ -8014,6 +8014,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; >> + } > > This does not fix the problem of autogenerating the address in any way > (which seems to work when I call attach --config while the domain is not > running). True, but it does avoid the attempt to attach a duplicated address - still I see now it's the autogeneration of a <hostdev> address that's broken when a domain is running and the attachment is for the config. When just looking at the virDomainHostdevAssignAddress code one could believe sure this should provide the right answer, but unlike the SCSI option for virDomainDiskDefAssignAddress which makes use of the def->dst in order to "seed" the @idx for the subsequent equations the hostdev code doesn't have a name so it linearly looks for an available address during post parse processing. In this instance however, it's using the running domain's vm->def instead of the persistent vm->newDef, thus obtaining the same answer twice with the second one being incorrect. Updated patches will be posted soon. > > Also, if we needded additional checks for guest addresses on attach, > we should either do it for all devices (and drop the calling of > per-domain postParse after adding a single device), or take care of the > conflicts in postParse. > Not 100% sure what you mean by this. Still the processing of a SCSI hostdev relies not only on existing SCSI hostdev's, but also any existing SCSI disk - so making the correct calculation is challenging. Whether that same "need" exists for other cold attach devices (net, lease, controller, chrdev, fs, rng, memory, redirdev, shmem, watchdog, and vsock) would seem to extend beyond the bounds of this bz. Thanks for the review - John -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
