[libvirt] [PATCH 1/3] qemu: hot-plug: Fix broken SCSI disk hot-plug

[Peter Krempa]

On Mon, Jun 27, 2016 at 16:43:46 +0200, Marc Hartmayer wrote:
> The commit "qemu: hot-plug: Assume support for -device in
> qemuDomainAttachSCSIDisk" dropped the code for the automatic SCSI
> controller creation used in SCSI disk hot-plugging. If we are
> hot-plugging a SCSI disk to a domain and there is no proper SCSI
> controller defined, it results in an "error: internal error: Could not
> find scsi controller with index X required for device" error.
> 
> For that reason reverting a hunk of the commit
> d4d32005d6e8b2cc0a2f26b483ca1de10171db6d.

Uh, you are right. I missed that
qemuDomainFindOrCreateSCSIDiskController has a side effect of creating
the controller in this case when getting rid of the old code.

> 
> This patch also adds an extra comment to the code to clarify the
> loop.
> 
> Reviewed-by: Boris Fiuczynski <fiuczy at linux.vnet.ibm.com>
> Reviewed-by: Bjoern Walk <bwalk at linux.vnet.ibm.com>
> Signed-off-by: Marc Hartmayer <mhartmay at linux.vnet.ibm.com>
> ---
>  src/qemu/qemu_hotplug.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index e0b8230..5e6a8cb 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -544,7 +544,9 @@ qemuDomainAttachSCSIDisk(virConnectPtr conn,
>                           virDomainObjPtr vm,
>                           virDomainDiskDefPtr disk)
>  {
> +    size_t i;
>      qemuDomainObjPrivatePtr priv = vm->privateData;
> +    virDomainControllerDefPtr cont = NULL;

This variable isn't necessary.

>      char *drivestr = NULL;
>      char *devstr = NULL;
>      int ret = -1;
> @@ -561,6 +563,24 @@ qemuDomainAttachSCSIDisk(virConnectPtr conn,
>          goto error;
>      }
>  
> +    /* Let's make sure the disk has a controller defined and loaded before
> +     * trying to add it. The controller used by the disk must exist before a
> +     * qemu command line string is generated.
> +     *
> +     * Ensure that the given controller and all controllers with a smaller index
> +     * exist; there must not be any missing index in between.
> +     */
> +    for (i = 0; i <= disk->info.addr.drive.controller; i++) {
> +        cont = qemuDomainFindOrCreateSCSIDiskController(driver, vm, i);
> +        if (!cont)
> +            goto error;

This can be checked right away as the value of 'cont' isn't used.

> +    }
> +
> +    /* Tell clang that "cont" is non-NULL.
> +       This is because disk->info.addr.driver.controller is unsigned,
> +       and hence the above loop must iterate at least once. */
> +    sa_assert(cont);

Also this whole comment and assert can be dropped.

> +
>      if (qemuAssignDeviceDiskAlias(vm->def, disk, priv->qemuCaps) < 0)
>          goto error;

I'll adjust the patch and push it shortly. Thanks for fixing up the mess
I've made.

Peter