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