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

[Daniel P. Berrange]

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) {


Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|