[libvirt] [PATCH v4] storage: netfs and iscsi need option srcSpec for resource discovery

[Osier Yang]

On 2012年07月31日 16:56, Guannan Ren wrote:
> The option 'srcSpec' to virsh command find-storage-pool-sources
> is optional for logical type of storage pool, but mandatory for
> netfs and iscsi type.
> When missing the option for netfs and iscsi, libvirt reports XML
> parsing error due to null string option srcSpec.
>
> before
> error: Failed to find any netfs pool sources
> error: (storage_source_specification):1: Document is empty
> (null)
>
> after:
> error: pool type 'iscsi' requires option --srcSpec for source discovery
> ---
>   src/remote/remote_driver.c          |   14 +-------------
>   src/storage/storage_backend_fs.c    |   13 +++++++++----
>   src/storage/storage_backend_iscsi.c |    7 +++++++
>   tools/virsh-pool.c                  |    6 ++++++
>   4 files changed, 23 insertions(+), 17 deletions(-)
>
> diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c
> index 9ac27b2..9354cb4 100644
> --- a/src/remote/remote_driver.c
> +++ b/src/remote/remote_driver.c
> @@ -2648,23 +2648,11 @@ remoteFindStoragePoolSources (virConnectPtr conn,
>       remote_find_storage_pool_sources_args args;
>       remote_find_storage_pool_sources_ret ret;
>       struct private_data *priv = conn->storagePrivateData;
> -    const char *emptyString = "";
>
>       remoteDriverLock(priv);
>
>       args.type = (char*)type;
> -    /*
> -     * I'd think the following would work here:
> -     *    args.srcSpec = (char**)&srcSpec;
> -     * since srcSpec is a remote_string (not a remote_nonnull_string).
> -     *
> -     * But when srcSpec is NULL, this yields:
> -     *    libvir: Remote error : marshaling args
> -     *
> -     * So for now I'm working around this by turning NULL srcSpecs
> -     * into empty strings.
> -     */
> -    args.srcSpec = srcSpec ? (char **)&srcSpec : (char **)&emptyString;
> +    args.srcSpec = srcSpec ? (char **)&srcSpec : NULL;

This answer my question on v3: is it must be not NULL. And good fix,
I can't see any reason to pass a empty string instead of NULL here.

>       args.flags = flags;
>
>       memset (&ret, 0, sizeof(ret));
> diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c
> index 01b517a..92a3228 100644
> --- a/src/storage/storage_backend_fs.c
> +++ b/src/storage/storage_backend_fs.c
> @@ -258,10 +258,15 @@ virStorageBackendFileSystemNetFindPoolSources(virConnectPtr conn ATTRIBUTE_UNUSE
>
>       virCheckFlags(0, NULL);
>
> -    source = virStoragePoolDefParseSourceString(srcSpec,
> -                                                VIR_STORAGE_POOL_NETFS);
> -    if (!source)
> -        goto cleanup;
> +    if (!srcSpec) {
> +        virReportError(VIR_ERR_INVALID_ARG,
> +                       "%s", _("hostname must be specified for netfs sources"));
> +        return NULL;
> +    }

Yeah, backends are the better place to do the checking. Though it's a
bit confused between the error and "srcSpec" if someone reads the
codes, but it's an clear error for user.

> +
> +    if (!(source = virStoragePoolDefParseSourceString(srcSpec,
> +                                                      VIR_STORAGE_POOL_NETFS)))
> +        return NULL;
>
>       if (source->nhost != 1) {
>           virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> diff --git a/src/storage/storage_backend_iscsi.c b/src/storage/storage_backend_iscsi.c
> index b2f0c6c..75c2e52 100644
> --- a/src/storage/storage_backend_iscsi.c
> +++ b/src/storage/storage_backend_iscsi.c
> @@ -582,6 +582,13 @@ virStorageBackendISCSIFindPoolSources(virConnectPtr conn ATTRIBUTE_UNUSED,
>
>       virCheckFlags(0, NULL);
>
> +    if (!srcSpec) {
> +        virReportError(VIR_ERR_INVALID_ARG,
> +                       "%s", _("hostname and device path "
> +                               "must be specified for iscsi sources"));
> +        return NULL;
> +    }
> +
>       if (!(source = virStoragePoolDefParseSourceString(srcSpec,
>                                                         list.type)))
>           return NULL;
> diff --git a/tools/virsh-pool.c b/tools/virsh-pool.c
> index af80427..0f0b21e 100644
> --- a/tools/virsh-pool.c
> +++ b/tools/virsh-pool.c
> @@ -1093,6 +1093,12 @@ cmdPoolDiscoverSources(vshControl * ctl, const vshCmd * cmd ATTRIBUTE_UNUSED)
>       if (srcSpecFile&&  virFileReadAll(srcSpecFile, VIRSH_MAX_XML_FILE,&srcSpec)<  0)
>           return false;
>
> +    if (!srcSpec&&  (STREQ(type, "netfs") || STREQ(type, "iscsi"))) {

Everything looks fine, except here. You will get error like:

"pool type 'foobar' requires options ...."

If one input 'foobar' as the pool type.

My opinion is not checking it in virsh, just let it goes through
the APIs.

ACK with the virsh hacking removed.

Regards,
Osier