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