Re: [libvirt] [PATCH v2 6/7] virsh: domstate: report detailed state if available

On 3/25/19 9:04 AM, Bjoern Walk wrote: > + 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, &params, &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, &params, &nparams, 0) < 0) { - if ((state = virshDomainState(ctl, dom, &reason)) < 0) { - ret = false; - goto cleanup; - } + if ((showInfo && + virDomainGetStateParams(dom, &state, &reason, &params, &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; }