[libvirt] [PATCH 2/4] qemu: Use same model when adding hostdev SCSI controller

[Eric Farman]

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 at redhat.com>
> ---
>   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;
>       }
>