[libvirt] [PATCH 2/8] lxc: Fix wrong error message on disk hotplug

[John Ferlan]

On 05/02/2016 10:32 AM, Peter Krempa wrote:
> Commit 36025c552 tried to improve error reporting for <disk type="lun">
> but reused the code in LXC which doesn't care about the actual disk
> type. The error messages would then contain a bogous hint that the
> config for the 'lun' device is invalid which might not be the case.
> 
> Re-do the relevant portion of the commit with the original message.
> ---
>  src/lxc/lxc_driver.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c
> index 1dfbde3..7b76daf 100644
> --- a/src/lxc/lxc_driver.c
> +++ b/src/lxc/lxc_driver.c
> @@ -4106,9 +4106,6 @@ lxcDomainAttachDeviceDiskLive(virLXCDriverPtr driver,
>          goto cleanup;
>      }
> 
> -    if (!virDomainDiskSourceIsBlockType(def->src, true))
> -        goto cleanup;
> -
>      src = virDomainDiskGetSource(def);
>      if (src == NULL) {
>          virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> @@ -4116,6 +4113,12 @@ lxcDomainAttachDeviceDiskLive(virLXCDriverPtr driver,
>          goto cleanup;
>      }
> 
> +    if (virStorageSourceIsBlockLocal(def->src)) {

Shouldn't this be "if (!virStorageSourceIsBlockLocal(def->src)" ?

ACK w/ the adjustment

John

> +        virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +                       _("Can't setup disk for non-block device"));
> +        goto cleanup;
> +    }
> +

FWIW:
Prior to my commit referenced above the check was:

    if (!virDomainDiskSourceIsBlockType(def->src)) {
        virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
                       _("Can't setup disk for non-block device"));

And yes, of course since virDomainDiskSourceIsBlockType checks
"src->path" first (and provides a different message), moving this after
that check seems right; otherwise, the error message is "source path not
found for device='lun' using type='%d'" if (!src->path).

All that commit did was try to have the lower layer generate the error
message. Of course in this case erroneously swapping "Can't setup disk
for non-block device" for "disk device='lun' is only valid for block
type disk source".

Still going back to 'a7785ccf' we see the call to "if
(!virDomainDiskSourceIsBlockType(def)"...


>      if (virDomainDiskIndexByName(vm->def, def->dst, true) >= 0) {
>          virReportError(VIR_ERR_OPERATION_FAILED,
>                         _("target %s already exists"), def->dst);
>