David Lively <dlively@virtualiron.com> wrote:
I've attached a (very) small incremental patch (i.e., to be applied after the one you've already merged) that addresses a couple things I noticed missing: (a) documents the new <source> <name> element in formatstorage.html.in (b) adds --source-name to the (optional) args for virsh pool-define-as
I've also attached a new version of the full patch containing this change, in case that's easier.
Hi Dave, That looks fine. The only nit I saw was in the context:
diff --git a/src/storage_conf.c b/src/storage_conf.c ... { + ret->name = virXPathString(conn, "string(/pool/name)", ctxt); + if (ret->name == NULL && + options->flags & VIR_STORAGE_BACKEND_POOL_SOURCE_NAME) + ret->name = virXPathString(conn, "string(/pool/source/name)", ctxt); + if (ret->name == NULL) { virStorageReportError(conn, VIR_ERR_XML_ERROR, "%s", _("missing name element"));
There, it'd be clearer to diagnose with something like "missing pool source device name", since there are a few other "name" elements. Not a show-stopper at all, but it'd be great if you could add a unit test or two, so this new code gets some regular coverage.