Re: [libvirt] [PATCH 13/25] conf: Generate address for scsi host device automatically

On 05/03/2013 02:07 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 algrithom to honor "bus" s/algrithom/algorithm > and "target" too, it's another story. The address generator s/it's another story/that's a different issue > 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 created implicitly by someone > else), and sits on $new_controller:0:0:0 instead. Implicit creation > --- > Since it needs to add the controllers for "drive" type address > implicitly, I add the generator in domain_conf.c instead of > the specific the driver, e.g. qemu. > --- > src/conf/domain_conf.c | 144 +++++++++++++++++++++++++++++++++++++++++++++++-- > 1 file changed, 139 insertions(+), 5 deletions(-) > > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c > index 31a8c46..cff2b46 100644 > --- a/src/conf/domain_conf.c > +++ b/src/conf/domain_conf.c > @@ -8658,8 +8658,140 @@ error: > return NULL; > } > > +/* 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; > +} > + > +/* 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++) { > + /* The controller itself is on unit 7 */ > + if (max_unit == 16 && i == 7) I would expect 16 and 7 to be constants

The 'max_unit' check is irrelevant if max_unit=7, then we're not getting here anyway...

> + continue; > + > + if (!virDomainDriveAddressIsUsedByDisk(def, > + VIR_DOMAIN_DISK_BUS_SCSI, controller, i) && > + !virDomainDriveAddressIsUsedByHostdev(def, > + VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI, controller, i)) > + return i; > + } > + > + return -1; > +} > + > +static int > +virDomainHostdevAssignAddress(virDomainXMLOptionPtr xmlopt, > + virDomainDefPtr def, > + virDomainHostdevDefPtr hostdev) This ends up being Scsi specific right? E.G. AssignSCSIAddress instead of generic AssignAddress (or wherever the "best" place is to put SCSI)

> +{ > + unsigned int max_unit; > + int next_unit; > + unsigned nscsi_controllers; s/;/ = 0; > + bool found; > + int i; > + > + if (hostdev->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI) > + return -1; > + > + /* See comments in virDomainDiskDefAssignAddress */ > + if (xmlopt->config.hasWideScsiBus) > + max_unit = 16; > + else > + max_unit = 7; Guess I would have figured these would be constants somewhere... Not that I expect the values to change... Units are numbered 0 -> 15 and 0 -> 6 then?

> + > + for (i = 0; i < def->ncontrollers; i++) { > + if (def->controllers[i]->type != VIR_DOMAIN_CONTROLLER_TYPE_SCSI) > + continue; > + > + nscsi_controllers++; If we're "implicitly" creating a new controller, then is it's number based on the total number of controllers or the next scsi controller?

> + next_unit = virDomainControllerSCSINextUnit(def, max_unit, > + def->controllers[i]->idx); > + if (next_unit >= 0) { > + found = true; > + break; > + } > + } > + > + 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 + 1; To be fair, I'm not familiar (yet) with how things are laid out, but it would seem to me that this might not be returning what you want/expect. I'm missing a nuance here - the 'controller' field is generally based off the def->controllers[i]->idx value, except when we're "implicitly creating" one. In this case it's just a 'random' value based on some count of existing nscsi_controllers. If there are zero existing controllers, then we start at 1

If there is one existing/full controller, then we start at 2

How does the 'idx' value get populated? Is it possible that this setting conflicts with a different and non scsi controller?

Is there a max controller number we shouldn't go past?