Commit id 'c5c96545' neglected to validate that the srcPriv was non-NULL before dereferencing. Similar problem to what was fixed by commit id '8056721c' but missed during multiple rebases and code reworks. Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt;

--- src/qemu/qemu_hotplug.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 6ef28bf05..9317e134a 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -2286,7 +2286,7 @@ qemuDomainAttachHostSCSIDevice(virConnectPtr conn, virDomainHostdevSubsysSCSIPtr scsisrc = &hostdev->source.subsys.u.scsi; virDomainHostdevSubsysSCSIiSCSIPtr iscsisrc = &scsisrc->u.iscsi; qemuDomainStorageSourcePrivatePtr srcPriv; - qemuDomainSecretInfoPtr secinfo; + qemuDomainSecretInfoPtr secinfo = NULL; if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE_SCSI_GENERIC)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", @@ -2328,7 +2328,8 @@ qemuDomainAttachHostSCSIDevice(virConnectPtr conn, goto cleanup; srcPriv = QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(iscsisrc->src); - secinfo = srcPriv->secinfo; + if (srcPriv) + secinfo = srcPriv->secinfo; if (secinfo && secinfo->type == VIR_DOMAIN_SECRET_INFO_TYPE_AES) { if (qemuBuildSecretInfoProps(secinfo, &secobjProps) < 0) goto cleanup;

On 12/06/2017 08:08 AM, John Ferlan wrote: > When qemuDomainFindOrCreateSCSIDiskController adds a controller, > let's use the same model as a currently found controller under the > assumption that the reason to add the controller in hotplug is > because virDomainHostdevAssignAddress determined that there were > too many devices on the existing controller, but only assigned a > new controller index and did not add a new controller and we > desire to use the same controller model as any existing conroller s/conroller/controller > and not take a chance that qemuDomainSetSCSIControllerModel would > use a default that may be incompatible. > > Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; > --- >   src/qemu/qemu_hotplug.c | 14 ++++++++++---- >   1 file changed, 10 insertions(+), 4 deletions(-) > > diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c > index 9317e134a..90d50e7b1 100644 > --- a/src/qemu/qemu_hotplug.c > +++ b/src/qemu/qemu_hotplug.c > @@ -587,6 +587,7 @@ > qemuDomainFindOrCreateSCSIDiskController(virQEMUDriverPtr driver, >   { >       size_t i; >       virDomainControllerDefPtr cont; > +    virDomainControllerModelSCSI model = -1; > >       for (i = 0; i < vm->def->ncontrollers; i++) { >           cont = vm->def->controllers[i]; > @@ -596,6 +597,12 @@ > qemuDomainFindOrCreateSCSIDiskController(virQEMUDriverPtr driver, > >           if (cont->idx == controller) >               return cont; > + > +        /* Save off the model - if we end up creating a controller it's > +         * because the user didn't provide one and we need to > automagically > +         * create one because the existing one is full - so let's be > sure > +         * to keep the same model in that case. */ > +        model = cont->model; Hrm...  I'm missing something... This is confusing, because nothing in this loop tells us the controller we find is full, just that we have a TYPE_SCSI controller and the index isn't found.  And if the index WAS found, we would've exited before this line anyway so we don't know if it was full or not.

