To reduce code duplication. Signed-off-by: Jiri Denemark <jdenemar(a)redhat.com&gt; --- tools/virsh-domain.c | 293 ++++++++++++++++++++++++--------------------------- 1 file changed, 136 insertions(+), 157 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index edbbc34..ba38ed9 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -9162,9 +9162,13 @@ struct virshQemuEventData { typedef struct virshQemuEventData virshQemuEventData; 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; @@ -9261,7 +9265,7 @@ cmdQemuMonitorEvent(vshControl *ctl, const vshCmd *cmd) if ((eventId = virConnectDomainQemuMonitorEventRegister(priv->conn, dom, event, - virshEventPrint, + virshEventQemuPrint, &data, NULL, flags)) < 0) goto cleanup; @@ -12139,19 +12143,38 @@ struct virshDomEventData { typedef struct virshDomEventData virshDomEventData; 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); +} + +static void virshEventGenericPrint(virConnectPtr conn ATTRIBUTE_UNUSED, virDomainPtr dom, void *opaque) { - virshDomEventData *data = opaque; + virBuffer buf = VIR_BUFFER_INITIALIZER; - if (!data->loop && *data->count) - return; - vshPrint(data->ctl, _("event '%s' for domain %s\n"), - data->cb->name, virDomainGetName(dom)); - (*data->count)++; - if (!data->loop) - vshEventDone(data->ctl); + virBufferAsprintf(&buf, _("event '%s' for domain %s\n"), + ((virshDomEventData *) opaque)->cb->name, + virDomainGetName(dom)); + virshEventPrint(opaque, &buf); } static void @@ -12161,16 +12184,13 @@ virshEventLifecyclePrint(virConnectPtr conn ATTRIBUTE_UNUSED, int detail, void *opaque) { - virshDomEventData *data = opaque; + virBuffer buf = VIR_BUFFER_INITIALIZER; - if (!data->loop && *data->count) - return; - vshPrint(data->ctl, _("event 'lifecycle' for domain %s: %s %s\n"), - virDomainGetName(dom), virshDomainEventToString(event), - virshDomainEventDetailToString(event, detail)); - (*data->count)++; - if (!data->loop) - vshEventDone(data->ctl); + virBufferAsprintf(&buf, _("event 'lifecycle' for domain %s: %s %s\n"), + virDomainGetName(dom), + virshDomainEventToString(event), + virshDomainEventDetailToString(event, detail)); + virshEventPrint(opaque, &buf); } static void @@ -12179,15 +12199,12 @@ virshEventRTCChangePrint(virConnectPtr conn ATTRIBUTE_UNUSED, long long utcoffset, void *opaque) { - virshDomEventData *data = opaque; + virBuffer buf = VIR_BUFFER_INITIALIZER; - if (!data->loop && *data->count) - return; - vshPrint(data->ctl, _("event 'rtc-change' for domain %s: %lld\n"), - virDomainGetName(dom), utcoffset); - (*data->count)++; - if (!data->loop) - vshEventDone(data->ctl); + virBufferAsprintf(&buf, _("event 'rtc-change' for domain %s: %lld\n"), + virDomainGetName(dom), + utcoffset); + virshEventPrint(opaque, &buf); } static void @@ -12196,15 +12213,12 @@ virshEventWatchdogPrint(virConnectPtr conn ATTRIBUTE_UNUSED, int action, void *opaque) { - virshDomEventData *data = opaque; + virBuffer buf = VIR_BUFFER_INITIALIZER; - if (!data->loop && *data->count) - return; - vshPrint(data->ctl, _("event 'watchdog' for domain %s: %s\n"), - virDomainGetName(dom), virshDomainEventWatchdogToString(action)); - (*data->count)++; - if (!data->loop) - vshEventDone(data->ctl); + virBufferAsprintf(&buf, _("event 'watchdog' for domain %s: %s\n"), + virDomainGetName(dom), + virshDomainEventWatchdogToString(action)); + virshEventPrint(opaque, &buf); } static void @@ -12215,16 +12229,14 @@ virshEventIOErrorPrint(virConnectPtr conn ATTRIBUTE_UNUSED, int action, void *opaque) { - virshDomEventData *data = opaque; + virBuffer buf = VIR_BUFFER_INITIALIZER; - if (!data->loop && *data->count) - return; - vshPrint(data->ctl, _("event 'io-error' for domain %s: %s (%s) %s\n"), - virDomainGetName(dom), srcPath, devAlias, - virshDomainEventIOErrorToString(action)); - (*data->count)++; - if (!data->loop) - vshEventDone(data->ctl); + virBufferAsprintf(&buf, _("event 'io-error' for domain %s: %s (%s) %s\n"), + virDomainGetName(dom), + srcPath, + devAlias, + virshDomainEventIOErrorToString(action)); + virshEventPrint(opaque, &buf); } static void @@ -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); } static void @@ -12268,17 +12280,16 @@ virshEventIOErrorReasonPrint(virConnectPtr conn ATTRIBUTE_UNUSED, const char *reason, void *opaque) { - virshDomEventData *data = opaque; + virBuffer buf = VIR_BUFFER_INITIALIZER; - if (!data->loop && *data->count) - return; - vshPrint(data->ctl, _("event 'io-error-reason' for domain %s: " - "%s (%s) %s due to %s\n"), - virDomainGetName(dom), srcPath, devAlias, - virshDomainEventIOErrorToString(action), reason); - (*data->count)++; - if (!data->loop) - vshEventDone(data->ctl); + virBufferAsprintf(&buf, _("event 'io-error-reason' for domain %s: " + "%s (%s) %s due to %s\n"), + virDomainGetName(dom), + srcPath, + devAlias, + virshDomainEventIOErrorToString(action), + reason); + virshEventPrint(opaque, &buf); } static void @@ -12289,17 +12300,15 @@ virshEventBlockJobPrint(virConnectPtr conn ATTRIBUTE_UNUSED, int status, void *opaque) { - virshDomEventData *data = opaque; + virBuffer buf = VIR_BUFFER_INITIALIZER; - if (!data->loop && *data->count) - return; - vshPrint(data->ctl, _("event '%s' for domain %s: %s for %s %s\n"), - data->cb->name, virDomainGetName(dom), - virshDomainBlockJobToString(type), - disk, virshDomainBlockJobStatusToString(status)); - (*data->count)++; - if (!data->loop) - vshEventDone(data->ctl); + virBufferAsprintf(&buf, _("event '%s' for domain %s: %s for %s %s\n"), + ((virshDomEventData *) opaque)->cb->name, + virDomainGetName(dom), + virshDomainBlockJobToString(type), + disk, + virshDomainBlockJobStatusToString(status)); + virshEventPrint(opaque, &buf); } static void @@ -12311,17 +12320,16 @@ virshEventDiskChangePrint(virConnectPtr conn ATTRIBUTE_UNUSED, int reason, void *opaque) { - virshDomEventData *data = opaque; + virBuffer buf = VIR_BUFFER_INITIALIZER; - if (!data->loop && *data->count) - return; - vshPrint(data->ctl, - _("event 'disk-change' for domain %s disk %s: %s -> %s: %s\n"), - virDomainGetName(dom), alias, NULLSTR(oldSrc), NULLSTR(newSrc), - virshDomainEventDiskChangeToString(reason)); - (*data->count)++; - if (!data->loop) - vshEventDone(data->ctl); + virBufferAsprintf(&buf, _("event 'disk-change' for domain %s disk %s: " + "%s -> %s: %s\n"), + virDomainGetName(dom), + alias, + NULLSTR(oldSrc), + NULLSTR(newSrc), + virshDomainEventDiskChangeToString(reason)); + virshEventPrint(opaque, &buf); } static void @@ -12331,17 +12339,13 @@ virshEventTrayChangePrint(virConnectPtr conn ATTRIBUTE_UNUSED, int reason, void *opaque) { - virshDomEventData *data = opaque; + virBuffer buf = VIR_BUFFER_INITIALIZER; - if (!data->loop && *data->count) - return; - vshPrint(data->ctl, - _("event 'tray-change' for domain %s disk %s: %s\n"), - virDomainGetName(dom), alias, - virshDomainEventTrayChangeToString(reason)); - (*data->count)++; - if (!data->loop) - vshEventDone(data->ctl); + virBufferAsprintf(&buf, _("event 'tray-change' for domain %s disk %s: %s\n"), + virDomainGetName(dom), + alias, + virshDomainEventTrayChangeToString(reason)); + virshEventPrint(opaque, &buf); } static void @@ -12361,16 +12365,12 @@ virshEventBalloonChangePrint(virConnectPtr conn ATTRIBUTE_UNUSED, unsigned long long actual, void *opaque) { - virshDomEventData *data = opaque; + virBuffer buf = VIR_BUFFER_INITIALIZER; - if (!data->loop && *data->count) - return; - vshPrint(data->ctl, - _("event 'balloon-change' for domain %s: %lluKiB\n"), - virDomainGetName(dom), actual); - (*data->count)++; - if (!data->loop) - vshEventDone(data->ctl); + virBufferAsprintf(&buf, _("event 'balloon-change' for domain %s: %lluKiB\n"), + virDomainGetName(dom), + actual); + virshEventPrint(opaque, &buf); } static void @@ -12379,16 +12379,12 @@ virshEventDeviceRemovedPrint(virConnectPtr conn ATTRIBUTE_UNUSED, const char *alias, void *opaque) { - virshDomEventData *data = opaque; + virBuffer buf = VIR_BUFFER_INITIALIZER; - if (!data->loop && *data->count) - return; - vshPrint(data->ctl, - _("event 'device-removed' for domain %s: %s\n"), - virDomainGetName(dom), alias); - (*data->count)++; - if (!data->loop) - vshEventDone(data->ctl); + virBufferAsprintf(&buf, _("event 'device-removed' for domain %s: %s\n"), + virDomainGetName(dom), + alias); + virshEventPrint(opaque, &buf); } static void @@ -12397,16 +12393,12 @@ virshEventDeviceAddedPrint(virConnectPtr conn ATTRIBUTE_UNUSED, const char *alias, void *opaque) { - virshDomEventData *data = opaque; + virBuffer buf = VIR_BUFFER_INITIALIZER; - if (!data->loop && *data->count) - return; - vshPrint(data->ctl, - _("event 'device-added' for domain %s: %s\n"), - virDomainGetName(dom), alias); - (*data->count)++; - if (!data->loop) - vshEventDone(data->ctl); + virBufferAsprintf(&buf, _("event 'device-added' for domain %s: %s\n"), + virDomainGetName(dom), + alias); + virshEventPrint(opaque, &buf); } static void @@ -12416,28 +12408,20 @@ virshEventTunablePrint(virConnectPtr conn ATTRIBUTE_UNUSED, int nparams, void *opaque) { - virshDomEventData *data = opaque; + virBuffer buf = VIR_BUFFER_INITIALIZER; size_t i; - char *value = NULL; - - if (!data->loop && *data->count) - return; - - vshPrint(data->ctl, - _("event 'tunable' for domain %s:\n"), - virDomainGetName(dom)); + char *value; + virBufferAsprintf(&buf, _("event 'tunable' for domain %s:\n"), + virDomainGetName(dom)); for (i = 0; i < nparams; i++) { value = virTypedParameterToString(&params[i]); if (value) { - vshPrint(data->ctl, _("\t%s: %s\n"), params[i].field, value); + virBufferAsprintf(&buf, "\t%s: %s\n", params[i].field, value); VIR_FREE(value); } } - - (*data->count)++; - if (!data->loop) - vshEventDone(data->ctl); + virshEventPrint(opaque, &buf); } VIR_ENUM_DECL(virshEventAgentLifecycleState) @@ -12462,19 +12446,14 @@ virshEventAgentLifecyclePrint(virConnectPtr conn ATTRIBUTE_UNUSED, int reason, void *opaque) { - virshDomEventData *data = opaque; + virBuffer buf = VIR_BUFFER_INITIALIZER; - if (!data->loop && *data->count) - return; - vshPrint(data->ctl, - _("event 'agent-lifecycle' for domain %s: state: '%s' reason: '%s'\n"), - virDomainGetName(dom), - UNKNOWNSTR(virshEventAgentLifecycleStateTypeToString(state)), - UNKNOWNSTR(virshEventAgentLifecycleReasonTypeToString(reason))); - - (*data->count)++; - if (!data->loop) - vshEventDone(data->ctl); + virBufferAsprintf(&buf, _("event 'agent-lifecycle' for domain %s: state: " + "'%s' reason: '%s'\n"), + virDomainGetName(dom), + UNKNOWNSTR(virshEventAgentLifecycleStateTypeToString(state)), + UNKNOWNSTR(virshEventAgentLifecycleReasonTypeToString(reason))); + virshEventPrint(opaque, &buf); } static vshEventCallback vshEventCallbacks[] = { -- 2.6.4

On Mon, Dec 21, 2015 at 14:46:33 +0100, Andrea Bolognani wrote: Done. Is it a deal? :-) 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. + */ 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

