Re: [libvirt] [PATCH v3 01/11] qemu: Add support for DUMP_COMPLETED event

On Mon, Jan 29, 2018 at 11:31:59 -0500, John Ferlan wrote: > The event will be fired when the domain memory only dump completes. > > Alloc a return buffer to store/pass along the dump statistics that > will be eventually shared by a query-dump command. > > Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; > --- > src/qemu/qemu_monitor.c | 29 ++++++++++++++++++++ > src/qemu/qemu_monitor.h | 31 +++++++++++++++++++++ > src/qemu/qemu_monitor_json.c | 65 ++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 125 insertions(+) > > diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c > index fc146bdbf..23153967c 100644 > --- a/src/qemu/qemu_monitor.c > +++ b/src/qemu/qemu_monitor.c > @@ -210,6 +210,10 @@ VIR_ENUM_IMPL(qemuMonitorBlockIOStatus, > QEMU_MONITOR_BLOCK_IO_STATUS_LAST, > "ok", "failed", "nospace") > > +VIR_ENUM_IMPL(qemuMonitorDumpStatus, > + QEMU_MONITOR_DUMP_STATUS_LAST, > + "none", "active", "completed", "failed") > + > char * > qemuMonitorEscapeArg(const char *in) > { > @@ -1667,6 +1671,21 @@ qemuMonitorEmitBlockThreshold(qemuMonitorPtr mon, > > > int > +qemuMonitorEmitDumpCompleted(qemuMonitorPtr mon, > + qemuMonitorDumpStatsPtr stats, > + const char *error) > +{ > + int ret = -1; > + > + VIR_DEBUG("mon=%p", mon); > + > + QEMU_MONITOR_CALLBACK(mon, ret, domainDumpCompleted, mon->vm, stats, error); > + > + return ret; > +} > + > + > +int > qemuMonitorSetCapabilities(qemuMonitorPtr mon) > { > QEMU_CHECK_MONITOR(mon); > @@ -4359,3 +4378,13 @@ qemuMonitorSetWatchdogAction(qemuMonitorPtr mon, > > return qemuMonitorJSONSetWatchdogAction(mon, action); > } > + > + > +void > +qemuMonitorEventDumpStatsFree(qemuMonitorDumpStatsPtr stats) > +{ > + if (!stats) > + return; > + > + VIR_FREE(stats); > +} The caller can just use VIR_FREE() directly and benefit from it automatically resetting the pointer to NULL. Anyway, I don't think we need to ever free this structure.

> diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h > index 67b785e60..37f335e9f 100644 > --- a/src/qemu/qemu_monitor.h > +++ b/src/qemu/qemu_monitor.h > @@ -246,6 +246,30 @@ typedef int (*qemuMonitorDomainBlockThresholdCallback)(qemuMonitorPtr mon, > unsigned long long excess, > void *opaque); > > +typedef enum { > + QEMU_MONITOR_DUMP_STATUS_NONE, > + QEMU_MONITOR_DUMP_STATUS_ACTIVE, > + QEMU_MONITOR_DUMP_STATUS_COMPLETED, > + QEMU_MONITOR_DUMP_STATUS_FAILED, > + > + QEMU_MONITOR_DUMP_STATUS_LAST, > +} qemuMonitorDumpStatus; > + > +VIR_ENUM_DECL(qemuMonitorDumpStatus) > + > +typedef struct _qemuMonitorDumpStats qemuMonitorDumpStats; > +typedef qemuMonitorDumpStats *qemuMonitorDumpStatsPtr; > +struct _qemuMonitorDumpStats { > + int status; /* qemuMonitorDumpStatus */ > + unsigned long long completed; /* bytes written */ > + unsigned long long total; /* total bytes to be written */ > +}; > + > +typedef int (*qemuMonitorDomainDumpCompletedCallback)(qemuMonitorPtr mon, > + virDomainObjPtr vm, > + qemuMonitorDumpStatsPtr stats, > + const char *error, > + void *opaque); > > typedef struct _qemuMonitorCallbacks qemuMonitorCallbacks; > typedef qemuMonitorCallbacks *qemuMonitorCallbacksPtr; > @@ -279,6 +303,7 @@ struct _qemuMonitorCallbacks { > qemuMonitorDomainMigrationPassCallback domainMigrationPass; > qemuMonitorDomainAcpiOstInfoCallback domainAcpiOstInfo; > qemuMonitorDomainBlockThresholdCallback domainBlockThreshold; > + qemuMonitorDomainDumpCompletedCallback domainDumpCompleted; > }; > > char *qemuMonitorEscapeArg(const char *in); > @@ -408,6 +433,10 @@ int qemuMonitorEmitBlockThreshold(qemuMonitorPtr mon, > unsigned long long threshold, > unsigned long long excess); > > +int qemuMonitorEmitDumpCompleted(qemuMonitorPtr mon, > + qemuMonitorDumpStatsPtr stats, > + const char *error); > + > int qemuMonitorStartCPUs(qemuMonitorPtr mon, > virConnectPtr conn); > int qemuMonitorStopCPUs(qemuMonitorPtr mon); > @@ -1146,4 +1175,6 @@ virJSONValuePtr qemuMonitorQueryNamedBlockNodes(qemuMonitorPtr mon); > > int qemuMonitorSetWatchdogAction(qemuMonitorPtr mon, > const char *action); > + > +void qemuMonitorEventDumpStatsFree(qemuMonitorDumpStatsPtr stats); > #endif /* QEMU_MONITOR_H */ > diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c > index 5ddd85575..a8cb8ce6b 100644 > --- a/src/qemu/qemu_monitor_json.c > +++ b/src/qemu/qemu_monitor_json.c > @@ -90,6 +90,7 @@ static void qemuMonitorJSONHandleMigrationStatus(qemuMonitorPtr mon, virJSONValu > static void qemuMonitorJSONHandleMigrationPass(qemuMonitorPtr mon, virJSONValuePtr data); > static void qemuMonitorJSONHandleAcpiOstInfo(qemuMonitorPtr mon, virJSONValuePtr data); > static void qemuMonitorJSONHandleBlockThreshold(qemuMonitorPtr mon, virJSONValuePtr data); > +static void qemuMonitorJSONHandleDumpCompleted(qemuMonitorPtr mon, virJSONValuePtr data); > > typedef struct { > const char *type; > @@ -106,6 +107,7 @@ static qemuEventHandler eventHandlers[] = { > { "BLOCK_WRITE_THRESHOLD", qemuMonitorJSONHandleBlockThreshold, }, > { "DEVICE_DELETED", qemuMonitorJSONHandleDeviceDeleted, }, > { "DEVICE_TRAY_MOVED", qemuMonitorJSONHandleTrayChange, }, > + { "DUMP_COMPLETED", qemuMonitorJSONHandleDumpCompleted, }, > { "GUEST_PANICKED", qemuMonitorJSONHandleGuestPanic, }, > { "MIGRATION", qemuMonitorJSONHandleMigrationStatus, }, > { "MIGRATION_PASS", qemuMonitorJSONHandleMigrationPass, }, > @@ -1143,6 +1145,69 @@ qemuMonitorJSONHandleBlockThreshold(qemuMonitorPtr mon, virJSONValuePtr data) > } > > > +static qemuMonitorDumpStatsPtr > +qemuMonitorJSONExtractDumpStats(virJSONValuePtr result) > +{ > + qemuMonitorDumpStatsPtr ret; > + const char *statusstr; > + > + if (VIR_ALLOC(ret) < 0) > + return NULL; I think it would be better if this just worked on an existing reference to qemuMonitorDumpStats passed as an additional argument.

> + > + if (!(statusstr = virJSONValueObjectGetString(result, "status"))) { > + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > + _("incomplete result, failed to get status")); > + goto error; > + } > + > + ret->status = qemuMonitorDumpStatusTypeFromString(statusstr); > + if (ret->status < 0) { > + virReportError(VIR_ERR_INTERNAL_ERROR, > + _("incomplete result, unknown status string '%s'"), > + statusstr); > + goto error; > + } > + > + if (virJSONValueObjectGetNumberUlong(result, "completed", &ret->completed) < 0) { > + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > + _("incomplete result, failed to get completed")); > + goto error; > + } > + > + if (virJSONValueObjectGetNumberUlong(result, "total", &ret->total) < 0) { > + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > + _("incomplete result, failed to get total")); > + goto error; > + } > + > + return ret; > + > + error: > + qemuMonitorEventDumpStatsFree(ret); > + return NULL; > +} > + > + > +static void > +qemuMonitorJSONHandleDumpCompleted(qemuMonitorPtr mon, > + virJSONValuePtr data) > +{ > + virJSONValuePtr result; > + qemuMonitorDumpStatsPtr stats = NULL; > + const char *error = NULL; > + > + if (!(result = virJSONValueObjectGetObject(data, "result"))) { > + VIR_WARN("missing result in dump completed event"); > + return; > + } > + > + stats = qemuMonitorJSONExtractDumpStats(result); > + error = virJSONValueObjectGetString(data, "error"); > + > + qemuMonitorEmitDumpCompleted(mon, stats, error); Using stats allocated on the stack would resolve the memory leak here.