On 09/09/14 18:04, Francesco Romani wrote:
>
>
> ----- Original Message -----
>> From: "Peter Krempa" <pkrempa@xxxxxxxxxx>
>> To: "Francesco Romani" <fromani@xxxxxxxxxx>, libvir-list@xxxxxxxxxx
>> Sent: Tuesday, September 9, 2014 2:14:23 PM
>> Subject: Re: [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@xxxxxxxxxx>
>>> ---
>>> 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
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list