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@xxxxxxxxxxxxxxx> 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 "/".
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list