We already provide ways to detect when a domain has been paused as a result of I/O error, but there was no way of getting the exact error or even the device that experienced it. This new API may be used for both. --- daemon/remote.c | 40 ++++++++++++++++++++++++++++++++ include/libvirt/libvirt.h.in | 32 +++++++++++++++++++++++++ python/generator.py | 3 +- src/driver.h | 7 +++++ src/libvirt.c | 52 ++++++++++++++++++++++++++++++++++++++++++ src/libvirt_public.syms | 1 + src/remote/remote_driver.c | 34 +++++++++++++++++++++++++++ src/remote/remote_protocol.x | 22 +++++++++++++++++- src/remote_protocol-structs | 16 +++++++++++++ src/rpc/gendispatch.pl | 47 +++++++++++++++++++++++++++++++++++++ 10 files changed, 252 insertions(+), 2 deletions(-) diff --git a/daemon/remote.c b/daemon/remote.c index cb8423a..31bf1de 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -98,6 +98,12 @@ remoteDeserializeTypedParameters(remote_typed_param *args_params_val, int limit, int *nparams); +static int +remoteSerializeDomainDiskErrors(virDomainDiskErrorPtr errors, + int nerrors, + remote_domain_disk_error **ret_errors_val, + u_int *ret_errors_len); + #include "remote_dispatch.h" #include "qemu_dispatch.h" @@ -3725,3 +3731,37 @@ make_nonnull_domain_snapshot(remote_nonnull_domain_snapshot *snapshot_dst, virDo snapshot_dst->name = strdup(snapshot_src->name); make_nonnull_domain(&snapshot_dst->dom, snapshot_src->domain); } + +static int +remoteSerializeDomainDiskErrors(virDomainDiskErrorPtr errors, + int nerrors, + remote_domain_disk_error **ret_errors_val, + u_int *ret_errors_len) +{ + remote_domain_disk_error *val = NULL; + int i = 0; + + if (VIR_ALLOC_N(val, nerrors) < 0) + goto no_memory; + + for (i = 0; i < nerrors; i++) { + if (!(val[i].disk = strdup(errors[i].disk))) + goto no_memory; + val[i].error = errors[i].error; + } + + *ret_errors_len = nerrors; + *ret_errors_val = val; + + return 0; + +no_memory: + if (val) { + int j; + for (j = 0; j < i; j++) + VIR_FREE(val[j].disk); + VIR_FREE(val); + } + virReportOOMError(); + return -1; +} diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index 0a7b324..272d142 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -1967,6 +1967,38 @@ virDomainGetBlockIoTune(virDomainPtr dom, int *nparams, unsigned int flags); +/** + * virDomainDiskErrorCode: + * + * Disk I/O error. + */ +typedef enum { + VIR_DOMAIN_DISK_ERROR_NONE = 0, /* no error */ + VIR_DOMAIN_DISK_ERROR_UNSPEC = 1, /* unspecified I/O error */ + VIR_DOMAIN_DISK_ERROR_NO_SPACE = 2, /* no space left on the device */ + +#ifdef VIR_ENUM_SENTINELS + VIR_DOMAIN_DISK_ERROR_LAST +#endif +} virDomainDiskErrorCode; + +/** + * virDomainDiskError: + * + */ +typedef struct _virDomainDiskError virDomainDiskError; +typedef virDomainDiskError *virDomainDiskErrorPtr; + +struct _virDomainDiskError { + char *disk; /* disk target */ + int error; /* virDomainDiskErrorCode */ +}; + +int virDomainGetDiskErrors(virDomainPtr dom, + virDomainDiskErrorPtr errors, + int maxerrors, + unsigned int flags); + /* * NUMA support diff --git a/python/generator.py b/python/generator.py index 6f813ae..b514af5 100755 --- a/python/generator.py +++ b/python/generator.py @@ -423,7 +423,8 @@ skip_impl = ( 'virDomainGetBlockIoTune', 'virDomainSetInterfaceParameters', 'virDomainGetInterfaceParameters', - 'virDomainGetCPUStats' # not implemented now. + 'virDomainGetCPUStats', # not implemented now. + 'virDomainGetDiskErrors', ) qemu_skip_impl = ( diff --git a/src/driver.h b/src/driver.h index ba7dbc4..f3ded19 100644 --- a/src/driver.h +++ b/src/driver.h @@ -810,6 +810,12 @@ typedef int unsigned int ncpus, unsigned int flags); +typedef int + (*virDrvDomainGetDiskErrors)(virDomainPtr dom, + virDomainDiskErrorPtr errors, + int maxerrors, + unsigned int flags); + /** * _virDriver: * @@ -981,6 +987,7 @@ struct _virDriver { virDrvDomainSetBlockIoTune domainSetBlockIoTune; virDrvDomainGetBlockIoTune domainGetBlockIoTune; virDrvDomainGetCPUStats domainGetCPUStats; + virDrvDomainGetDiskErrors domainGetDiskErrors; }; typedef int diff --git a/src/libvirt.c b/src/libvirt.c index e702a34..345c95b 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -18281,3 +18281,55 @@ error: virDispatchError(domain->conn); return -1; } + +/** + * virDomainGetDiskErrors: + * @dom: a domain object + * @errors: array to populate on output + * @maxerrors: size of @errors array + * @flags: extra flags; not used yet, so callers should always pass 0 + * + * The function populates @errors array with all disks that encountered an + * I/O error. Each disk is identified by its target (the dev attribute of + * target subelement in domain XML), such as "vda", and accompanied with + * the error that was seen on it. + * + * Returns number of disks with errors filled in the @errors array or -1 on + * error. + */ +int +virDomainGetDiskErrors(virDomainPtr dom, + virDomainDiskErrorPtr errors, + int maxerrors, + unsigned int flags) +{ + VIR_DOMAIN_DEBUG(dom, "errors=%p, maxerrors=%d, flags=%x", + errors, maxerrors, flags); + + virResetLastError(); + + if (!VIR_IS_DOMAIN(dom)) { + virLibDomainError(VIR_ERR_INVALID_DOMAIN, __FUNCTION__); + virDispatchError(NULL); + return -1; + } + + if (dom->conn->flags & VIR_CONNECT_RO) { + virLibDomainError(VIR_ERR_OPERATION_DENIED, __FUNCTION__); + goto error; + } + + if (dom->conn->driver->domainGetDiskErrors) { + int ret = dom->conn->driver->domainGetDiskErrors(dom, errors, + maxerrors, flags); + if (ret < 0) + goto error; + return ret; + } + + virLibConnError(VIR_ERR_NO_SUPPORT, __FUNCTION__); + +error: + virDispatchError(dom->conn); + return -1; +} diff --git a/src/libvirt_public.syms b/src/libvirt_public.syms index 1c4e0a3..ced9fb3 100644 --- a/src/libvirt_public.syms +++ b/src/libvirt_public.syms @@ -519,6 +519,7 @@ LIBVIRT_0.9.9 { LIBVIRT_0.9.10 { global: virDomainGetCPUStats; + virDomainGetDiskErrors; virDomainPMSuspendForDuration; virDomainShutdownFlags; virStorageVolResize; diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 61b96e9..33b468a 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -1427,6 +1427,39 @@ cleanup: } static int +remoteDeserializeDomainDiskErrors(remote_domain_disk_error *ret_errors_val, + u_int ret_errors_len, + int limit, + virDomainDiskErrorPtr errors, + int maxerrors) +{ + int i = 0; + int j; + + if (ret_errors_len > limit || ret_errors_len > maxerrors) { + remoteError(VIR_ERR_RPC, "%s", + _("returned number of disk errors exceeds limit")); + goto error; + } + + for (i = 0; i < ret_errors_len; i++) { + if (!(errors[i].disk = strdup(ret_errors_val[i].disk))) { + virReportOOMError(); + goto error; + } + errors[i].error = ret_errors_val[i].error; + } + + return 0; + +error: + for (j = 0; j < i; j++) + VIR_FREE(errors[i].disk); + + return -1; +} + +static int remoteDomainBlockStatsFlags(virDomainPtr domain, const char *path, virTypedParameterPtr params, @@ -4849,6 +4882,7 @@ static virDriver remote_driver = { .domainSetNumaParameters = remoteDomainSetNumaParameters, /* 0.9.9 */ .domainGetNumaParameters = remoteDomainGetNumaParameters, /* 0.9.9 */ .domainGetCPUStats = remoteDomainGetCPUStats, /* 0.9.10 */ + .domainGetDiskErrors = remoteDomainGetDiskErrors, /* 0.9.10 */ }; static virNetworkDriver network_driver = { diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x index b58925a..547c108 100644 --- a/src/remote/remote_protocol.x +++ b/src/remote/remote_protocol.x @@ -219,6 +219,11 @@ const REMOTE_DOMAIN_GET_CPU_STATS_NCPUS_MAX = 128; */ const REMOTE_DOMAIN_GET_CPU_STATS_MAX = 2048; +/* + * Upper limit on number of disks with errors + */ +const REMOTE_DOMAIN_DISK_ERRORS_MAX = 256; + /* UUID. VIR_UUID_BUFLEN definition comes from libvirt.h */ typedef opaque remote_uuid[VIR_UUID_BUFLEN]; @@ -359,6 +364,10 @@ struct remote_node_get_memory_stats { unsigned hyper value; }; +struct remote_domain_disk_error { + remote_nonnull_string disk; + int error; +}; /*----- Calls. -----*/ @@ -2397,6 +2406,16 @@ struct remote_domain_shutdown_flags_args { unsigned int flags; }; +struct remote_domain_get_disk_errors_args { + remote_nonnull_domain dom; + int maxerrors; + unsigned int flags; +}; + +struct remote_domain_get_disk_errors_ret { + remote_domain_disk_error errors<REMOTE_DOMAIN_DISK_ERRORS_MAX>; /* insert@1 */ +}; + /*----- Protocol. -----*/ @@ -2708,7 +2727,8 @@ enum remote_procedure { REMOTE_PROC_STORAGE_VOL_RESIZE = 260, /* autogen autogen */ REMOTE_PROC_DOMAIN_PM_SUSPEND_FOR_DURATION = 261, /* autogen autogen */ - REMOTE_PROC_DOMAIN_GET_CPU_STATS = 262 /* skipgen skipgen */ + REMOTE_PROC_DOMAIN_GET_CPU_STATS = 262, /* skipgen skipgen */ + REMOTE_PROC_DOMAIN_GET_DISK_ERRORS = 263 /* autogen autogen */ /* * Notice how the entries are grouped in sets of 10 ? diff --git a/src/remote_protocol-structs b/src/remote_protocol-structs index 5eac9bf..e031d04 100644 --- a/src/remote_protocol-structs +++ b/src/remote_protocol-structs @@ -94,6 +94,10 @@ struct remote_node_get_memory_stats { remote_nonnull_string field; uint64_t value; }; +struct remote_domain_disk_error { + remote_nonnull_string disk; + int error; +}; struct remote_open_args { remote_string name; u_int flags; @@ -1866,6 +1870,17 @@ struct remote_domain_shutdown_flags_args { remote_nonnull_domain dom; u_int flags; }; +struct remote_domain_get_disk_errors_args { + remote_nonnull_domain dom; + int maxerrors; + u_int flags; +}; +struct remote_domain_get_disk_errors_ret { + struct { + u_int errors_len; + remote_domain_disk_error * errors_val; + } errors; +}; enum remote_procedure { REMOTE_PROC_OPEN = 1, REMOTE_PROC_CLOSE = 2, @@ -2129,4 +2144,5 @@ enum remote_procedure { REMOTE_PROC_STORAGE_VOL_RESIZE = 260, REMOTE_PROC_DOMAIN_PM_SUSPEND_FOR_DURATION = 261, REMOTE_PROC_DOMAIN_GET_CPU_STATS = 262, + REMOTE_PROC_DOMAIN_GET_DISK_ERRORS = 263, }; diff --git a/src/rpc/gendispatch.pl b/src/rpc/gendispatch.pl index 446f229..72c1159 100755 --- a/src/rpc/gendispatch.pl +++ b/src/rpc/gendispatch.pl @@ -539,6 +539,39 @@ elsif ($opt_b) { } elsif ($ret_member =~ m/^remote_nonnull_string (\S+)<\S+>;/) { # error out on unannotated arrays die "remote_nonnull_string array without insert@<offset> annotation: $ret_member"; + } elsif ($ret_member =~ m/^remote_domain_disk_error (\S+)<(\S+)>;\s*\/\*\s*insert@(\d+)\s*\*\//) { + push(@vars_list, + "virDomainDiskErrorPtr $1 = NULL", + "int len"); + splice(@args_list, int($3), 0, ("$1")); + push(@getters_list, + " if (args->max$1 > $2) {", + " virNetError(VIR_ERR_INTERNAL_ERROR, \"%s\",", + " _(\"max$1 too large\"));", + " goto cleanup;", + " }", + "", + " if (VIR_ALLOC_N($1, args->max$1) < 0) {", + " virReportOOMError();", + " goto cleanup;", + " }\n"); + $single_ret_var = "len"; + $single_ret_check = " < 0"; + push(@ret_list, + "if (remoteSerializeDomainDiskErrors($1, len,", + " &ret->$1.$1_val,", + " &ret->$1.$1_len) < 0)", + " goto cleanup;\n"); + push(@free_list, + " if ($1) {", + " int i;", + " for (i = 0; i < len; i++)", + " VIR_FREE(${1}[i].disk);", + " }", + " VIR_FREE($1);"); + } elsif ($ret_member =~ m/^remote_domain_disk_error (\S+)<\S+>;/) { + # error out on unannotated arrays + die "remote_domain_disk_error array without insert@<offset> annotation: $ret_member"; } elsif ($ret_member =~ m/^remote_nonnull_string (\S+);/) { if ($call->{ProcName} eq "GetType") { # SPECIAL: virConnectGetType returns a constant string that must @@ -1297,6 +1330,20 @@ elsif ($opt_k) { } elsif ($ret_member =~ m/^remote_typed_param (\S+)<\S+>;/) { # error out on unannotated arrays die "remote_typed_param array without insert@<offset> annotation: $ret_member"; + } elsif ($ret_member =~ m/^remote_domain_disk_error (\S+)<(\S+)>;\s*\/\*\s*insert@(\d+)\s*\*\//) { + splice(@args_list, int($3), 0, ("virDomainDiskErrorPtr $1")); + push(@ret_list, "rv = ret.$1.$1_len;"); + push(@ret_list2, + "if (remoteDeserializeDomainDiskErrors(ret.$1.$1_val,", + " ret.$1.$1_len,", + " $2,", + " $1,", + " max$1) < 0)", + " goto cleanup;"); + $single_ret_cleanup = 1; + } elsif ($ret_member =~ m/^remote_domain_disk_error (\S+)<\S+>;/) { + # error out on unannotated arrays + die "remote_domain_disk_error array without insert@<offset> annotation: $ret_member"; } elsif ($ret_member =~ m/^int (\S+);/) { my $arg_name = $1; -- 1.7.8.4

