9:50 a.m.
On Thu, Jul 16, 2015 at 11:57:52AM -0400, John Ferlan wrote:
On 07/16/2015 08:23 AM, Ján Tomko wrote:
> On Mon, Jun 22, 2015 at 05:05:07PM -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 | 40 +++++++++++++++++++++++++++++-----------
>> 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, 47 insertions(+), 25 deletions(-)
>>
>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>> index 6259d4a..0999e86 100644
>> --- a/src/conf/domain_conf.c
>> +++ b/src/conf/domain_conf.c
>> @@ -5672,7 +5672,8 @@ virDomainDiskFindByBusAndDst(virDomainDefPtr def,
>>
>> int
>> virDomainDiskDefAssignAddress(virDomainXMLOptionPtr xmlopt,
>> - virDomainDiskDefPtr def)
>> + virDomainDiskDefPtr def,
>> + const virDomainDef *vmdef)
>> {
>> int idx = virDiskNameToIndex(def->dst);
>> if (idx < 0) {
>> @@ -5683,7 +5684,10 @@ virDomainDiskDefAssignAddress(virDomainXMLOptionPtr
xmlopt,
>> }
>>
>> switch (def->bus) {
>> - case VIR_DOMAIN_DISK_BUS_SCSI:
>> + case VIR_DOMAIN_DISK_BUS_SCSI: {
>> + unsigned int controller;
>> + unsigned int unit;
>> +
>> def->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE;
>>
>> if (xmlopt->config.hasWideSCSIBus) {
>> @@ -5692,22 +5696,36 @@ virDomainDiskDefAssignAddress(virDomainXMLOptionPtr
xmlopt,
>> * Unit 7 is the SCSI controller itself. Therefore unit 7
>> * cannot be assigned to disks and is skipped.
>> */
>> - def->info.addr.drive.controller = idx / 15;
>> - def->info.addr.drive.bus = 0;
>> - def->info.addr.drive.unit = idx % 15;
>> + controller = idx / 15;
>> + unit = idx % 15;
>>
>> /* Skip the SCSI controller at unit 7 */
>> - if (def->info.addr.drive.unit >= 7)
>> - ++def->info.addr.drive.unit;
>> + if (unit >= 7)
>> + ++unit;
>> } else {
>> /* For a narrow SCSI bus we define the default mapping to be
>> * 7 units per bus, 1 bus per controller, many controllers */
>> - def->info.addr.drive.controller = idx / 7;
>> - def->info.addr.drive.bus = 0;
>> - def->info.addr.drive.unit = idx % 7;
>> + controller = idx / 7;
>> + unit = idx % 7;
>> + }
>> +
>
> This mixes non-functional and functional changes.
>
OK - I can separate out out just the non-functional into its own patch.
Do you want me to send it out again or trust that I know what I'm doing?
Both. I did not give it an 'ACK with that split out' because I expected
another series with the address generation done after all is parsed.
>> + if (virDomainDriveAddressIsUsedByHostdev(vmdef,
>> +
VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI,
>> + controller, 0, 0, unit)) {
>> + virReportError(VIR_ERR_XML_ERROR,
>> + _("address generated using disk target name
'%s' "
>> + "may conflict with SCSI host device address
"
>> + "controller='%u' bus='%u'
target='%u' unit='%u"),
>> + def->dst, controller, 0, 0, unit);
>
The disks are parsed before hostdevs, so this will only catch conflicts
on hotplug.
> 'may conflict' sounds ambiguous. Either it does conflict
and we should
> reject it, or it does not and there is no error.
>
> And this seems like a problem of libvirt's address generation algorithm,
> not an XML error.
>
> Jan
>
Right - it does conflict...
The issue is that the 'idx' is generated as a result of the target
device name ("dst"). The assumption being "sda" would be controller
=
unit = 0, 'sdb' would be ctlr = 0, unit = 1, 'sdc' would be ctlr = 0,
unit = 2, etc.
However, for hostdev's there is no such "dst" logic - it's a straight
assignment.
When the guest boots, whatever is assigned to "c = 0, u = 0" is assigned
to 'sda' (regardless of whether it's a hostdev or disk).
So if the hostdev which had it's address provided in the XML, but the
disk which didn't *and* is using "<target dev='sda'.../>",
then there's
a possible conflict because "sda" resolves to "c = 0, b = 0". The
error
message I guess should thus state :
"using disk target name '%s' conflicts with SCSI host device address
..."
Would you still call that an XML error or perhaps CONFIG_UNSUPPORTED
Oh, so the targets are not really guaranteed. I guess that could be
considered an XML error, even if it is caused by our interpretation
of the (mandatory) target attribute.
Jan