[libvirt] [PATCH v2 12/12] conf: Check for hostdev conflicts when assign default disk address

[John Ferlan]

...

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