Nothing in a <hostdev> tag says what type of controller we want, so what happens when controller[1] is virtio-scsi, and controller[2] is LSI? This will create a second LSI controller which wouldn't help if we're hotplugging another virtio-scsi device and controller[1] was full.  (Is mixing SCSI controller types common?  I don't know, I stay in a little virtio-scsi playpen.)

And I'm still trying to figure out how qemuDomainSetSCSIControllerModel plays into this and other codepaths.

Sorry, just sending my questions because this code is more fresh in your mind than my own.

>       } > >       /* No SCSI controller present, for backward compatibility we > @@ -604,11 +611,10 @@ > qemuDomainFindOrCreateSCSIDiskController(virQEMUDriverPtr driver, >           return NULL; >       cont->type = VIR_DOMAIN_CONTROLLER_TYPE_SCSI; >       cont->idx = controller; > -    cont->model = -1; > +    cont->model = model; > > -    VIR_INFO("No SCSI controller present, hotplugging one"); > -    if (qemuDomainAttachControllerDevice(driver, > -                                         vm, cont) < 0) { > +    VIR_INFO("No SCSI controller present, hotplugging one model=%d", > model); Seems like a good use for virDomainControllerModelSCSITypeToString(model) perhaps?

> +    if (qemuDomainAttachControllerDevice(driver, vm, cont) < 0) { >           VIR_FREE(cont); >           return NULL; >       } >

When qemuDomainFindOrCreateSCSIDiskController adds a controller, let's use the same model as a currently found controller under the assumption that the reason to add the controller in hotplug is because virDomainHostdevAssignAddress determined that there were too many devices on the existing controller, but only assigned a new controller index and did not add a new controller and we desire to use the same controller model as any existing conroller

and not take a chance that qemuDomainSetSCSIControllerModel would use a default that may be incompatible. Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/qemu/qemu_hotplug.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 9317e134a..90d50e7b1 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c

@@ -596,6 +597,12 @@ qemuDomainFindOrCreateSCSIDiskController(virQEMUDriverPtr driver, if (cont->idx == controller) return cont; + + /* Save off the model - if we end up creating a controller it's

+ * because the user didn't provide one and we need to automagically + * create one because the existing one is full - so let's be sure + * to keep the same model in that case. */ + model = cont->model; } /* No SCSI controller present, for backward compatibility we

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'/> >

On Wed, Dec 06, 2017 at 08:08:05AM -0500, 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. > Yet the change adds 'virtio-scsi'. >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; This declaration can be moved inside the if to reduce its scope. (I'm with Andrea on this [0])

>+ 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; > } > } > ACK Jan [0] https://www.redhat.com/archives/libvir-list/2017-December/msg00563.html

-- libvir-list mailing list libvir-list(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list

https://bugzilla.redhat.com/show_bug.cgi?id=1519130

Commit id 'dc692438' reverted the automagic addition of a SCSI controller attempt during virDomainHostdevAssignAddress; however, the logic to determine where to place the next_unit depended upon the "new" controller being added. Without the new controller the the next time through the call for the next SCSI hostdev found would result in the "next_unit" never changing from 0 (zero) and as a result the addition of the device will fail due to being a duplicate unit number of the first with the error message: virDomainDefCheckDuplicateDriveAddresses:$line : unsupported configuration: SCSI host address controller='0' bus='1' target='0' unit='0' in use by another SCSI host device So instead of walking the controller list looking for SCSI controllers, all we can do is "pretend" that they exist and allow other code to create them later as necessary. Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/conf/domain_conf.c | 31 +++++++++++++------------------ 1 file changed, 13 insertions(+), 18 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 61b4a0075..73c6708cf 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -4322,10 +4322,8 @@ virDomainHostdevAssignAddress(virDomainXMLOptionPtr xmlopt, const virDomainDef *def, virDomainHostdevDefPtr hostdev) { - int next_unit = 0; - unsigned controller = 0; + int controller = 0; unsigned int max_unit; - size_t i; int ret; if (xmlopt->config.features & VIR_DOMAIN_DEF_FEATURE_WIDE_SCSI) @@ -4333,33 +4331,30 @@ virDomainHostdevAssignAddress(virDomainXMLOptionPtr xmlopt, else max_unit = SCSI_NARROW_BUS_MAX_CONT_UNIT; - for (i = 0; i < def->ncontrollers; i++) { - if (def->controllers[i]->type != VIR_DOMAIN_CONTROLLER_TYPE_SCSI) - continue;

- - controller++; - ret = virDomainControllerSCSINextUnit(def, max_unit, - def->controllers[i]->idx); - if (ret >= 0) { - next_unit = ret; - controller = def->controllers[i]->idx; - break; - } - } - /* NB: Do not attempt calling virDomainDefMaybeAddController to * automagically add a "new" controller. Doing so will result in * qemuDomainFindOrCreateSCSIDiskController "finding" the controller * in the domain def list and thus not hotplugging the controller as * well as the hostdev in the event that there are either no SCSI * controllers defined or there was no space on an existing one. + * + * Because we cannot add a controller, then we should not walk the + * defined controllers list in order to find empty space.

Doing + * so fails to return the valid next unit number for the 2nd + * hostdev being added to the as yet to be created controller. */ + do { + ret = virDomainControllerSCSINextUnit(def, max_unit, controller); + if (ret < 0) + controller++; + } while (ret < 0); +