A new --timestamp option for event virsh command can be used to print timestamp of each event. Signed-off-by: Jiri Denemark <jdenemar(a)redhat.com&gt; --- tools/virsh-domain.c | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index ba38ed9..6e219de 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -12137,6 +12137,7 @@ struct virshDomEventData { vshControl *ctl; bool loop; int *count; + bool timestamp; vshEventCallback *cb; int id; }; @@ -12154,7 +12155,16 @@ virshEventPrint(virshDomEventData *data, if (!data->loop && *data->count) goto cleanup; - vshPrint(data->ctl, "%s", msg); + if (data->timestamp) { + char timestamp[VIR_TIME_STRING_BUFLEN]; + + if (virTimeStringNowRaw(timestamp) < 0) + timestamp[0] = '\0'; + + vshPrint(data->ctl, "%s: %s", timestamp, msg); + } else { + vshPrint(data->ctl, "%s", msg); + } (*data->count)++; if (!data->loop) @@ -12533,6 +12543,10 @@ static const vshCmdOptDef opts_event[] = { .type = VSH_OT_BOOL, .help = N_("list valid event types") }, + {.name = "timestamp", + .type = VSH_OT_BOOL, + .help = N_("show timestamp for each printed event") + }, {.name = NULL} }; @@ -12548,6 +12562,7 @@ cmdEvent(vshControl *ctl, const vshCmd *cmd) int event = -1; bool all = vshCommandOptBool(cmd, "all"); bool loop = vshCommandOptBool(cmd, "loop"); + bool timestamp = vshCommandOptBool(cmd, "timestamp"); int count = 0; virshControlPtr priv = ctl->privData; @@ -12580,6 +12595,7 @@ cmdEvent(vshControl *ctl, const vshCmd *cmd) data[i].ctl = ctl; data[i].loop = loop; data[i].count = &count; + data[i].timestamp = timestamp; data[i].cb = &vshEventCallbacks[i]; data[i].id = -1; } @@ -12589,6 +12605,7 @@ cmdEvent(vshControl *ctl, const vshCmd *cmd) data[0].ctl = ctl; data[0].loop = vshCommandOptBool(cmd, "loop"); data[0].count = &count; + data[0].timestamp = timestamp; data[0].cb = &vshEventCallbacks[event]; data[0].id = -1; } -- 2.6.4

