Re: [PATCH v2] conf: Generate address for scsi host device automatically

[Osier Yang <jyang@xxxxxxxxxx>]

On 28/05/13 23:59, Osier Yang wrote:
> On 28/05/13 23:09, John Ferlan wrote:
> > On 05/23/2013 12:48 PM, Osier Yang wrote:
> > > With unknown good reasons, the attribute "bus" of scsi device
> > > address is always set to 0, same for attribute "target". (See
> > > virDomainDiskDefAssignAddress).
> > >
> > > Though we might need to change the algorithm to honor "bus"
> > > and "target" too, that's a different issue. The address generator
> > >
> > > for scsi host device in this patch just follows the unknown
> > > good reasons, only considering the "controller" and "unit".
> > > It walks through all scsi controllers and their units, to see
> > > if the address $controller:0:0:$unit can be used, if found
> > > one, it sits on it, otherwise, it creates a new controller
> > > (actually the controller is implicitly created by someone
> > > else), and sits on $new_controller:0:0:0 instead.
> > >
> > > ---
> > > v1 - v2:
> > >    * A new helper virDomainSCSIDriveAddressIsUsed
> > >    * Move virDomainDefMaybeAddHostdevSCSIcontroller
> > >    * More comments
> > >    * Add testing.
> > >    * problems fixed with the testing:
> > >      1) s/nscsi_controllers + 1/nscsi_controllers/,
> > >      2) Add the controller implicitly after a scsi hostdev def
> > >         parsing, as it can use a new scsi controller index
> > >         (nscsi_controllers), which should be added implicitly.
> > First pass of code is found at:
> >
> > https://www.redhat.com/archives/libvir-list/2013-May/msg00340.html
> >
> > The previous/initial review and reply is found at:
> >
> > https://www.redhat.com/archives/libvir-list/2013-May/msg00506.html
> >
> > https://www.redhat.com/archives/libvir-list/2013-May/msg00526.html
> >
> > > ---
> > >   src/conf/domain_conf.c                             | 209 
> > > ++++++++++++++++++---
> > >   .../qemuxml2argv-hostdev-scsi-autogen-address.xml  |  95 ++++++++++
> > >   ...qemuxml2xmlout-hostdev-scsi-autogen-address.xml | 106 +++++++++++
> > >   tests/qemuxml2xmltest.c                            |   2 +
> > >   4 files changed, 382 insertions(+), 30 deletions(-)
> > >   create mode 100644 
> > > tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-autogen-address.xml
> > >   create mode 100644 
> > > tests/qemuxml2xmloutdata/qemuxml2xmlout-hostdev-scsi-autogen-address.xml 
> > >
> > >
> > > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> > > index ad5550c..86d743b 100644
> > > --- a/src/conf/domain_conf.c
> > > +++ b/src/conf/domain_conf.c
> > > @@ -3782,6 +3782,148 @@ cleanup:
> > >       return ret;
> > >   }
> > >   +/* Check if a drive type address $controller:0:0:$unit is already
> > > + * taken by a disk or not.
> > > + */
> > > +static bool
> > > +virDomainDriveAddressIsUsedByDisk(virDomainDefPtr def,
> > > +                                  enum virDomainDiskBus type,
> > > +                                  unsigned int controller,
> > > +                                  unsigned int unit)
> > > +{
> > > +    virDomainDiskDefPtr disk;
> > > +    int i;
> > > +
> > > +    for (i = 0; i < def->ndisks; i++) {
> > > +        disk = def->disks[i];
> > > +
> > > +        if (disk->bus != type ||
> > > +            disk->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE)
> > > +            continue;
> > > +
> > > +        if (disk->info.addr.drive.controller == controller &&
> > > +            disk->info.addr.drive.unit == unit &&
> > > +            disk->info.addr.drive.bus == 0 &&
> > > +            disk->info.addr.drive.target == 0)
> > > +            return true;
> > > +    }
> > > +
> > > +    return false;
> > > +}
> > > +
> > > +/* Check if a drive type address $controller:0:0:$unit is already
> > > + * taken by a host device or not.
> > > + */
> > > +static bool
> > > +virDomainDriveAddressIsUsedByHostdev(virDomainDefPtr def,
> > > +                                     enum 
> > > virDomainHostdevSubsysType type,
> > > +                                     unsigned int controller,
> > > +                                     unsigned int unit)
> > > +{
> > > +    virDomainHostdevDefPtr hostdev;
> > > +    int i;
> > > +
> > > +    for (i = 0; i < def->nhostdevs; i++) {
> > > +        hostdev = def->hostdevs[i];
> > > +
> > > +        if (hostdev->source.subsys.type != type)
> > > +            continue;
> > > +
> > > +        if (hostdev->info->addr.drive.controller == controller &&
> > > +            hostdev->info->addr.drive.unit == unit &&
> > > +            hostdev->info->addr.drive.bus == 0 &&
> > > +            hostdev->info->addr.drive.target == 0)
> > > +            return true;
> > > +    }
> > > +
> > > +    return false;
> > > +}
> > > +
> > > +static bool
> > > +virDomainSCSIDriveAddressIsUsed(virDomainDefPtr def,
> > > +                                unsigned int controller,
> > > +                                unsigned int unit)
> > > +{
> > > +    /* In current implementation, the maximum unit number of a 
> > > controller
> > > +     * is either 16 or 7 (narrow SCSI bus), and if the maximum unit 
> > > number
> > > +     * is 16, the controller itself is on unit 7 */
> > > +    if (unit == 7)
> >
> > FYI: The previous code cared that a wide address was being used when
> > returning true - this code doesn't.  Just making sure that change is
> > intentional.  Essentially, 7 cannot be used.  The comments above the
> > code talk about wide/narrow which it seems becomes irrelevant. The
> > reality seems to be that you are avoiding using 7 altogether because
> > it's reserved for the controller itself, correct?  Regardless of whether
> > a wide or narrow.  The controller itself uses the unit number of the
> > maximum value of the narrow band.  At least that's how I'm reading it.
>
> You are right, will update the comment.

Actually no, I was confused. For the narrow scsi bus, the unit can't be 7,
so I just shortened the statement by removing the checking for max_unit 
== 16

I don't know why the units on the narrow scsi bus is 7 though, it should 
be 8
instead. See:

http://en.wikipedia.org/wiki/SCSI

> > > +        return true;
> > > +
> > > +    if (virDomainDriveAddressIsUsedByDisk(def, 
> > > VIR_DOMAIN_DISK_BUS_SCSI,
> > > +                                          controller, unit) ||
> > > +        virDomainDriveAddressIsUsedByHostdev(def, 
> > > VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI,
> > > +                                             controller, unit))
> > > +}
> > > +
> > > +/* Find out the next usable "unit" of a specific controller */
> > > +static int
> > > +virDomainControllerSCSINextUnit(virDomainDefPtr def,
> > > +                                unsigned int max_unit,
> > > +                                unsigned int controller)
> > > +{
> > > +    int i;
> > > +
> > > +    for (i = 0; i < max_unit; i++) {
> > > +        if (!virDomainSCSIDriveAddressIsUsed(def, controller, i))
> > > +            return i;
> > > +    }
> > > +
> > > +    return -1;
> > > +}
> > > +
> > > +#define SCSI_WIDE_BUS_MAX_CONT_UNIT 16
> > > +#define SCSI_NARROW_BUS_MAX_CONT_UNIT 7
> > > +
> > > +static int
> > > +virDomainHostdevAssignAddress(virDomainXMLOptionPtr xmlopt,
> > > +                              virDomainDefPtr def,
> > > +                              virDomainHostdevDefPtr hostdev)
> > > +{
> > > +    int next_unit;
> > > +    unsigned nscsi_controllers = 0;
> > > +    bool found = false;
> > > +    int i;
> > > +
> > > +    if (hostdev->source.subsys.type != 
> > > VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI)
> > > +        return -1;
> > > +
> > > +    for (i = 0; i < def->ncontrollers; i++) {
> > As a for loop exit condition, consider either using
> >
> >                                           && !found
> > or
> >
> >                                           && next_unit == -1
> >
> > as long as you initialize next_unit = -1 above...
> >
> > > +        if (def->controllers[i]->type != 
> > > VIR_DOMAIN_CONTROLLER_TYPE_SCSI)
> > > +            continue;
> > > +
> > > +        nscsi_controllers++;
> > > +        next_unit = virDomainControllerSCSINextUnit(def,
> > > + xmlopt->config.hasWideScsiBus ?
> > > + SCSI_WIDE_BUS_MAX_CONT_UNIT :
> > > + SCSI_NARROW_BUS_MAX_CONT_UNIT,
> > > + def->controllers[i]->idx);
> > > +        if (next_unit >= 0) {
> > > +            found = true;
> > > +            break;
> > break; is irrelevant using !found in loop exit or the whole statement is
> > irrelevent if using next_unit == -1 for loop exit...
> >
> > > +        }
> > > +    }
> > > +
> > > +    hostdev->info->type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE;
> > > +
> > > +    if (found) {
> > > +        hostdev->info->addr.drive.controller = 
> > > def->controllers[i]->idx;
> > > +        hostdev->info->addr.drive.bus = 0;
> > > +        hostdev->info->addr.drive.target = 0;
> > > +        hostdev->info->addr.drive.unit = next_unit;
> > > +    } else {
> > > +        hostdev->info->addr.drive.controller = nscsi_controllers;
> > > +        hostdev->info->addr.drive.bus = 0;
> > > +        hostdev->info->addr.drive.target = 0;
> > > +        hostdev->info->addr.drive.unit = 0;
> > > +    }
> >
> > Although it looked odd initially, I think I convinced myself the logic
> > is OK. Initially, nscsi_controllers = 0;
> >
> > If none exist, then 'controller = 0' and 'units' are assigned until
> > full, right?
>
> Right.
>
> > If one exists and is full, then 'controller = 1' and is
> > placed at unit = 0.
>
> Right.
>
> > NOTE:
> > Using next_unit to break the loop means this code could be rewritten as:
> >
> > hostdev->info->addr.drive.controller = (next_unit >= 0 ?
> > def->controllers[i]->idx :
> >                                          nscsi_controllers)
> > hostdev->info->addr.drive.bus = 0;
> > hostdev->info->addr.drive.target = 0;
> > hostdev->info->addr.drive.unit = (next_unit >= 0 ? next_unit : 0);
> >
> > Although one could argue setting "= 0;" is redundant due to allocation.
>
> Compact. I will consider your suggestion for the loop in v3.
>
> > > +
> > > +    return 0;
> > > +}
> > > +
> > >   static int
> > >   virDomainHostdevDefParseXMLSubsys(xmlNodePtr node,
> > >                                     xmlXPathContextPtr ctxt,
> > > @@ -8802,7 +8944,9 @@ error:
> > >   }
> > >     static virDomainHostdevDefPtr
> > > -virDomainHostdevDefParseXML(const xmlNodePtr node,
> > > +virDomainHostdevDefParseXML(virDomainXMLOptionPtr xmlopt,
> > > +                            virDomainDefPtr vmdef,
> > > +                            const xmlNodePtr node,
> > >                               xmlXPathContextPtr ctxt,
> > >                               virHashTablePtr bootHash,
> > >                               unsigned int flags)
> > > @@ -8862,7 +9006,9 @@ virDomainHostdevDefParseXML(const xmlNodePtr 
> > > node,
> > >               }
> > >               break;
> > >           case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI:
> > > -            if (def->info->type == 
> > > VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) {
> > > +            if (def->info->type == 
> > > VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE &&
> > > +                virDomainHostdevAssignAddress(xmlopt, vmdef, def) < 
> > > 0) {
> > > +
> > >                   virReportError(VIR_ERR_XML_ERROR, "%s",
> > >                                  _("SCSI host devices must have 
> > > address specified"));
> > >                   goto error;
> > > @@ -9271,8 +9417,8 @@ virDomainDeviceDefParse(const char *xmlStr,
> > >               goto error;
> > >       } else if (xmlStrEqual(node->name, BAD_CAST "hostdev")) {
> > >           dev->type = VIR_DOMAIN_DEVICE_HOSTDEV;
> > > -        if (!(dev->data.hostdev = virDomainHostdevDefParseXML(node, 
> > > ctxt, NULL,
> > > - flags)))
> > > +        if (!(dev->data.hostdev = 
> > > virDomainHostdevDefParseXML(xmlopt, def, node,
> > > + ctxt, NULL, flags)))
> > >               goto error;
> > >       } else if (xmlStrEqual(node->name, BAD_CAST "controller")) {
> > >           dev->type = VIR_DOMAIN_DEVICE_CONTROLLER;
> > > @@ -10255,6 +10401,30 @@ error:
> > >       return NULL;
> > >   }
> > >   +static int
> > > +virDomainDefMaybeAddHostdevSCSIcontroller(virDomainDefPtr def)
> > > +{
> > > +    /* Look for any hostdev scsi dev */
> > > +    int i;
> > > +    int maxController = -1;
> > > +    virDomainHostdevDefPtr hostdev;
> > > +
> > > +    for (i = 0; i < def->nhostdevs; i++) {
> > > +        hostdev = def->hostdevs[i];
> > > +        if (hostdev->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS &&
> > > +            hostdev->source.subsys.type == 
> > > VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI &&
> > > +            (int)hostdev->info->addr.drive.controller > 
> > > maxController) {
> > > +            maxController = hostdev->info->addr.drive.controller;
> > > +        }
> > > +    }
> > > +
> > > +    for (i = 0; i <= maxController; i++) {
> > > +        if (virDomainDefMaybeAddController(def, 
> > > VIR_DOMAIN_CONTROLLER_TYPE_SCSI, i, -1) < 0)
> > > +            return -1;
> > > +    }
> > > +
> > > +    return 0;
> > > +}
> > >     static virDomainDefPtr
> > >   virDomainDefParseXML(xmlDocPtr xml,
> > > @@ -11618,7 +11788,8 @@ virDomainDefParseXML(xmlDocPtr xml,
> > >       for (i = 0; i < n; i++) {
> > >           virDomainHostdevDefPtr hostdev;
> > >   -        hostdev = virDomainHostdevDefParseXML(nodes[i], ctxt, 
> > > bootHash, flags);
> > > +        hostdev = virDomainHostdevDefParseXML(xmlopt, def, nodes[i],
> > > +                                              ctxt, bootHash, flags);
> > >           if (!hostdev)
> > >               goto error;
> > >   @@ -11631,6 +11802,9 @@ virDomainDefParseXML(xmlDocPtr xml,
> > >           }
> > >             def->hostdevs[def->nhostdevs++] = hostdev;
> > > +
> > > +        if (virDomainDefMaybeAddHostdevSCSIcontroller(def) < 0)
> > > +            goto error;
> > >       }
> > >       VIR_FREE(nodes);
> > >   @@ -13232,31 +13406,6 @@ 
> > > virDomainDefMaybeAddSmartcardController(virDomainDefPtr def)
> > >       return 0;
> > >   }
> > >   -static int
> > > -virDomainDefMaybeAddHostdevSCSIcontroller(virDomainDefPtr def)
> > > -{
> > > -    /* Look for any hostdev scsi dev */
> > > -    int i;
> > > -    int maxController = -1;
> > > -    virDomainHostdevDefPtr hostdev;
> > > -
> > > -    for (i = 0; i < def->nhostdevs; i++) {
> > > -        hostdev = def->hostdevs[i];
> > > -        if (hostdev->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS &&
> > > -            hostdev->source.subsys.type == 
> > > VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI &&
> > > -            (int)hostdev->info->addr.drive.controller > 
> > > maxController) {
> > > -            maxController = hostdev->info->addr.drive.controller;
> > > -        }
> > > -    }
> > > -
> > > -    for (i = 0; i <= maxController; i++) {
> > > -        if (virDomainDefMaybeAddController(def, 
> > > VIR_DOMAIN_CONTROLLER_TYPE_SCSI, i, -1) < 0)
> > > -            return -1;
> > > -    }
> > > -
> > > -    return 0;
> > > -}
> > > -
> > >   /*
> > >    * Based on the declared <address/> info for any devices,
> > >    * add necessary drive controllers which are not already present
> > > diff --git 
> > > a/tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-autogen-address.xml 
> > > b/tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-autogen-address.xml
> > > new file mode 100644
> > > index 0000000..21f112b
> > > --- /dev/null
> > > +++ 
> > > b/tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-autogen-address.xml
> > > @@ -0,0 +1,95 @@
> > > +<domain type='qemu'>
> > > +  <name>QEMUGuest2</name>
> > > + <uuid>c7a5fdbd-edaf-9466-926a-d65c16db1809</uuid>
> > > +  <memory unit='KiB'>219100</memory>
> > > +  <currentMemory unit='KiB'>219100</currentMemory>
> > > +  <vcpu placement='static'>1</vcpu>
> > > +  <os>
> > > +    <type arch='i686' machine='pc'>hvm</type>
> > > +    <boot dev='hd'/>
> > > +  </os>
> > > +  <clock offset='utc'/>
> > > +  <on_poweroff>destroy</on_poweroff>
> > > +  <on_reboot>restart</on_reboot>
> > > +  <on_crash>destroy</on_crash>
> > > +  <devices>
> > > +    <emulator>/usr/bin/qemu</emulator>
> > > +    <disk type='block' device='disk'>
> > > +      <source dev='/dev/HostVG/QEMUGuest2'/>
> > > +      <target dev='hda' bus='ide'/>
> > > +      <address type='drive' controller='0' bus='0' target='0' 
> > > unit='0'/>
> > > +    </disk>
> > > +    <controller type='scsi' index='0' model='virtio-scsi'/>
> > > +    <controller type='usb' index='0'/>
> > > +    <controller type='ide' index='0'/>
> > > +    <controller type='pci' index='0' model='pci-root'/>
> > > +    <hostdev mode='subsystem' type='scsi' managed='yes'>
> > > +      <source>
> > > +        <adapter name='scsi_host0'/>
> > > +        <address bus='0' target='0' unit='0'/>
> > > +      </source>
> > > +    </hostdev>
> > > +    <hostdev mode='subsystem' type='scsi' managed='yes'>
> > > +      <source>
> > > +        <adapter name='scsi_host1'/>
> > > +        <address bus='0' target='0' unit='1'/>
> > > +      </source>
> > > +    </hostdev>
> > > +    <hostdev mode='subsystem' type='scsi' managed='yes'>
> > > +      <source>
> > > +        <adapter name='scsi_host2'/>
> > > +        <address bus='0' target='0' unit='2'/>
> > > +      </source>
> > > +    </hostdev>
> > > +    <hostdev mode='subsystem' type='scsi' managed='yes'>
> > > +      <source>
> > > +        <adapter name='scsi_host3'/>
> > > +        <address bus='0' target='0' unit='3'/>
> > > +      </source>
> > > +    </hostdev>
> > > +    <hostdev mode='subsystem' type='scsi' managed='yes'>
> > > +      <source>
> > > +        <adapter name='scsi_host4'/>
> > > +        <address bus='0' target='0' unit='4'/>
> > > +      </source>
> > > +    </hostdev>
> > > +    <hostdev mode='subsystem' type='scsi' managed='yes'>
> > > +      <source>
> > > +        <adapter name='scsi_host5'/>
> > > +        <address bus='0' target='0' unit='5'/>
> > > +      </source>
> > > +    </hostdev>
> > > +    <hostdev mode='subsystem' type='scsi' managed='yes'>
> > > +      <source>
> > > +        <adapter name='scsi_host6'/>
> > > +        <address bus='0' target='0' unit='6'/>
> > > +      </source>
> > > +    </hostdev>
> > > +    <hostdev mode='subsystem' type='scsi' managed='yes'>
> > > +      <source>
> > > +        <adapter name='scsi_host7'/>
> > > +        <address bus='0' target='0' unit='7'/>
> > > +      </source>
> > > +    </hostdev>
> > > +    <hostdev mode='subsystem' type='scsi' managed='yes'>
> > > +      <source>
> > > +        <adapter name='scsi_host8'/>
> > > +        <address bus='0' target='0' unit='8'/>
> > > +      </source>
> > > +    </hostdev>
> > > +    <hostdev mode='subsystem' type='scsi' managed='yes'>
> > > +      <source>
> > > +        <adapter name='scsi_host9'/>
> > > +        <address bus='0' target='0' unit='9'/>
> > > +      </source>
> > > +      <address type='drive' controller='1' bus='0' target='0' 
> > > unit='5'/>
> > [1] see below
> >
> > > +    </hostdev>
> > > +    <hostdev mode='subsystem' type='scsi' managed='yes'>
> > > +      <source>
> > > +        <adapter name='scsi_host10'/>
> > > +        <address bus='0' target='0' unit='10'/>
> > > +      </source>
> > > +    </hostdev>
> > > +    <memballoon model='virtio'/>
> > > +  </devices>
> > > +</domain>
> > > diff --git 
> > > a/tests/qemuxml2xmloutdata/qemuxml2xmlout-hostdev-scsi-autogen-address.xml 
> > > b/tests/qemuxml2xmloutdata/qemuxml2xmlout-hostdev-scsi-autogen-address.xml 
> > >
> > > new file mode 100644
> > > index 0000000..e416654
> > > --- /dev/null
> > > +++ 
> > > b/tests/qemuxml2xmloutdata/qemuxml2xmlout-hostdev-scsi-autogen-address.xml
> > > @@ -0,0 +1,106 @@
> > > +<domain type='qemu'>
> > > +  <name>QEMUGuest2</name>
> > > + <uuid>c7a5fdbd-edaf-9466-926a-d65c16db1809</uuid>
> > > +  <memory unit='KiB'>219100</memory>
> > > +  <currentMemory unit='KiB'>219100</currentMemory>
> > > +  <vcpu placement='static'>1</vcpu>
> > > +  <os>
> > > +    <type arch='i686' machine='pc'>hvm</type>
> > > +    <boot dev='hd'/>
> > > +  </os>
> > > +  <clock offset='utc'/>
> > > +  <on_poweroff>destroy</on_poweroff>
> > > +  <on_reboot>restart</on_reboot>
> > > +  <on_crash>destroy</on_crash>
> > > +  <devices>
> > > +    <emulator>/usr/bin/qemu</emulator>
> > > +    <disk type='block' device='disk'>
> > > +      <source dev='/dev/HostVG/QEMUGuest2'/>
> > > +      <target dev='hda' bus='ide'/>
> > > +      <address type='drive' controller='0' bus='0' target='0' 
> > > unit='0'/>
> > > +    </disk>
> > > +    <controller type='scsi' index='0' model='virtio-scsi'/>
> > > +    <controller type='usb' index='0'/>
> > > +    <controller type='ide' index='0'/>
> > > +    <controller type='pci' index='0' model='pci-root'/>
> > > +    <controller type='scsi' index='1'/>
> > > +    <hostdev mode='subsystem' type='scsi' managed='yes'>
> > > +      <source>
> > > +        <adapter name='scsi_host0'/>
> > > +        <address bus='0' target='0' unit='0'/>
> > > +      </source>
> > > +      <address type='drive' controller='0' bus='0' target='0' 
> > > unit='0'/>
> > > +    </hostdev>
> > > +    <hostdev mode='subsystem' type='scsi' managed='yes'>
> > > +      <source>
> > > +        <adapter name='scsi_host1'/>
> > > +        <address bus='0' target='0' unit='1'/>
> > > +      </source>
> > > +      <address type='drive' controller='0' bus='0' target='0' 
> > > unit='1'/>
> > > +    </hostdev>
> > > +    <hostdev mode='subsystem' type='scsi' managed='yes'>
> > > +      <source>
> > > +        <adapter name='scsi_host2'/>
> > > +        <address bus='0' target='0' unit='2'/>
> > > +      </source>
> > > +      <address type='drive' controller='0' bus='0' target='0' 
> > > unit='2'/>
> > > +    </hostdev>
> > > +    <hostdev mode='subsystem' type='scsi' managed='yes'>
> > > +      <source>
> > > +        <adapter name='scsi_host3'/>
> > > +        <address bus='0' target='0' unit='3'/>
> > > +      </source>
> > > +      <address type='drive' controller='0' bus='0' target='0' 
> > > unit='3'/>
> > > +    </hostdev>
> > > +    <hostdev mode='subsystem' type='scsi' managed='yes'>
> > > +      <source>
> > > +        <adapter name='scsi_host4'/>
> > > +        <address bus='0' target='0' unit='4'/>
> > > +      </source>
> > > +      <address type='drive' controller='0' bus='0' target='0' 
> > > unit='4'/>
> > > +    </hostdev>
> > > +    <hostdev mode='subsystem' type='scsi' managed='yes'>
> > > +      <source>
> > > +        <adapter name='scsi_host5'/>
> > > +        <address bus='0' target='0' unit='5'/>
> > > +      </source>
> > > +      <address type='drive' controller='0' bus='0' target='0' 
> > > unit='5'/>
> > > +    </hostdev>
> > > +    <hostdev mode='subsystem' type='scsi' managed='yes'>
> > > +      <source>
> > > +        <adapter name='scsi_host6'/>
> > > +        <address bus='0' target='0' unit='6'/>
> > > +      </source>
> > > +      <address type='drive' controller='0' bus='0' target='0' 
> > > unit='6'/>
> > > +    </hostdev>
> > > +    <hostdev mode='subsystem' type='scsi' managed='yes'>
> > > +      <source>
> > > +        <adapter name='scsi_host7'/>
> > > +        <address bus='0' target='0' unit='7'/>
> > > +      </source>
> > > +      <address type='drive' controller='1' bus='0' target='0' 
> > > unit='0'/>
> > > +    </hostdev>
> > > +    <hostdev mode='subsystem' type='scsi' managed='yes'>
> > > +      <source>
> > > +        <adapter name='scsi_host8'/>
> > > +        <address bus='0' target='0' unit='8'/>
> > > +      </source>
> > > +      <address type='drive' controller='1' bus='0' target='0' 
> > > unit='1'/>
> > > +    </hostdev>
> > > +    <hostdev mode='subsystem' type='scsi' managed='yes'>
> > > +      <source>
> > > +        <adapter name='scsi_host9'/>
> > > +        <address bus='0' target='0' unit='9'/>
> > > +      </source>
> > > +      <address type='drive' controller='1' bus='0' target='0' 
> > > unit='5'/>
> > > +    </hostdev>
> > > +    <hostdev mode='subsystem' type='scsi' managed='yes'>
> > > +      <source>
> > > +        <adapter name='scsi_host10'/>
> > > +        <address bus='0' target='0' unit='10'/>
> > > +      </source>
> > > +      <address type='drive' controller='1' bus='0' target='0' 
> > > unit='2'/>
> > > +    </hostdev>
> > > +    <memballoon model='virtio'/>
> > > +  </devices>
> > > +</domain>
> > > diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c
> > > index 64a271c..06e4206 100644
> > > --- a/tests/qemuxml2xmltest.c
> > > +++ b/tests/qemuxml2xmltest.c
> > > @@ -298,6 +298,8 @@ mymain(void)
> > >       DO_TEST("hostdev-scsi-shareable");
> > >       DO_TEST("hostdev-scsi-sgio");
> > >   +    DO_TEST_DIFFERENT("hostdev-scsi-autogen-address");
> > > +
> > [1]
> > Still don't understand the test setup 100%, but I think for the purposes
> > of what you're trying to prove that the defined unit=5 above should be 1
> > or 2 and then ensure that '3' gets created to prove the algorithm will
> > find one at '1' or '2'.  As it stands all that seems to be provided is
> > the scsi10 will use '3' even though '5' exists.
>
> Yes, I want to prove it will allocate the smallest available unit number,
> instead of increasing on top of the current largest unit number.
>
> /me should document this in the commit log.
>
> > Curious to understand why the 2nd controller gets created without the
> > "model='virtio-scsi'"?  Does it matter? Does creating a controller on
> > the fly like this would seem to imply that the controllers would be 
> > alike.
>
> It's a problem actually, I intended to write a RFC for it, but not posted
> yet. Assuming that one wants to hotplug a virtio-scsi disk with XML:
>
>     <disk type='file' device='disk'>
>       <driver name='qemu' type='raw'/>
>       <source file='/var/lib/libvirt/images/f18_sdb.img'/>
>       <target dev='sda' bus='scsi'/>
>       <address type='drive' controller='1' bus='0' target='0' unit='0'/>
>     </disk>
>
> If there is no scsi controller with index "1", the implicitly added 
> scsi controller
> will use the default scsi controller model "lsilogic" (true for 
> qemu/kvm). That's
> obviously not what the user wants.
>
> Though we provide the way to hotplug a controller, one can hotplug a
> controller first, and then hotplug the disk. But it's just a bit 
> better, as it still
> needs some manual work.
>
> There are many other problems, I'm going to write them out when posting
> the RFC to solve the problem (I don't have a good thought yet though).
>
> > Also what is the delineation between a narrow vs. wide bus in the xml?
> > It seems you'd want to have a test that ensures that a wide scsi bus
> > skips unit=7 when adding 10 devices, right?
> I wanted to add test for the wide scsi bus too, but the test qemu only 
> supports
> narrow scsi bus, I didn't dig it more, as I thought it may not be 
> deserved to
> do. The 10 devices in this test is to prove a new controller can be 
> added, and the
> address can be allocated as expected.

QEMU driver never supported wide SCSI bus yet. The "wide scsi bus" support
was added by Matthias for ESX driver. See:

http://www.redhat.com/archives/libvir-list/2010-June/msg00452.html

I do think qemu driver should support too, but I have to figure out how to
detect it from qemu first, and will be a later patch...

Will post v3 with your suggestion on the for loop. Thanks.

Osier

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list