On Mon, Jan 30, 2012 at 15:56:11 -0700, Eric Blake wrote: I did so and addressed the other comments in v3. ... Yeah, although I didn't try so hard after making this a multi-return call, so v3 now uses skipgen skipgen :-) The generator already seems to know how to deal with virTypedParameter but only for APIs with only one item in their *_ret structure. Jirka

On 01/30/2012 09:01 AM, Jiri Denemark wrote: Ah, so this only returns disks with errors. Document that in the public API. 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. 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(a)redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

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 char * +vshDomainIOErrorToString(int error) +{ + switch ((virDomainDiskErrorCode) error) { + case VIR_DOMAIN_DISK_ERROR_NONE: + return _("no error"); + case VIR_DOMAIN_DISK_ERROR_UNSPEC: + return _("unspecified error"); + case VIR_DOMAIN_DISK_ERROR_NO_SPACE: + return _("no space"); + case VIR_DOMAIN_DISK_ERROR_LAST: + ; + } + + return _("unknown error"); +} + +static const vshCmdInfo info_domblkerror[] = { + {"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} +}; + +static bool +cmdDomBlkError(vshControl *ctl, const vshCmd *cmd) +{ + virDomainPtr dom; + char *xml; + xmlDocPtr xmldoc = NULL; + xmlXPathContextPtr ctxt = NULL; + virDomainDiskErrorPtr disks = NULL; + int ndisks; + int i; + int count; + bool ret = false; + + if (!vshConnectionUsability(ctl, ctl->conn)) + return false; + + if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) + return false; + + 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; + + if (ndisks < 1) { + vshError(ctl, _("no disks attached to this domain")); + goto cleanup; + } + + if (VIR_ALLOC_N(disks, ndisks) < 0) + goto cleanup; + + 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)); + } + + ret = true; + +cleanup: + VIR_FREE(disks); + xmlXPathFreeContext(ctxt); + xmlFreeDoc(xmldoc); + virDomainFree(dom); + return ret; +} + +/* * "qemu-monitor-command" command */ static const vshCmdInfo info_qemu_monitor_command[] = { @@ -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}, {"domif-getlink", cmdDomIfGetLink, opts_domif_getlink, info_domif_getlink, 0}, {"domiflist", cmdDomiflist, opts_domiflist, info_domiflist, 0}, {"domifstat", cmdDomIfstat, opts_domifstat, info_domifstat, 0}, diff --git a/tools/virsh.pod b/tools/virsh.pod index 4bc25bf..e9598ac 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -507,6 +507,13 @@ on hypervisor. Get memory stats for a running domain. +=item B<domblkerror> I<domain-id> + +Show errors on block devices. This command usually comes handy when +B<domstate> command says that a domain was paused due to I/O error. +The B<domblkerror> command lists all block devices in error state and +the error seen on each of them. + =item B<domblkinfo> I<domain> I<block-device> Get block device size info for a domain. A I<block-device> corresponds -- 1.7.8.4