[libvirt] [PATCH] storage pool discovery

[Jim Meyering]

Hi David,

I spotted a few things that would be good to change:

David Lively <dlively at virtualiron.com> wrote:
> diff --git a/configure.in b/configure.in
> index 8e04f14..5ef0384 100644
> --- a/configure.in
> +++ b/configure.in
> @@ -660,6 +660,10 @@ if test "$with_storage_fs" = "yes" -o "$with_storage_fs" = "check"; then
>    fi
>  fi
>  AM_CONDITIONAL([WITH_STORAGE_FS], [test "$with_storage_fs" = "yes"])
> +if test "$with_storage_fs" = "yes"; then
> +  AC_PATH_PROG([SHOWMOUNT], [showmount], [], [$PATH:/sbin:/usr/sbin])
> +  AC_DEFINE_UNQUOTED([SHOWMOUNT], ["$SHOWMOUNT"], [Location or name of the showmount program])

Please split long lines:

    AC_DEFINE_UNQUOTED([SHOWMOUNT], ["$SHOWMOUNT"],
      [Location or name of the showmount program])

...
> diff --git a/src/storage_backend_fs.c b/src/storage_backend_fs.c
...
> +static int
> +virStorageBackendFileSystemNetDiscoverPoolsFunc(virConnectPtr conn ATTRIBUTE_UNUSED,
> +						virStoragePoolObjPtr pool ATTRIBUTE_UNUSED,
> +						char **const groups,
> +						void *data)
> +{
> +    virNetfsDiscoverState *state = data;
> +    virStringList *newItem;
> +    const char *name, *path;
> +    int len;
> +
> +    path = groups[0];
> +
> +    name = rindex(path, '/');

rindex is deprecated.  Using it causes portability problems.
Use strrchr instead.

> +    if (name == NULL) {
> +	virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR, "%s", _("no / in path?"));

If you include the offending string in the diagnostic
and add a word of description, it'll be more useful:

  virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR,
                        _("invalid netfs path (no slash): %s"), path);

> +	return -1;
> +    }
> +    name += 1;
> +
> +    /* Append new XML desc to list */
> +
> +    if (VIR_ALLOC(newItem) != 0) {
> +        virStorageReportError(conn, VIR_ERR_NO_MEMORY, "%s", _("new xml desc"));
> +        return -1;
> +    }
> +
> +
> +    /* regexp pattern is too greedy, so we may have trailing spaces to trim.
> +     * NB showmount output ambiguous for (evil) exports with names w/trailing whitespace

Too long.

> +     */
> +    len = 0;
> +    while (name[len])
> +        len++;

This is equivalent to the three lines above:
       len = strlen (name);

> +    while (c_isspace(name[len-1]))
> +        len--;

It is customary to trim spaces and TABs (but less so CR, FF, NL),
so c_isblank might be better than c_isspace here.

...

> +static int
> +virStorageBackendFileSystemNetDiscoverPools(virConnectPtr conn,
...
> + cleanup:
> +    if (doc)
> +	xmlFreeDoc(doc);
> +    if (xpath_ctxt)

You can remove this "if" test.  It's unnecessary.
BTW, "make syntax-check" should spot this and fail because of it.

> +	xmlXPathFreeContext(xpath_ctxt);
> +    VIR_FREE(state.host);
> +    while (state.list) {
> +	p = state.list->next;
> +	VIR_FREE(state.list);
> +	state.list = p;
> +    }
> +
> +    return n_descs;
> +}
...
> diff --git a/src/storage_backend_logical.c b/src/storage_backend_logical.c
> index 9a0c27f..3c16d20 100644
> --- a/src/storage_backend_logical.c
> +++ b/src/storage_backend_logical.c
...
> @@ -257,6 +258,90 @@ virStorageBackendLogicalRefreshPoolFunc(virConnectPtr conn ATTRIBUTE_UNUSED,
>
>
>  static int
> +virStorageBackendLogicalDiscoverPoolsFunc(virConnectPtr conn ATTRIBUTE_UNUSED,
> +					  virStoragePoolObjPtr pool ATTRIBUTE_UNUSED,
> +					  size_t n_tokens,
> +					  char **const tokens,
> +					  void *data)
> +{
> +    virStringList **rest = data;
> +    virStringList *newItem;
> +    const char *name;
> +    int len;
> +
> +    /* Append new XML desc to list */
> +
> +    if (VIR_ALLOC(newItem) != 0) {
> +        virStorageReportError(conn, VIR_ERR_NO_MEMORY, "%s", _("new xml desc"));
> +        return -1;
> +    }
> +
> +    if (n_tokens != 1) {
> +	virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR, "%s", _("n_tokens should be 1"));
> +	return -1;
> +    }
> +
> +
> +    name = tokens[0];
> +    virSkipSpaces(&name);

Is is necessary to skip leading backslashes and carriage returns here?

> +    len = 0;
> +    while (name[len])
> +        len++;
> +    while (c_isspace(name[len-1]))
> +        len--;

A zero-length or all-"space" name would lead to referencing name[-1].
You can use this instead:

    while (len && c_isspace(name[len-1]))
        len--;

The similar code above isn't vulnerable like this
due to its guarantee that there is a "/".