On 3/25/19 9:04 AM, Bjoern Walk wrote:
> Add a new parameter to virsh domstate, --info, to report additional
> information for the domain state, e.g. for a QEMU guest running a S390
> domain:
>
> virsh # domstate --info guest-1
> crashed (panicked: s390: core='0' psw-mask='0x0002000180000000' \
> psw-addr='0x000000000010f146' reason='disabled-wait')
>
> When the new API virDomainGetStateParams is not available for the server
> or not supported by the hypervisor driver, fall back to the old API
> using virDomainGetState function.
>
> The --info parameter implies the --reason parameter and if additional
> information is not available, the output is the same.
>
> Reviewed-by: Boris Fiuczynski <fiuczy@xxxxxxxxxxxxx>
> Signed-off-by: Bjoern Walk <bwalk@xxxxxxxxxxxxx>
> ---
> tools/virsh-domain-monitor.c | 102 +++++++++++++++++++++++++++++++----
> tools/virsh.pod | 5 +-
> 2 files changed, 95 insertions(+), 12 deletions(-)
>
> diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c
> index ad739a9d..bf9d4970 100644
> --- a/tools/virsh-domain-monitor.c
> +++ b/tools/virsh-domain-monitor.c
> @@ -1391,35 +1391,117 @@ static const vshCmdOptDef opts_domstate[] = {
> .type = VSH_OT_BOOL,
> .help = N_("also print reason for the state")
> },
> + {.name = "info",
> + .type = VSH_OT_BOOL,
> + .help = N_("also print reason and information for the state")
> + },
> {.name = NULL}
> };
>
> +static char *
> +vshStateInfoMsgFormat(virTypedParameterPtr params,
> + int nparams)
> +{
> + char *ret = NULL;
> + int type;
> +
> + if (virTypedParamsGetInt(params, nparams,
> + VIR_DOMAIN_STATE_PARAMS_INFO_TYPE, &type) < 0) {
> + return NULL;
> + }
> +
> + switch (type) {
> + case VIR_DOMAIN_STATE_INFO_TYPE_QEMU_HYPERV: {
> + unsigned long long arg1, arg2, arg3, arg4, arg5;
> +
> + if (virTypedParamsGetULLong(params, nparams,
> + VIR_DOMAIN_STATE_PARAMS_PANIC_INFO_HYPERV_ARG1, &arg1) < 0 ||
> + virTypedParamsGetULLong(params, nparams,
> + VIR_DOMAIN_STATE_PARAMS_PANIC_INFO_HYPERV_ARG2, &arg2) < 0 ||
> + virTypedParamsGetULLong(params, nparams,
> + VIR_DOMAIN_STATE_PARAMS_PANIC_INFO_HYPERV_ARG3, &arg3) < 0 ||
> + virTypedParamsGetULLong(params, nparams,
> + VIR_DOMAIN_STATE_PARAMS_PANIC_INFO_HYPERV_ARG4, &arg4) < 0 ||
> + virTypedParamsGetULLong(params, nparams,
> + VIR_DOMAIN_STATE_PARAMS_PANIC_INFO_HYPERV_ARG5, &arg5) < 0) {
> + return NULL;
> + }
> +
> + ignore_value(virAsprintf(&ret, "hyper-v: arg1='0x%llx', arg2='0x%llx', "
> + "arg3='0x%llx', arg4='0x%llx', arg5='0x%llx'",
> + arg1, arg2, arg3, arg4, arg5));
> + }
> + break;
> + case VIR_DOMAIN_STATE_INFO_TYPE_QEMU_S390: {
> + int core;
> + unsigned long long psw_mask;
> + unsigned long long psw_addr;
> + const char *reason;
> +
> + if (virTypedParamsGetInt(params, nparams,
> + VIR_DOMAIN_STATE_PARAMS_PANIC_INFO_S390_CORE, &core) < 0 ||
> + virTypedParamsGetULLong(params, nparams,
> + VIR_DOMAIN_STATE_PARAMS_PANIC_INFO_S390_PSW_MASK, &psw_mask) < 0 ||
> + virTypedParamsGetULLong(params, nparams,
> + VIR_DOMAIN_STATE_PARAMS_PANIC_INFO_S390_PSW_ADDR, &psw_addr) < 0 ||
> + virTypedParamsGetString(params, nparams,
> + VIR_DOMAIN_STATE_PARAMS_PANIC_INFO_S390_PSW_REASON, &reason) < 0) {
> + return NULL;
> + }
> +
> + ignore_value(virAsprintf(&ret, "s390: core='%d' psw-mask='0x%016llx' "
> + "psw-addr='0x%016llx' reason='%s'",
> + core, psw_mask, psw_addr, reason));
Don't we want to print this in some more machine friendly way? For instance one argument per line? Also, we have a function (sort of) that prints out typed params. You may take the inspiration from virshDomainStatsPrintRecord()
> + }
> + break;
> + case VIR_DOMAIN_STATE_INFO_TYPE_NONE:
> + case VIR_DOMAIN_STATE_INFO_TYPE_LAST:
> + break;
> + }
> +
> + return ret;
> +}
> +
> static bool
> cmdDomstate(vshControl *ctl, const vshCmd *cmd)
> {
> virDomainPtr dom;
> bool ret = true;
> bool showReason = vshCommandOptBool(cmd, "reason");
> + bool showInfo = vshCommandOptBool(cmd, "info");
> + virTypedParameterPtr params = NULL;
> + int nparams = 0;
> int state, reason;
> + char *info = NULL;
>
> if (!(dom = virshCommandOptDomain(ctl, cmd, NULL)))
> return false;
>
> - if ((state = virshDomainState(ctl, dom, &reason)) < 0) {
> - ret = false;
> - goto cleanup;
> + if (virDomainGetStateParams(dom, &state, &reason, ¶ms, &nparams, 0) < 0) {
> + if ((state = virshDomainState(ctl, dom, &reason)) < 0) {
> + ret = false;
> + goto cleanup;
> + }
> }
This looks fishy. Firstly, if --info was requested but we're talking to
an older daemon then virDomainGetStateParams() will fail (the API is not
there) and virshDomainState() is called instead which doesn't fill out
@params and thus we won't print any additional info even though it was
requested.
Also, printing out some error message would be nice:
diff --git i/tools/virsh-domain-monitor.c w/tools/virsh-domain-monitor.c
index bf9d4970a7..27c33f354a 100644
--- i/tools/virsh-domain-monitor.c
+++ w/tools/virsh-domain-monitor.c
@@ -1477,11 +1477,13 @@ cmdDomstate(vshControl *ctl, const vshCmd *cmd)
if (!(dom = virshCommandOptDomain(ctl, cmd, NULL)))
return false;
- if (virDomainGetStateParams(dom, &state, &reason, ¶ms, &nparams, 0) < 0) {
- if ((state = virshDomainState(ctl, dom, &reason)) < 0) {
- ret = false;
- goto cleanup;
- }
+ if ((showInfo &&
+ virDomainGetStateParams(dom, &state, &reason, ¶ms, &nparams, 0) < 0) ||
+ (!showInfo &&
+ (state = virshDomainState(ctl, dom, &reason)) < 0)) {
+ vshError(ctl, _("Unable to get state of domain %s"), virDomainGetName(dom));
+ ret = false;
+ goto cleanup;
}
>
> - if (showReason) {
> - vshPrint(ctl, "%s (%s)\n",
> - virshDomainStateToString(state),
> - virshDomainStateReasonToString(state, reason));
> - } else {
> - vshPrint(ctl, "%s\n",
> - virshDomainStateToString(state));
> + vshPrint(ctl, "%s", virshDomainStateToString(state));
> +
> + if (showReason || showInfo) {
> + vshPrint(ctl, " (%s", virshDomainStateReasonToString(state, reason));
> +
> + if (showInfo) {
> + info = vshStateInfoMsgFormat(params, nparams);
> + if (info)
> + vshPrint(ctl, ": %s", info);
> + }
> + vshPrint(ctl, ")");
> }
>
> + vshPrint(ctl, "\n");
> +
> cleanup:
> + VIR_FREE(info);
> + virTypedParamsFree(params, nparams);
> virshDomainFree(dom);
> return ret;
> }
> diff --git a/tools/virsh.pod b/tools/virsh.pod
> index db723431..82ab83a3 100644
> --- a/tools/virsh.pod
> +++ b/tools/virsh.pod
> @@ -1568,10 +1568,11 @@ specified in the second argument.
>
> B<Note>: Domain must be inactive and without snapshots.
>
> -=item B<domstate> I<domain> [I<--reason>]
> +=item B<domstate> I<domain> [I<--reason>] [I<--info>]
>
> Returns state about a domain. I<--reason> tells virsh to also print
> -reason for the state.
> +reason for the state. I<--info> prints additional state information if
> +available, I<--info> implies I<--reason>.
>
> =item B<domcontrol> I<domain>
>
>
Michal
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
