[libvirt] [PATCH 1/4] virsh: Refactor event printing

[Andrea Bolognani]

n Mon, 2015-12-21 at 11:04 +0100, Jiri Denemark wrote:
> To reduce code duplication.
> 
> Signed-off-by: Jiri Denemark <jdenemar at redhat.com>
> ---
>  tools/virsh-domain.c | 293 ++++++++++++++++++++++++---------------------------
>  1 file changed, 136 insertions(+), 157 deletions(-)

Really nice cleanup, looks good overall.

A couple of comments below.

>  static void
> -virshEventPrint(virConnectPtr conn ATTRIBUTE_UNUSED, virDomainPtr dom,
> -                const char *event, long long seconds, unsigned int micros,
> -                const char *details, void *opaque)
> +virshEventQemuPrint(virConnectPtr conn ATTRIBUTE_UNUSED,
> +                    virDomainPtr dom,
> +                    const char *event,
> +                    long long seconds,
> +                    unsigned int micros,
> +                    const char *details,
> +                    void *opaque)
>  {
>      virshQemuEventData *data = opaque;
>      virJSONValuePtr pretty = NULL;

A comment describing the functions would be nice. Their use is
pretty obvious, so not a deal breaker.

>  static void
> +virshEventPrint(virshDomEventData *data,
> +                virBufferPtr buf)
> +{
> +    char *msg;
> +
> +    if (!(msg = virBufferContentAndReset(buf)))
> +        return;
> +
> +    if (!data->loop && *data->count)
> +        goto cleanup;
> +
> +    vshPrint(data->ctl, "%s", msg);
> +
> +    (*data->count)++;
> +    if (!data->loop)
> +        vshEventDone(data->ctl);
> +
> + cleanup:
> +    VIR_FREE(msg);
> +}

This works just fine, however I dislike the fact that the virBuffer
is allocated by the caller but released here.

I'd rather use virBufferCurrentContent() here and leave the caller
responsible for releasing the buffer. Doing so would make the callers
slightly more verbose, but result in cleaner code overall IMHO.

> @@ -12237,26 +12249,26 @@ virshEventGraphicsPrint(virConnectPtr conn ATTRIBUTE_UNUSED,
>                          const virDomainEventGraphicsSubject *subject,
>                          void *opaque)
>  {
> -    virshDomEventData *data = opaque;
> +    virBuffer buf = VIR_BUFFER_INITIALIZER;
>      size_t i;
>  
> -    if (!data->loop && *data->count)
> -        return;
> -    vshPrint(data->ctl, _("event 'graphics' for domain %s: "
> -                          "%s local[%s %s %s] remote[%s %s %s] %s"),
> -             virDomainGetName(dom), virshGraphicsPhaseToString(phase),
> -             virshGraphicsAddressToString(local->family),
> -             local->node, local->service,
> -             virshGraphicsAddressToString(remote->family),
> -             remote->node, remote->service,
> -             authScheme);
> -    for (i = 0; i < subject->nidentity; i++)
> -        vshPrint(data->ctl, " %s=%s", subject->identities[i].type,
> -                 subject->identities[i].name);
> -    vshPrint(data->ctl, "\n");
> -    (*data->count)++;
> -    if (!data->loop)
> -        vshEventDone(data->ctl);
> +    virBufferAsprintf(&buf, _("event 'graphics' for domain %s: "
> +                              "%s local[%s %s %s] remote[%s %s %s] %s\n"),
> +                      virDomainGetName(dom),
> +                      virshGraphicsPhaseToString(phase),
> +                      virshGraphicsAddressToString(local->family),
> +                      local->node,
> +                      local->service,
> +                      virshGraphicsAddressToString(remote->family),
> +                      remote->node,
> +                      remote->service,
> +                      authScheme);
> +    for (i = 0; i < subject->nidentity; i++) {
> +        virBufferAsprintf(&buf, "\t%s=%s\n",
> +                          subject->identities[i].type,
> +                          subject->identities[i].name);
> +    }
> +    virshEventPrint(opaque, &buf);
>  }

You're changing the format a bit here - I like the new one better, and
it's the same used in virshEventTunablePrint() below. I'm just concerned
about users parsing this in some way and being confused by the change.

>  VIR_ENUM_DECL(virshEventAgentLifecycleState)
> @@ -12462,19 +12446,14 @@ virshEventAgentLifecyclePrint(virConnectPtr conn ATTRIBUTE_UNUSED,
>                                int reason,
>                                void *opaque)
>  {
> -    virshDomEventData *data = opaque;
> +    virBuffer buf = VIR_BUFFER_INITIALIZER;
>  
> -    if (!data->loop && *data->count)
> -        return;
> -    vshPrint(data->ctl,
> -             _("event 'agent-lifecycle' for domain %s: state: '%s' reason: '%s'\n"),
> -             virDomainGetName(dom),
> -             UNKNOWNSTR(virshEventAgentLifecycleStateTypeToString(state)),
> -             UNKNOWNSTR(virshEventAgentLifecycleReasonTypeToString(reason)));
> -
> -    (*data->count)++;
> -    if (!data->loop)
> -        vshEventDone(data->ctl);
> +    virBufferAsprintf(&buf, _("event 'agent-lifecycle' for domain %s: state: "
> +                              "'%s' reason: '%s'\n"),
> +                      virDomainGetName(dom),
> +                      UNKNOWNSTR(virshEventAgentLifecycleStateTypeToString(state)),
> +                      UNKNOWNSTR(virshEventAgentLifecycleReasonTypeToString(reason)));
> +    virshEventPrint(opaque, &buf);
>  }

I can see a lot more places where UNKNOWNSTR() could be used. Out of
scope for this commit, in any case.

Cheers.

-- 
Andrea Bolognani
Software Engineer - Virtualization Team