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

On Mon, Jul 08, 2013 at 03:20:31PM -0400, John Ferlan wrote: > At vm startup, reconnect, and attach - check for the presence of the balloon > driver and save the path in the private area of the driver. This path will > remain constant throughout the life of the domain and can then be used rather > than attempting to find the path each time balloon driver statistics are > fetched or the collection period changes. The qom object model model 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. > If a balloon driver path is found a check of the existing collection period > will be made against the saved domain value in order to determine if an > adjustment needs to be made to the period to start or stop collecting stats > --- > src/qemu/qemu_monitor.c | 130 +++++++++++++++++++++++++++++++++++++++++++ > src/qemu/qemu_monitor.h | 2 + > src/qemu/qemu_monitor_json.c | 42 ++++++++++++++ > src/qemu/qemu_monitor_json.h | 3 + > src/qemu/qemu_process.c | 21 ++++++- > 5 files changed, 196 insertions(+), 2 deletions(-) > > diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c > index fe5ab0a..038c9e8 100644 > --- a/src/qemu/qemu_monitor.c > +++ b/src/qemu/qemu_monitor.c > @@ -83,6 +83,9 @@ struct _qemuMonitor { > > /* cache of query-command-line-options results */ > virJSONValuePtr options; > + > + /* If found, path to the virtio memballoon driver */ > + char *balloonpath; Hmm, we can't distinguish between "not searched yet" and "searched, but didn't find one". This means in the latter case we'll repeatedly search every time. Not too bad, but a little wasteful. Perhaps add a 'bool ballooninit' to track whether we've searched yet.

> + * 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) > +{ > + int i,j; size_t now

> + int npaths = 0; > + int nprops = 0; > + int ret = 0; > + char *nextpath = NULL; > + qemuMonitorJSONListPathPtr *paths = NULL; > + qemuMonitorJSONListPathPtr *bprops = NULL; > + > + /* 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"); > + return -1; > + } What about if 'vm->def->memballoon == NULL' ? Shouldn't we return -1 in that case too ? eg this condition should actually be if (vm->def->memballoon == NULL || vm->def->memballoon->model != VIR_DOMAIN_MEMBALLOON_MODEL_VIRTIO) {