Re: [libvirt] [PATCH v3 05/10] Determine whether to start balloon memory stats gathering.

On 07/12/2013 08:39 AM, Daniel P. Berrange wrote: > On Thu, Jul 11, 2013 at 07:55:55PM -0400, John Ferlan wrote: >> At vm startup and attach attempt to set the balloon driver statistics >> collection period based on the value found in the domain xml file. This >> is not done at reconnect since it's possible that a collection period >> was set on the live guest and making the set period call would reset to >> whatever value is stored in the config file. >> >> Setting the stats collection period has a side effect of searching through >> the qom-list output for the virtio balloon driver and making sure that it >> has the right properties in order to allow setting of a collection period >> and eventually fetching of statistics. >> >> The walk through the qom-list is expensive and thus the balloonpath will >> be saved in the monitor private structure as well as a flag indicating >> that the initialization has already been attempted (in the event that a >> path is not found, no sense to keep checking). >> >> This processing model conforms to the qom object model model which >> requires setting object properties after device startup. That is, it's >> not possible to pass the period along via the startup code as it won't >> be recognized. >> --- >> src/qemu/qemu_monitor.c | 130 +++++++++++++++++++++++++++++++++++++++++++ >> src/qemu/qemu_monitor.h | 2 + >> src/qemu/qemu_monitor_json.c | 24 ++++++++ >> src/qemu/qemu_monitor_json.h | 3 + >> src/qemu/qemu_process.c | 14 ++++- >> 5 files changed, 171 insertions(+), 2 deletions(-) >> >> diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c >> index 6f9a8fc..a3e250c 100644 >> --- a/src/qemu/qemu_monitor.c >> +++ b/src/qemu/qemu_monitor.c >> @@ -83,6 +83,10 @@ struct _qemuMonitor { >> >> /* cache of query-command-line-options results */ >> virJSONValuePtr options; >> + >> + /* If found, path to the virtio memballoon driver */ >> + char *balloonpath; >> + bool ballooninit; >> }; >> >> static virClassPtr qemuMonitorClass; >> @@ -248,6 +252,7 @@ static void qemuMonitorDispose(void *obj) >> virCondDestroy(&mon->notify); >> VIR_FREE(mon->buffer); >> virJSONValueFree(mon->options); >> + VIR_FREE(mon->balloonpath); >> } >> >> >> @@ -925,6 +930,105 @@ qemuMonitorSetOptions(qemuMonitorPtr mon, virJSONValuePtr options) >> mon->options = options; >> } >> >> +/* Search the qom objects for the balloon driver object by it's known name >> + * of "virtio-balloon-pci". The entry for the driver will be found in the >> + * returned 'type' field using the syntax "child<virtio-balloon-pci>". >> + * >> + * Once found, check the entry to ensure it has the correct property listed. >> + * If it does not, then obtaining statistics from qemu will not be possible. >> + * This feature was added to qemu 1.5. >> + * >> + * This procedure will be call recursively until found or the qom-list is >> + * exhausted. >> + * >> + * Returns: >> + * >> + * 1 - Found >> + * 0 - Not found still looking >> + * -1 - Error bail out >> + * >> + * NOTE: This assumes we have already called qemuDomainObjEnterMonitor() >> + */ >> +static int >> +qemuMonitorFindBalloonObjectPath(qemuMonitorPtr mon, >> + virDomainObjPtr vm, >> + const char *curpath) >> +{ >> + size_t i, j, npaths = 0, nprops = 0; >> + int ret = 0; >> + char *nextpath = NULL; >> + qemuMonitorJSONListPathPtr *paths = NULL; >> + qemuMonitorJSONListPathPtr *bprops = NULL; >> + >> + /* Already set and won't change or we already search and failed to find */ >> + if (mon->balloonpath || mon->ballooninit) >> + return 1; > > This isn't correct logic. You need > > if (mon->balloonpath) { > return 1; > } else if (mon->ballooninit) { > virReportError(VIR_ERR_INTERNAL_ERROR, "%s" > _("Cannot determine balloon device path")); > return -1; > } > This is a recursive function - setting ballooninit in here means I have to know when I'm back at the first call. That's why it's set in the callers. Since ballooninit means I've either made the call and was successful or I've made the call and wasn't successful. I think an argument could be made that the checking of balloonpath probably is extraneous. Since the balloon driver stats are only supported in qemu 1.5 and beyond, do we really want an error message? >> + >> + /* Not supported */ >> + if (!vm->def->memballoon || >> + vm->def->memballoon->model != VIR_DOMAIN_MEMBALLOON_MODEL_VIRTIO) { >> + VIR_DEBUG("Model must be virtio to get memballoon path"); > > You need to use virReportError here, so the caller sees an error > messages. > hmm... Do we really want to virReportError() in either case? They're not necessarily specifically asking for the statistics here... Although with your suggestion to only call SetMemoryStatsPeriod if the period is defined, I do see the point. Although the first time someone does a 'virsh dommemstats domain' they'd see this message and perhaps wonder. Also if the user didn't have the balloon driver configured, then this would be spit out.