Re: [libvirt] [PATCH 3/4] conf: Use existing SCSI hostdev model to create new

On 12/06/2017 08:08 AM, John Ferlan wrote: > In virDomainDefMaybeAddHostdevSCSIcontroller when we add a new > controller because someone neglected to add one or we're adding > one because the existing one is full, we should copy over the > model number from the existing controller since whatever we > create should at least have the same characteristics as the one > we cannot use because it's full. > > NB: This affects the existing hostdev-scsi-autogen-address test > which would add a default ('lsi') SCSI controller for the various > scsi_host's that would create a controller for the hostdev. > > Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; > --- >   src/conf/domain_conf.c                                    | 13 > ++++++++++++- >   tests/qemuxml2xmloutdata/hostdev-scsi-autogen-address.xml |  2 +- >   2 files changed, 13 insertions(+), 2 deletions(-) > > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c > index 66e21c4bd..61b4a0075 100644 > --- a/src/conf/domain_conf.c > +++ b/src/conf/domain_conf.c > @@ -17689,12 +17689,22 @@ > virDomainDefMaybeAddHostdevSCSIcontroller(virDomainDefPtr def) >       size_t i; >       int maxController = -1; >       virDomainHostdevDefPtr hostdev; > +    virDomainControllerModelSCSI model = -1; > +    virDomainControllerModelSCSI newModel = -1; > >       for (i = 0; i < def->nhostdevs; i++) { >           hostdev = def->hostdevs[i]; >           if (virHostdevIsSCSIDevice(hostdev) && >               (int)hostdev->info->addr.drive.controller > > maxController) { >               maxController = hostdev->info->addr.drive.controller; > +            /* We may be creating a new controller because this one > is full. > +             * So let's grab the model from it and update the model > we're > +             * going to add as long as this one isn't undefined. The > premise > +             * being keeping the same controller model for all SCSI > hostdevs. */ > +            model = virDomainDeviceFindControllerModel(def, > hostdev->info, > +                                                       > VIR_DOMAIN_CONTROLLER_TYPE_SCSI); > +            if (model != -1) > +                newModel = model; What's the harm if the last controller were undefined?  Wouldn't it get populated down the road anyway if one were not set at this point (we send -1 today, after all).  That could eliminate one of the two virDomainControllerModelSCSI variables here, I would think.

But, either way I think it's fine. Reviewed-by: Eric Farman <farman(a)linux.vnet.ibm.com&gt; >           } >       } > > @@ -17702,7 +17712,8 @@ > virDomainDefMaybeAddHostdevSCSIcontroller(virDomainDefPtr def) >           return 0; > >       for (i = 0; i <= maxController; i++) { > -        if (virDomainDefMaybeAddController(def, > VIR_DOMAIN_CONTROLLER_TYPE_SCSI, i, -1) < 0) > +        if (virDomainDefMaybeAddController(def, > VIR_DOMAIN_CONTROLLER_TYPE_SCSI, > +                                           i, newModel) < 0) >               return -1; >       } > > diff --git a/tests/qemuxml2xmloutdata/hostdev-scsi-autogen-address.xml > b/tests/qemuxml2xmloutdata/hostdev-scsi-autogen-address.xml > index 8e93056ee..cea212b64 100644 > --- a/tests/qemuxml2xmloutdata/hostdev-scsi-autogen-address.xml > +++ b/tests/qemuxml2xmloutdata/hostdev-scsi-autogen-address.xml > @@ -29,7 +29,7 @@ >         <address type='pci' domain='0x0000' bus='0x00' slot='0x01' > function='0x1'/> >       </controller> >       <controller type='pci' index='0' model='pci-root'/> > -    <controller type='scsi' index='1'> > +    <controller type='scsi' index='1' model='virtio-scsi'> >         <address type='pci' domain='0x0000' bus='0x00' slot='0x04' > function='0x0'/> >       </controller> >       <input type='mouse' bus='ps2'/> >