3:24 p.m.
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(a)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.
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