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

[Bjoern Walk]

Michal Privoznik <mprivozn at redhat.com> [2019-03-27, 02:05PM +0100]:
> 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()

One argument per line is certainly doable. How to exploit the print
function I am not so sure, because I want to have special formatting for
different parameters (i.e., the ULL values for PSW_(MASK|ADDR) are hex).
I don't see a way to get around this with a generic function.

> > +        }
> > +        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.

I intent to have the new API function superseed the existing one, so I
actually want to call virDomainGetStateParams() unconditionally and only
fall back to virshDomainState() when the new API is not there (so that
hopefully at some distant point in the future, we could deprecate the
old stuff).

> Also, printing out some error message would be nice:

If the new API is not available, I wanted to just silently have the old
behaviour. The domain state and reason can always be retrieve (by means
of the old API) only additional information could be missing. I am not
sure if we should error out if this is the case.

> 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;
>      }

-- 
IBM Systems
Linux on Z & Virtualization Development
--------------------------------------------------
IBM Deutschland Research & Development GmbH
Schönaicher Str. 220, 71032 Böblingen
Phone: +49 7031 16 1819
--------------------------------------------------
Vorsitzende des Aufsichtsrats: Matthias Hartmann
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 902 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20190401/5db08880/attachment-0001.sig>