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

[John Ferlan]

On 07/11/2013 10:33 AM, Daniel P. Berrange wrote:
> 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.
> 
> 
Oh yeah - added that, thanks.

>> + * 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
> 

Yep

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

Ah - right! Must've been sleeping when I used the && and not a if
(!vm->def->memballoon...)

John