[libvirt] [PATCH v2 2/3] qemu: Implement virDomainGetDiskErrors

[Eric Blake]

On 01/30/2012 09:01 AM, Jiri Denemark wrote:
> ---
>  src/qemu/qemu_conf.h         |    1 +
>  src/qemu/qemu_driver.c       |   83 ++++++++++++++++++++++++++++++++++++++++++
>  src/qemu/qemu_monitor.c      |   40 ++++++++++++++++++++
>  src/qemu/qemu_monitor.h      |    1 +
>  src/qemu/qemu_monitor_json.c |    8 ++++
>  src/qemu/qemu_monitor_text.c |   15 ++++++++
>  6 files changed, 148 insertions(+), 0 deletions(-)
> 

> +static int
> +qemuDomainGetDiskErrors(virDomainPtr dom,
> +                        virDomainDiskErrorPtr errors,
> +                        int nerrors,
> +                        unsigned int flags)
> +{

> +    qemuDomainObjEnterMonitor(driver, vm);
> +    table = qemuMonitorGetBlockInfo(priv->mon);
> +    qemuDomainObjExitMonitor(driver, vm);
> +    if (!table)
> +        goto endjob;
> +
> +    for (i = n = 0; i < vm->def->ndisks; i++) {
> +        struct qemuDomainDiskInfo *info;
> +        virDomainDiskDefPtr disk = vm->def->disks[i];
> +
> +        if ((info = virHashLookup(table, disk->info.alias)) &&
> +            info->io_status != VIR_DOMAIN_DISK_ERROR_NONE) {

Ah, so this only returns disks with errors.  Document that in the public
API.

> +            if (n >= nerrors) {
> +                qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                                _("too many disks with error"));

Do we really want to error out if we can't tell them about _all_ known
errors?  Let's think back to my request in 1/3 - if the user can query
how many errors are present, and at the moment we answer the query,
there is only 1 disk with an error, but before they can allocate and
repeat the request, we encounter a second error.

Either they should _always_ pass us space for _all_ disks tied to the
domain (and fail if nerrors < vm->def->ndisks), or we should allow for
races with the user passing in a too-small array, where we just give
back as much information as they gave us room for, rather than erroring
out because they didn't give us enough room.  In fact, if you go with my
request to allow the user shorthand, I'm okay with special case nerrors
== 0 to just blindly return vm->def->ndisks, even if there aren't that
many errors, so that the user will be unlikely to ever pass us too small
of an array (the user should be prepared for the return value to be less
than maxerrors on input, in fact, a return value of 0 when there are no
errors would be reasonable in the case where the array is non-NULL).

The virTypedParameter cases must not reject a too-small array, but then
again, that is for back-compat reasons (it is likely that a newer server
introduces attributes not expected by an older client, so if the older
client hard-coded the array size, the newer server should still deliver
only the older stats that the older client was expecting); but for the
disk error case, the number of disks is dependent on how many disks are
attached to the domain, and not something where newer servers will see
more disks than older clients, so for this API, I can live with an error
if the user's incoming array is too small.

> +++ b/src/qemu/qemu_monitor_text.c
> @@ -839,6 +839,21 @@ int qemuMonitorTextGetBlockInfo(qemuMonitorPtr mon,
>                          VIR_DEBUG("error reading tray_open: %s", p);

Do we still need to implement this in qemu_monitor_text, now that we
require JSON for any qemu version new enough to actually provide this
information in the first place?

-- 
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/7a502500/attachment-0001.sig>