[libvirt] [PATCH 1/8] qemu: bulk stats: extend internal collection API

[Peter Krempa]

On 09/09/14 18:04, Francesco Romani wrote:
> 
> 
> ----- Original Message -----
>> From: "Peter Krempa" <pkrempa at redhat.com>
>> To: "Francesco Romani" <fromani at redhat.com>, libvir-list at redhat.com
>> Sent: Tuesday, September 9, 2014 2:14:23 PM
>> Subject: Re: [libvirt] [PATCH 1/8] qemu: bulk stats: extend internal collection API
>>
>> On 09/08/14 15:05, Francesco Romani wrote:
>>> Future patches which will implement more
>>> bulk stats groups for QEMU will need to access
>>> the connection object.
>>>
>>> To accomodate that, a few changes are needed:
>>>
>>> * enrich internal prototype to pass connection object.
>>> * add per-group flag to mark if one collector needs
>>>   monitor access or not.
>>> * if at least one collector of the requested stats
>>>   needs monitor access, thus we must start a query job
>>>   for each domain. The specific collectors will
>>>   run nested monitor jobs inside that.
>>>
>>> Signed-off-by: Francesco Romani <fromani at redhat.com>
>>> ---
>>>  src/qemu/qemu_driver.c | 51
>>>  ++++++++++++++++++++++++++++++++++++++++++--------
>>>  1 file changed, 43 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>>> index d724eeb..2950a4b 100644
>>> --- a/src/qemu/qemu_driver.c
>>> +++ b/src/qemu/qemu_driver.c
>>
>>> @@ -17338,7 +17339,8 @@ qemuDomainGetStatsState(virDomainObjPtr dom,
>>>  
>>>  
>>>  typedef int
>>> -(*qemuDomainGetStatsFunc)(virDomainObjPtr dom,
>>> +(*qemuDomainGetStatsFunc)(virConnectPtr conn,
>>
>> Looking through the rest of the series. Rather than the complete
>> connection object you need just the virQEMUDriverPtr for entering the
>> monitor, but I can live with this.
> 
> Since I need to resubmit to address your comments, I'll fix this
> to pass just virQEMUDriverPtr.
>  
>>>  
>>> -        if (qemuDomainGetStats(conn, dom, stats, &tmp, flags) < 0)
>>> +        if (needmon && qemuDomainObjBeginJob(driver, dom, QEMU_JOB_QUERY)
>>> < 0)
>>>              goto cleanup;
>>>  
>>> -        if (tmp)
>>> -            tmpstats[nstats++] = tmp;
>>> +        if ((needmon && virDomainObjIsActive(dom)) || !needmon) {
>>
>> Hmm this skips offline domains entirely if one of the stats groups needs
>> the monitor.
>>
>> I think we should rather skip individual stats groups, or better stats
>> fields that we can't provide.
>>
>> Any ideas?
> 
> What about this (pseudo-C):
> 
>     unsigned int privflags = 0;
> 
>     if (needmon && qemuDomainObjBeginJob(driver, dom, QEMU_JOB_QUERY)
>         needmon = false;
>         /* auto disable monitoring, the remainder of the function should be unchanged */
>     else
>         privflags |= MONITOR_AVAILABLE;
>  
>     if ((needmon && virDomainObjIsActive(dom)) || !needmon) {
>         if (qemuDomainGetStats(conn, dom, stats, &tmp, privflags) < 0)
>             /* pass monitor availability down the chain. Individual workers will
>                bail out immediately and silently if they need monitor but it is
>                not available
>              */
>             goto endjob;
> 
>         if (tmp)
>             tmpstats[nstats++] = tmp;
>     }
> 
> 
> No other change should be needed to this patch, and with trivial changes
> all the others can be fixed.

Also you can just grab the lock always and the workers will exit if the
VM is not alive. Having a domain job should be fine.

> 
> 

Peter


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20140910/b16ee121/attachment-0001.sig>