On Mon, Dec 21, 2015 at 16:25:56 +0100, Andrea Bolognani wrote: The main reason for adding the timestamp was to be able to look at the output and see, e.g., that a few events came very close to each other, then there was a 30s delay before another event came. I don't think getting the exact timing from libvirtd would be useful for anything. It's not just about the callback signature, each event would have to provide the timestamp... IMHO it's not worth the effort at all. qemu-monitor-event is designed to provide more-or-less raw data from QEMU. Personally, I'd leave it alone, but I don't feel strongly either way :-) The main reason for doing it this way was laziness (what a surprise!). Jirka

virshCatchDisconnect expects ctl, but we were just passing NULL instead. Signed-off-by: Jiri Denemark <jdenemar(a)redhat.com&gt; --- tools/virsh.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index 7484bed..2b53bf7 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -207,7 +207,7 @@ virshReconnect(vshControl *ctl) vshError(ctl, "%s", _("failed to connect to the hypervisor")); } else { if (virConnectRegisterCloseCallback(priv->conn, virshCatchDisconnect, - NULL, NULL) < 0) + ctl, NULL) < 0) vshError(ctl, "%s", _("Unable to register disconnect callback")); if (connected) vshError(ctl, "%s", _("Reconnected to the hypervisor")); @@ -294,7 +294,7 @@ cmdConnect(vshControl *ctl, const vshCmd *cmd) } if (virConnectRegisterCloseCallback(priv->conn, virshCatchDisconnect, - NULL, NULL) < 0) + ctl, NULL) < 0) vshError(ctl, "%s", _("Unable to register disconnect callback")); return true; -- 2.6.4

The *event --loop commands would keep running even though a connection to libvirtd is lost. This doesn't make a lot of sense since clearly we won't get any new events from the closed connection. Signed-off-by: Jiri Denemark <jdenemar(a)redhat.com&gt; --- tools/virsh.c | 1 + 1 file changed, 1 insertion(+) diff --git a/tools/virsh.c b/tools/virsh.c index 2b53bf7..1a9a713 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -130,6 +130,7 @@ virshCatchDisconnect(virConnectPtr conn, virFreeError(error); } disconnected++; + vshEventDone(ctl); } } -- 2.6.4