[libvirt] [PATCH] storage pool discovery

[Jim Meyering]

David Lively <dlively at virtualiron.com> wrote:
> Here's the patch with those issues addressed (also merged with latest
> upstream - avoids internal.h merge conflict) ...

Hi David,
Looks good.
A couple of minor suggestions and questions.

...
> diff --git a/configure.in b/configure.in
> index 9479f1c..430a097 100644
> --- a/configure.in
> +++ b/configure.in
> @@ -671,6 +671,11 @@ 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])
> +fi

Do we want to require the "showmount" package via the spec file?

...
> 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;

path can be const, too.

> +
> +    path = groups[0];
> +
> +    name = strrchr(path, '/');
> +    if (name == NULL) {
> +        virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR,
> +                              _("invalid netfs path (no slash): %s"), path);
> +        return -1;
> +    }
> +    name += 1;

It'd be good to diagnose a path that ends in a slash (*name == '\0'),
rather than emitting XML with an empty name, below.

> +    /* Append new XML desc to list */
> +
> +    if (VIR_ALLOC(newItem) != 0) {
> +        virStorageReportError(conn, VIR_ERR_NO_MEMORY, "%s", _("new xml desc"));
> +        return -1;
> +    }
> +
> +    if (asprintf(&newItem->val,
> +                 "<source><host name='%s'/><dir path='%s'/></source>\n",
> +                 state->host, path) <= 0) {
> +        virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR, "%s", _("asprintf failed"));
> +        VIR_FREE(newItem);
> +        return -1;
> +    }
> +
> +    newItem->len = strlen(newItem->val);
> +    newItem->next = state->list;
> +    state->list = newItem;
> +
> +    return 0;
> +}
> +
> +static char *
> +virStorageBackendFileSystemNetDiscoverPools(virConnectPtr conn,
> +                                            const char *srcSpec,
> +                                            unsigned int flags ATTRIBUTE_UNUSED)
> +{
> +    /*
> +     *  # showmount --no-headers -e HOSTNAME
> +     *  /tmp   *
> +     *  /A dir demo1.foo.bar,demo2.foo.bar
> +     *
> +     * Extract directory name (including possible interior spaces ...).
> +     */
> +
> +    const char *regexes[] = {
> +        "^(/.*\\S) +\\S+$"
> +    };
> +    int vars[] = {
> +        1
> +    };
> +    xmlDocPtr doc = NULL;
> +    xmlXPathContextPtr xpath_ctxt = NULL;
> +    virNetfsDiscoverState state = { .host = NULL, .list = NULL };
> +    const char *prog[] = { SHOWMOUNT, "--no-headers", "--exports", NULL, NULL };
> +    int exitstatus, len;

Please use "size_t" as the type for string-length
variables like "len".

> +    virStringList *p;
> +    char *retval = NULL;
> +
> +    doc = xmlReadDoc((const xmlChar *)srcSpec, "srcSpec.xml", NULL,
> +                     XML_PARSE_NOENT | XML_PARSE_NONET |
> +                     XML_PARSE_NOERROR | XML_PARSE_NOWARNING);
> +    if (doc == NULL) {
> +        virStorageReportError(conn, VIR_ERR_XML_ERROR, "%s", _("bad <source> spec"));
> +        goto cleanup;
> +    }
> +
> +    xpath_ctxt = xmlXPathNewContext(doc);
> +    if (xpath_ctxt == NULL) {
> +        virStorageReportError(conn, VIR_ERR_NO_MEMORY, "%s", _("xpath_ctxt"));
> +        goto cleanup;
> +    }
> +
> +    state.host = virXPathString(conn, "string(/source/host/@name)", xpath_ctxt);
> +    if (!state.host || !state.host[0]) {
> +        virStorageReportError(conn, VIR_ERR_XML_ERROR, "%s",
> +                              _("missing <host> in <source> spec"));
> +        goto cleanup;
> +    }
> +    prog[3] = state.host;
> +
> +    if (virStorageBackendRunProgRegex(conn, NULL, prog, 1, regexes, vars,
> +                                      virStorageBackendFileSystemNetDiscoverPoolsFunc,
> +                                      &state, &exitstatus) < 0)
> +        goto cleanup;
> +
> +    /* Turn list of strings into final document */
> +    len = strlen(SOURCES_START_TAG) + strlen(SOURCES_END_TAG);
> +    for (p = state.list; p; p = p->next)
> +        len += p->len;
> +    if (VIR_ALLOC_N(retval, len+1) < 0) {
> +        virStorageReportError(conn, VIR_ERR_NO_MEMORY, _("retval (%d bytes)"), len);
> +        goto cleanup;
> +    }
> +    strcpy(retval, SOURCES_START_TAG);
> +    len = strlen(SOURCES_START_TAG);
> +    for (p = state.list; p; p = p->next) {
> +        strcpy(retval + len, p->val);
> +        len += p->len;
> +    }
> +    strcpy(retval + len, SOURCES_END_TAG);
> +
> + cleanup:
> +    xmlFreeDoc(doc);
> +    xmlXPathFreeContext(xpath_ctxt);
> +    VIR_FREE(state.host);
> +    while (state.list) {
> +        p = state.list->next;
> +        VIR_FREE(state.list);
> +        state.list = p;
> +    }
> +
> +    return retval;
> +}
...
> diff --git a/src/storage_backend_logical.c b/src/storage_backend_logical.c
...
> +static char *
> +virStorageBackendLogicalDiscoverPools(virConnectPtr conn,
> +                                      const char *srcSpec ATTRIBUTE_UNUSED,
> +                                      unsigned int flags ATTRIBUTE_UNUSED)
> +{
> +    /*
> +     * # sudo vgs --noheadings -o vg_name
> +     *   VolGroup00
> +     *   VolGroup01
> +     */
> +    const char *regexes[] = {
> +        "^\\s*(\\S+)\\s*$"
> +    };
> +    int vars[] = {

This can be const.

> +        1
> +    };
> +    virStringList *descs = NULL;
> +    const char *prog[] = { VGS, "--noheadings", "-o", "vg_name", NULL };
> +    int exitstatus, len;

len should be size_t, here too.

> +    virStringList *p;
> +    char *retval = NULL;
> +
> +    if (virStorageBackendRunProgRegex(conn, NULL, prog, 1, regexes, vars,
> +                                      virStorageBackendLogicalDiscoverPoolsFunc,
> +                                      &descs, &exitstatus) < 0)
> +        return NULL;
> +

The following block of 15 lines is identical to the one above.
It'd be nice to factor out the duplication.

Actually, this
virStorageBackendLogicalDiscoverPools and the preceding
virStorageBackendFileSystemNetDiscoverPools share enough
code that I wonder if it'd might be worthwhile to unify them.
Probably not, but it's close...

> +    /* Turn list of strings into final document */
> +    len = strlen(SOURCES_START_TAG) + strlen(SOURCES_END_TAG);
> +    for (p = descs; p; p = p->next)
> +        len += p->len;
> +    if (VIR_ALLOC_N(retval, len+1) < 0) {
> +        virStorageReportError(conn, VIR_ERR_NO_MEMORY, _("retval (%d bytes)"), len);
> +        goto cleanup;
> +    }
> +    strcpy(retval, SOURCES_START_TAG);
> +    len = strlen(SOURCES_START_TAG);
> +    for (p = descs; p; p = p->next) {
> +        strcpy(retval + len, p->val);
> +        len += p->len;
> +    }
> +    strcpy(retval + len, SOURCES_END_TAG);
> +
> + cleanup:
> +    while (descs) {
> +        p = descs->next;
> +        VIR_FREE(descs);
> +        descs = p;
> +    }
> +
> +    return retval;
> +}
...
> diff --git a/src/virsh.c b/src/virsh.c
> index 67d9658..08fb425 100644
> --- a/src/virsh.c
> +++ b/src/virsh.c
> @@ -3422,6 +3422,138 @@ cmdPoolList(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED)
>      return TRUE;
>  }
>
> +/*
> + * "pool-discover-as" command
> + */
> +static vshCmdInfo info_pool_discover_as[] = {

I think all similar static array declarations are "const".
At least they were supposed to be.
We probably need an automatic check for this.

    static const vshCmdInfo info_pool_discover_as[] = {

> +    {"syntax", "pool-discover-as <type> [options]"},
> +    {"help", gettext_noop("discover pools")},
> +    {"desc", gettext_noop("Returns discover of pools.")},
> +    {NULL, NULL}
> +};
> +
> +static vshCmdOptDef opts_pool_discover_as[] = {
> +    {"type", VSH_OT_DATA, VSH_OFLAG_REQ, gettext_noop("type of storage pool to discover")},
> +    {"host", VSH_OT_DATA, VSH_OFLAG_NONE, gettext_noop("optional host to query")},
> +    {"port", VSH_OT_DATA, VSH_OFLAG_NONE, gettext_noop("optional port to query")},
> +    {NULL, 0, 0, NULL}
> +};
> +
> +static int
> +cmdPoolDiscoverAs(vshControl * ctl, const vshCmd * cmd ATTRIBUTE_UNUSED)
> +{
> +    char *type, *host;
> +    char *srcSpec = NULL;
> +    char *srcList;
> +    int found;
> +
> +    type = vshCommandOptString(cmd, "type", &found);
> +    if (!found)
> +        return FALSE;
> +    host = vshCommandOptString(cmd, "host", &found);
> +    if (!found)
> +        host = NULL;
> +
> +    if (!vshConnectionUsability(ctl, ctl->conn, TRUE))
> +        return FALSE;
> +
> +    if (host) {
> +        int hostlen = strlen(host);

use size_t

> +        char *port = vshCommandOptString(cmd, "port", &found);
> +        int ret;
> +        if (!found) {
> +            port = strrchr(host, ':');
> +            if (port) {
> +                if (*(++port))
> +                    hostlen = (int)(port - host - 1);

no need for the cast to "int"

> +                else
> +                    port = NULL;
> +            }
> +        }
> +        ret = port ?
> +            asprintf(&srcSpec,
> +                     "<source><host name='%.*s' port='%s'/></source>",
> +                     hostlen, host, port) :

Do the hostname and port string need to be quoted (and/or evoke
warning/failure), in case they contain things like "'"?

> +            asprintf(&srcSpec,
> +                     "<source><host name='%.*s'/></source>",
> +                     hostlen, host);
> +        if (ret < 0) {
> +            switch (errno) {
> +            case ENOMEM:
> +                vshError(ctl, FALSE, "%s", _("Out of memory"));
> +                break;
> +            default:
> +                vshError(ctl, FALSE, _("asprintf failed (errno %d)"), errno);
> +            }
> +            return FALSE;
> +        }
> +    }
> +
> +    srcList = virConnectDiscoverStoragePools(ctl->conn, type, srcSpec, 0);
> +    free(srcSpec);
> +    if (srcList == NULL) {
> +        vshError(ctl, FALSE, "%s", _("Failed to discover pools"));
> +        return FALSE;
> +    }
> +    vshPrint(ctl, "%s", srcList);
> +    free(srcList);
> +
> +    return TRUE;
> +}
> +
> +
> +/*
> + * "pool-discover" command
> + */
> +static vshCmdInfo info_pool_discover[] = {

If these can be const, please make them so (along with any above):

  static const vshCmdInfo info_pool_discover[] = {

> +    {"syntax", "pool-discover <type> [srcSpec]"},
> +    {"help", gettext_noop("discover pools")},
> +    {"desc", gettext_noop("Returns discover of pools.")},
> +    {NULL, NULL}
> +};
> +
> +static vshCmdOptDef opts_pool_discover[] = {
> +    {"type", VSH_OT_DATA, VSH_OFLAG_REQ,
> +     gettext_noop("type of storage pool to discover")},
> +    {"srcSpec", VSH_OT_DATA, VSH_OFLAG_NONE,
> +     gettext_noop("optional file of source xml to query for pools")},
> +    {NULL, 0, 0, NULL}
> +};
...