[libvirt] [PATCH v2 3/3] virsh: Implement domblkerror command

[Eric Blake]

On 01/30/2012 09:01 AM, Jiri Denemark wrote:
> This command lists all disk devices with errors
> ---
>  tools/virsh.c   |   89 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  tools/virsh.pod |    7 ++++
>  2 files changed, 96 insertions(+), 0 deletions(-)
> 
> diff --git a/tools/virsh.c b/tools/virsh.c
> index 3a59746..1b93852 100644
> --- a/tools/virsh.c
> +++ b/tools/virsh.c
> @@ -16037,6 +16037,94 @@ cleanup:
>  }
>  
>  /*
> + * "domioerror" command
> + */

> +
> +static const vshCmdInfo info_domblkerror[] = {

Looks like you missed a rename; s/domioerror/domblkerror/ in the comment.

> +    {"help", N_("Show errors on block devices")},
> +    {"desc", N_("Show block device errors")},
> +    {NULL, NULL}
> +};
> +
> +static const vshCmdOptDef opts_domblkerror[] = {
> +    {"domain", VSH_OT_DATA, VSH_OFLAG_REQ, N_("domain name, id, or uuid")},
> +    {NULL, 0, 0, NULL}

Should we also allow this additional usage:

virsh domblkerror dom vda

which lists the status of just vda (no error, no space left, ...)?  That
would mean adding:

{"disk", VSH_OT_DATA, VSH_OFLAG_OPT, N_("particular disk to check")}

then filtering through the libvirt API to match just that disk?  You can
say "no, that's overkill" and not implement it, and I won't be hurt.

> +    if (!(xml = virDomainGetXMLDesc(dom, 0)))
> +        goto cleanup;
> +
> +    xmldoc = virXMLParseStringCtxt(xml, _("(domain_definition)"), &ctxt);
> +    VIR_FREE(xml);
> +    if (!xmldoc)
> +        goto cleanup;
> +
> +    if (virXPathInt("count(./devices/disk)", ctxt, &ndisks) < 0)
> +        goto cleanup;

Guess what - if we made the API take NULL,0 as a shorthand to query how
big to make our array, you wouldn't have to resort to this XML parsing.

> +    if ((count = virDomainGetDiskErrors(dom, disks, ndisks, 0)) == -1)
> +        goto cleanup;
> +
> +    for (i = 0; i < count; i++) {
> +        vshPrint(ctl, "%s: %s\n",
> +                 disks[i].disk,
> +                 vshDomainIOErrorToString(disks[i].error));
> +    }

Are we okay that if there are no disk errors (count is 0), we have no
output, not even mentioning that the command succeeded?

> @@ -16240,6 +16328,7 @@ static const vshCmdDef domMonitoringCmds[] = {
>      {"domblklist", cmdDomblklist, opts_domblklist, info_domblklist, 0},
>      {"domblkstat", cmdDomblkstat, opts_domblkstat, info_domblkstat, 0},
>      {"domcontrol", cmdDomControl, opts_domcontrol, info_domcontrol, 0},
> +    {"domblkerror", cmdDomBlkError, opts_domblkerror, info_domblkerror, 0},

Sort this line earlier.

Overall, I like the series, and want this API in 0.9.10; can we get a v3
prior to RC1?

-- 
Eric Blake   eblake at redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 620 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20120130/37d8636c/attachment-0001.sig>