Hi David, I spotted a few things that would be good to change: David Lively <dlively@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 "/".