[libvirt] [PATCH 0/4] virsh: Some events cleanups and improvements
Jiri Denemark (4): virsh: Refactor event printing virsh: Add timestamps to events virsh: Pass ctl to virshCatchDisconnect virsh: Interrupt *event --loop on disconnect tools/virsh-domain.c | 310 +++++++++++++++++++++++++-------------------------- tools/virsh.c | 5 +- 2 files changed, 156 insertions(+), 159 deletions(-) -- 2.6.4
To reduce code duplication. Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- 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(¶ms[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
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.
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.
@@ -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.
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); }
I can see a lot more places where UNKNOWNSTR() could be used. Out of scope for this commit, in any case. Cheers. -- Andrea Bolognani Software Engineer - Virtualization Team
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
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
A new --timestamp option for event virsh command can be used to print timestamp of each event. Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- 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, 2015-12-21 at 11:04 +0100, Jiri Denemark wrote:
@@ -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); + }
So the timestamp is not really for the moment the even occurred, rather the moment it was printed. I don't know if the difference is something we should really care about. The signature for virConnectDomainEventGenericCallback, unlike the one for virConnectDomainQemuMonitorEventCallback, does not provide timining information. Would it be a good idea to maybe have a new kind of callback for generic events that includes such information, instead of using the timestamp from when we printed the message? What about having qemu-monitor-event support --timestamp as well? I know the timing information are already part of the output, but we could use the same format used here (much more readable) and avoid duplicating them in the message. POC attached. Just bouncing back my ideas. You probably have very good reasons to be doing it this way :) Cheers. -- Andrea Bolognani Software Engineer - Virtualization Team
On Mon, Dec 21, 2015 at 16:25:56 +0100, Andrea Bolognani wrote:
On Mon, 2015-12-21 at 11:04 +0100, Jiri Denemark wrote:
@@ -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); + }
So the timestamp is not really for the moment the even occurred, rather the moment it was printed. I don't know if the difference is something we should really care about.
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.
The signature for virConnectDomainEventGenericCallback, unlike the one for virConnectDomainQemuMonitorEventCallback, does not provide timining information. Would it be a good idea to maybe have a new kind of callback for generic events that includes such information, instead of using the timestamp from when we printed the message?
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.
What about having qemu-monitor-event support --timestamp as well? I know the timing information are already part of the output, but we could use the same format used here (much more readable) and avoid duplicating them in the message. POC attached.
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
On Thu, 2016-01-07 at 22:23 +0100, Jiri Denemark wrote:
On Mon, 2015-12-21 at 11:04 +0100, Jiri Denemark wrote:
@@ -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); + } So the timestamp is not really for the moment the even occurred, rather the moment it was printed. I don't know if the difference is something we should really care about. 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,
The signature for virConnectDomainEventGenericCallback, unlike the one for virConnectDomainQemuMonitorEventCallback, does not provide timining information. Would it be a good idea to maybe have a new kind of callback for generic events that includes such information, instead of using the timestamp from when we printed the message? It's not just about the callback signature, each event would have to
On Mon, Dec 21, 2015 at 16:25:56 +0100, Andrea Bolognani wrote: 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. provide the timestamp... IMHO it's not worth the effort at all.
I see, let's just keep it as it is then.
What about having qemu-monitor-event support --timestamp as well? I know the timing information are already part of the output, but we could use the same format used here (much more readable) and avoid duplicating them in the message. POC attached. 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!).
Since you don't feel strongly either way, I'll just check my POC again and then send it out for review :) In the meantime, ACK on your patch. Cheers. -- Andrea Bolognani Software Engineer - Virtualization Team
virshCatchDisconnect expects ctl, but we were just passing NULL instead. Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- 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
On Mon, 2015-12-21 at 11:04 +0100, Jiri Denemark wrote:
virshCatchDisconnect expects ctl, but we were just passing NULL instead.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- 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;
ACK. -- Andrea Bolognani Software Engineer - Virtualization Team
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@redhat.com> --- 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
On Mon, 2015-12-21 at 11:04 +0100, Jiri Denemark wrote:
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@redhat.com> --- 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); } }
ACK. -- Andrea Bolognani Software Engineer - Virtualization Team
participants (2)
-
Andrea Bolognani -
Jiri Denemark