hostdev->info->type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE; hostdev->info->addr.drive.controller = controller; hostdev->info->addr.drive.bus = 0; hostdev->info->addr.drive.target = 0; - hostdev->info->addr.drive.unit = next_unit; + hostdev->info->addr.drive.unit = ret; return 0; }

https://bugzilla.redhat.com/show_bug.cgi?id=1519130 Commit id 'dc692438' reverted the automagic addition of a SCSI controller attempt during virDomainHostdevAssignAddress; however, the logic to determine where to place the next_unit depended upon the "new" controller being added. Without the new controller the the next time through the call for the next SCSI hostdev found would result in the "next_unit" never changing from 0 (zero) and as a result the addition of the device will fail due to being a duplicate unit number of the first with the error message: virDomainDefCheckDuplicateDriveAddresses:$line : unsupported configuration: SCSI host address controller='0' bus='1' target='0' unit='0' in use by another SCSI host device So instead of walking the controller list looking for SCSI controllers, all we can do is "pretend" that they exist and allow other code to create them later as necessary. Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/conf/domain_conf.c | 31 +++++++++++++------------------ 1 file changed, 13 insertions(+), 18 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 61b4a0075..73c6708cf 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -4322,10 +4322,8 @@ virDomainHostdevAssignAddress(virDomainXMLOptionPtr xmlopt, const virDomainDef *def, virDomainHostdevDefPtr hostdev) { - int next_unit = 0;

