12:18 p.m.
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(a)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 "/".