[libvirt] [PATCH] storage pool discovery

[David Lively]

Thanks much for your comments, Jim.  They all look reasonable (though I
may be intentionally trimming a NL in one of these cases -- I'm
checking).

And I'll start following the HACKING file recommendations before
submitting my next attempt :-)

(Though note I'm not yet submitting tests since we haven't really
finished hashing out the API - at least some crucial XML details ...)

Dave



On Mon, 2008-08-04 at 08:25 +0200, Jim Meyering wrote:
> 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 "/".