Re: [libvirt] [PATCH] introducing <source> <name> (for logical storage pools)

[Jim Meyering <jim meyering net>]

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.