On Mon, Dec 21, 2015 at 14:46:33 +0100, Andrea Bolognani wrote:
> n Mon, 2015-12-21 at 11:04 +0100, Jiri Denemark wrote:
> > To reduce code duplication.
> >
> > Signed-off-by: Jiri Denemark <jdenemar@xxxxxxxxxx>
> > ---
> > 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.
Done. Is it a deal? :-)
> > 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.
Jirka
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list