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

  > >   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.      + */

> > @@ -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.