... >> After looking through the code and thinking more about this - using >> virDomainDefPostParse won't work for the hotplug case since it's only >> going through virDomainDeviceDefParse and virDomainDeviceDefPostParse >> code in order to validate whether the address (whether provided or >> generated) is duplicated. >> >> Leaving the vmdef in virDomainDiskDefAssignAddress allows for the check >> of the generated device address based on the name to be compared against >> known hostdev addresses to ensure there isn't a conflict. Since the >> knowledge of what device type is being used is there - it just seems >> more natural to make the check there rather than repeating the same >> check in multiple callers. > > Okay, auto-generating device data based on the whole domain definition > makes sense in DeviceDefPostParse. Reading my reply, it seems I thought > leaving that check there would not play well with checking the drive > addresses against all devices, not just the devices of the "opposite" > type. > > I still do not like using vmdef in virDomainHostdevDefParseXML. > Removed, but I'll submit a follow-up patch rather than risk any wrath from pushing an unreviewed patch which essentially does this ;-) >> >> With respect to the other issue you note - that someone can provide >> duplicate drive addresses for either disk or hostdev - that's a slightly >> different issue, but is resolvable as well. Of course it's perhaps also >> true of other address types - that is I'm not sure that problem is >> 'drive' specific. It seems a separate patch could use the >> virDomainDefHasDeviceAddressIterator in some way to check all addresses >> for duplicates. >> > > The only difference I see here is that the disk address might not have > been provided by user, but generated by libvirt based on the disk > target. > > Checking for conflicts with devices of another type while leaving out > devices of the same type seems incomplete. > I don't disagree, but it's a more "general" problem and started getting more complex than "just" checking disks and/or scsi subsys hostdev's whivch was the focus of this current bug/effort. I do have a local branch started which I hope to focus on shortly - perhaps after wandering through some USB patches. >> I believe the existing patches should remain as is: >> >> Patch 9 - checks that the provided 'drive' address for a SCSI hostdev >> doesn't conflict with any current disk address. Since the disks would >> be parsed first this works to ensure there are no generated or provided >> address conflicts >> > > I would rather see the currently unused vmdef attribute removed from > virDomainHostdevDefParseXML. It seems the check would be just as > effective in DeviceDefPostParse. > > ACK with the attribute removed and the check moved to PostParse. > (If you still believe it should reamin as is, just resend it and wait > for an ACK from someone else :)) > Done - the check was moved to PostParse and works fine. I guess I focused more on the 'disk' problem than the fact that 'hostdev' can either be user supplied or libvirt generated based on what's already in use as opposed to causing a specific usage based solely on the target. >> Patch 11 - just sets up to make patch 12 appear cleaner >> >> Patch 12 - checks to ensure the generated address based on name doesn't >> conflict with some defined SCSI hostdev address. >> > > ACK to these two. While they do not catch all the cases, they at least > check that the address generated by libvirt does not clash with user's > input. > I pushed the adjusted 10, 11, and 12 and have posted a remove 'vmdef' from the virDomainHostdevDefParseXML patch. Tks - John -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list