On Tue, Aug 04, 2015 at 10:21:29AM -0400, John Ferlan wrote:
On 08/03/2015 09:57 AM, Ján Tomko wrote:
> On Wed, Jul 22, 2015 at 10:54:34AM -0400, John Ferlan wrote:
>>
https://bugzilla.redhat.com/show_bug.cgi?id=1210587 (completed)
>>
>> When generating the default drive address for a SCSI <disk> device,
>> check the generated address to ensure it doesn't conflict with a SCSI
>> <hostdev> address. The <disk> address generation algorithm uses the
>> <target> "dev" name in order to determine which controller and
unit
>> in order to place the device. Since a SCSI <hostdev> device doesn't
>> require a target device name, its placement on the guest SCSI address
>> "could" conflict. For instance, if a SCSI <hostdev> exists at
>> controller=0 unit=0 and an attempt to hotplug 'sda' into the guest
>> made, there would be a conflict if the <hostdev> is already using
>> /dev/sda.
>>
>> Signed-off-by: John Ferlan <jferlan(a)redhat.com>
>> ---
>> src/conf/domain_conf.c | 16 ++++++++++++++--
>> src/conf/domain_conf.h | 3 ++-
>> src/qemu/qemu_command.c | 4 ++--
>> src/vmx/vmx.c | 22 ++++++++++++----------
>> src/vmx/vmx.h | 3 ++-
>> 5 files changed, 32 insertions(+), 16 deletions(-)
>>
>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>> index 39a4cf8..0dac60c 100644
>> --- a/src/conf/domain_conf.c
>> +++ b/src/conf/domain_conf.c
>> @@ -4079,7 +4079,7 @@ virDomainDeviceDefPostParseInternal(virDomainDeviceDefPtr
dev,
>> }
>>
>> if (disk->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE
&&
>> - virDomainDiskDefAssignAddress(xmlopt, disk) < 0)
>> + virDomainDiskDefAssignAddress(xmlopt, disk, def) < 0)
>> return -1;
>> }
>>
>> @@ -5860,7 +5860,8 @@ virDomainDiskDiffersSourceOnly(virDomainDiskDefPtr disk,
>>
>> int
>> virDomainDiskDefAssignAddress(virDomainXMLOptionPtr xmlopt,
>> - virDomainDiskDefPtr def)
>> + virDomainDiskDefPtr def,
>> + const virDomainDef *vmdef)
>
> This function does not really do any 'real' assignment, it just
> translates the target name to a fixed address.
> Unlike PCI address assignment, where we find unoccupied slots based on
> the domain defition, the domain definition is not really needed here.
>
> Moving the address check conflict to virDomainDefPostParse, after
> the addresses have been filled out by virDomainDeviceDefPostParse,
> would remove the need to change all the function prototypes and it would
> be a good place to check for address conflicts between disks too - after
> this series, two drives with the same addresses are allowed, as long as
> they have different target names.
>
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.
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 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 :))
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.
Jan
If you have some specific suggestions for ways to handle this -
I'm
certainly open to reconsidering, but because of the existing hotplug
paths which don't call virDomainDefPostParse I don't see it as a viable
solution to the "whole" problem.
John
--
libvir-list mailing list
libvir-list(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list