[libvirt] [PATCH 2/2] conf: Validate disk lun using correct types

[Laine Stump]

On 06/26/2015 02:15 PM, John Ferlan wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1201143
>
> The formatdomain.html.in description for <disk> device 'lun' indicates that

you could just say formatdomain.html...

> it must be either a type 'block' or type 'network' with protocol 'iscsi';
> however, we did not make that check until domain startup.
>
> This caused issues for virt-manager which had an unexpected failure at
> run time rather config time.
>
> This patch adds a check in post part disk device checking for the specific
> and supported lun types as well as adjusting the test failure to be for
> parse config rather than run time.
>
> Signed-off-by: John Ferlan <jferlan at redhat.com>
> ---
>  src/conf/domain_conf.c   | 22 ++++++++++++++++++++++
>  tests/qemuxml2argvtest.c |  2 +-
>  2 files changed, 23 insertions(+), 1 deletion(-)
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 2c3b96b..211aa9f 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -3893,6 +3893,28 @@ virDomainDeviceDefPostParseInternal(virDomainDeviceDefPtr dev,
>                  return -1;
>              }
>          }
> +
> +        /* Validate LUN configuration
> +         * NOTE: virStorageTranslateDiskSourcePool is not run yet, so for
> +         *       disk "volume"'s, the closest we can get at config time is
> +         *       to ensure mode isn't direct since host/default will allow
> +         *       lun/block usage. At run time if it's determined the wrong
> +         *       voltype and pooltype values are set, then failure occurs
> +         */
> +        if (disk->device == VIR_DOMAIN_DISK_DEVICE_LUN &&
> +            !(disk->src->type == VIR_STORAGE_TYPE_BLOCK ||
> +              (disk->src->type == VIR_STORAGE_TYPE_NETWORK &&
> +               disk->src->protocol == VIR_STORAGE_NET_PROTOCOL_ISCSI) ||
> +              (disk->src->type == VIR_STORAGE_TYPE_VOLUME &&
> +               disk->src->srcpool &&
> +               disk->src->srcpool->mode !=
> +               VIR_STORAGE_SOURCE_POOL_MODE_DIRECT))) {
> +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +                           _("disk '%s' improperly configured to be a "
> +                             "device='lun'"),

maybe "improperly configured for ..."? Either way (or any other way).

Under the assumption that you understand the reasoning behind the logic
much better than I do, ACK.