...
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