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

[Andrea Bolognani]

On Thu, t 22:24 +0100, Jiri Denemark wrote:
> 
> > >   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.
> 
> I think we just need a comment explicitly saying that this function will
> free the buffer. And I added it to the description requested by your
> not-a-deal-breaker comment.
> 
> Is this good enough?
> 
>     +/**
>     + * virshEventPrint:
>     + *
>     + * @data: opaque data passed to all event callbacks
>     + * @buf: string buffer describing the event
>     + *
>     + * Print the event description found in @buf and update virshDomEventData.
>     + *
>     + * This function resets @buf and frees all memory consumed by its content.
>     + */

I still don't like it :)

But there are lots of other places in libvirt where memory allocated
by the caller is freed by the callee, so I guess I'll just have to
live with it either way :)

> > > @@ -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.
> 
> Right, I changed the format to be consistent with another event which
> also reported name=value stuff. I don't think we need to be worried
> about the output in this particular case.

Since you're confident changing the format won't break anything,

ACK.

Cheers.

-- 
Andrea Bolognani
Software Engineer - Virtualization Team