Re: [libvirt] [PATCH] storage pool discovery
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 "/".