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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list