- unsigned controller = 0; + int controller = 0; unsigned int max_unit; - size_t i; int ret; if (xmlopt->config.features & VIR_DOMAIN_DEF_FEATURE_WIDE_SCSI) @@ -4333,33 +4331,30 @@ virDomainHostdevAssignAddress(virDomainXMLOptionPtr xmlopt, else max_unit = SCSI_NARROW_BUS_MAX_CONT_UNIT; - for (i = 0; i < def->ncontrollers; i++) { - if (def->controllers[i]->type != VIR_DOMAIN_CONTROLLER_TYPE_SCSI) - continue; - - controller++; - ret = virDomainControllerSCSINextUnit(def, max_unit, - def->controllers[i]->idx); - if (ret >= 0) { - next_unit = ret; - controller = def->controllers[i]->idx; - break; - } - } - /* NB: Do not attempt calling virDomainDefMaybeAddController to * automagically add a "new" controller. Doing so will result in * qemuDomainFindOrCreateSCSIDiskController "finding" the controller * in the domain def list and thus not hotplugging the controller as * well as the hostdev in the event that there are either no SCSI * controllers defined or there was no space on an existing one. + * + * Because we cannot add a controller, then we should not walk the + * defined controllers list in order to find empty space. Doing + * so fails to return the valid next unit number for the 2nd + * hostdev being added to the as yet to be created controller. */ + do { + ret = virDomainControllerSCSINextUnit(def, max_unit, controller); + if (ret < 0) + controller++; + } while (ret < 0); +

hostdev->info->type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE; hostdev->info->addr.drive.controller = controller; hostdev->info->addr.drive.bus = 0; hostdev->info->addr.drive.target = 0; - hostdev->info->addr.drive.unit = next_unit; + hostdev->info->addr.drive.unit = ret; return 0; } -- 2.13.6 -- libvir-list mailing list libvir-list(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list

On 12/20/2017 07:38 AM, Ján Tomko wrote: > On Wed, Dec 06, 2017 at 08:08:06AM -0500, John Ferlan wrote: >> https://bugzilla.redhat.com/show_bug.cgi?id=1519130 >> >> Commit id 'dc692438' reverted the automagic addition of a SCSI >> controller attempt during virDomainHostdevAssignAddress; however, >> the logic to determine where to place the next_unit depended upon >> the "new" controller being added.  Without the new controller the >> the next time through the call for the next SCSI hostdev found >> would result in the "next_unit" never changing from 0 (zero) and >> as a result the addition of the device will fail due to being a >> duplicate unit number of the first with the error message: >> >>  virDomainDefCheckDuplicateDriveAddresses:$line : unsupported >>      configuration: SCSI host address controller='0' bus='1' >>      target='0' unit='0' in use by another SCSI host device >> >> So instead of walking the controller list looking for SCSI >> controllers, all we can do is "pretend" that they exist and >> allow other code to create them later as necessary. >> >> Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; >> --- >> src/conf/domain_conf.c | 31 +++++++++++++------------------ >> 1 file changed, 13 insertions(+), 18 deletions(-) >> >> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c >> index 61b4a0075..73c6708cf 100644 >> --- a/src/conf/domain_conf.c >> +++ b/src/conf/domain_conf.c >> @@ -4322,10 +4322,8 @@ >> virDomainHostdevAssignAddress(virDomainXMLOptionPtr xmlopt, >>                               const virDomainDef *def, >>                               virDomainHostdevDefPtr hostdev) >> { >> -    int next_unit = 0; > > Please keep the descriptive 'next_unit' variable name. > >> -    unsigned controller = 0; >> +    int controller = 0; >>     unsigned int max_unit; >> -    size_t i; >>     int ret; >> >>     if (xmlopt->config.features & VIR_DOMAIN_DEF_FEATURE_WIDE_SCSI) >> @@ -4333,33 +4331,30 @@ >> virDomainHostdevAssignAddress(virDomainXMLOptionPtr xmlopt, >>     else >>         max_unit = SCSI_NARROW_BUS_MAX_CONT_UNIT; >> >> -    for (i = 0; i < def->ncontrollers; i++) { >> -        if (def->controllers[i]->type != >> VIR_DOMAIN_CONTROLLER_TYPE_SCSI) >> -            continue; >> - >> -        controller++; >> -        ret = virDomainControllerSCSINextUnit(def, max_unit, >> -                                              def->controllers[i]->idx); >> -        if (ret >= 0) { >> -            next_unit = ret; >> -            controller = def->controllers[i]->idx; >> -            break; >> -        } >> -    } >> - >>     /* NB: Do not attempt calling virDomainDefMaybeAddController to >>      * automagically add a "new" controller. Doing so will result in >>      * qemuDomainFindOrCreateSCSIDiskController "finding" the controller >>      * in the domain def list and thus not hotplugging the controller as >>      * well as the hostdev in the event that there are either no SCSI >>      * controllers defined or there was no space on an existing one. >> +     * >> +     * Because we cannot add a controller, then we should not walk the >> +     * defined controllers list in order to find empty space. Doing >> +     * so fails to return the valid next unit number for the 2nd >> +     * hostdev being added to the as yet to be created controller. >>      */ >> +    do { >> +        ret = virDomainControllerSCSINextUnit(def, max_unit, >> controller); >> +        if (ret < 0) >> +            controller++; >> +    } while (ret < 0); >> + > > Easier to read as: > for (next_unit = -1; next_unit < -1; controller++) >    next_unit = virDomainControllerSCSINextUnit(def, max_unit, controller); > Not functionally the same comparisons... Caused hostdev-scsi-autogen-address to fail... There's 11 hostdevs and only 1 controller. We never get controller==1 from that for loop. I can change @ret above to be @next_unit though

John > ACK > > Jan >> >>     hostdev->info->type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE; >>     hostdev->info->addr.drive.controller = controller; >>     hostdev->info->addr.drive.bus = 0; >>     hostdev->info->addr.drive.target = 0; >> -    hostdev->info->addr.drive.unit = next_unit; >> +    hostdev->info->addr.drive.unit = ret; >> >>     return 0; >> } >> --  >> 2.13.6 >> >> -- >> libvir-list mailing list >> libvir-list(a)redhat.com >> https://www.redhat.com/mailman/listinfo/libvir-list -- libvir-list mailing list libvir-list(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list