[libvirt] [PATCH 1/2] qemu: implement block group for bulk stats.

[Li Wei]

On 09/03/2014 07:19 AM, Eric Blake wrote:
> On 08/29/2014 01:56 AM, Li Wei wrote:
>> This patch add the block group for bulk stats.
>> The following typed parameter used for each block stats:
>>     block.count - number of block devices in this domain
>>     block.0.name - name of the block device
>>     block.0.rd_bytes - number of read bytes
>>     block.0.rd_operations - number of read requests
>>     block.0.rd_total_time -  total time spend on cache reads in nano-seconds
>>     block.0.wr_bytes - number of write bytes
>>     block.0.wr_operations - number of write requests
>>     block.0.wr_total_time - total time spend on cache write in nano-seconds
>>     block.0.flush_operations - total flush requests
>>     block.0.flush_total_time - total time spend on cache flushing in nano-seconds
>>
>> Signed-off-by: Li Wei <lw at cn.fujitsu.com>
>> ---
> 
>> +++ b/src/libvirt.c
>> @@ -21632,6 +21632,19 @@ virConnectGetAllDomainStats(virConnectPtr conn,
>>   * "state.reason" - reason for entering given state, returned as int from
>>   *                  virDomain*Reason enum corresponding to given state.
>>   *
>> + * VIR_DOMAIN_STATS_BLOCK: Return block device stats. The typed parameter keys
>> + * are in this format:
>> + * "block.count" - number of block devices in this domain
>> + * "block.0.name" - name of the block device
>> + * "block.0.rd_bytes" - number of read bytes
>> + * "block.0.rd_operations" - number of read requests
>> + * "block.0.rd_total_time" -  total time spend on cache reads in nano-seconds
>> + * "block.0.wr_bytes" - number of write bytes
>> + * "block.0.wr_operations" - number of write requests
>> + * "block.0.wr_total_time" - total time spend on cache write in nano-seconds
>> + * "block.0.flush_operations" - total flush requests
>> + * "block.0.flush_total_time" - total time spend on cache flushing in nano-seconds
> 
> s/nano-seconds/nanoseconds/
> Missing expected units (such as unsigned long long)
> 
> Looks like Francesco has taken this patch and modified it into a newer
> version.

Thanks for telling me this, I'll discard this patch and focus on
reviewing Francesco's one.

Thanks.

> 
>>  
>> +static int
>> +qemuDomainGetStatsBlock(virDomainObjPtr dom,
>> +                        virDomainStatsRecordPtr record,
>> +                        int *maxparams,
>> +                        unsigned int flags)
>> +{
>> +    int ret;
>> +    qemuDomainObjPrivatePtr priv = dom->privateData;
>> +
>> +    /* only valid for active domain, ignore inactive ones silently */
>> +    if (!virDomainObjIsActive(dom))
>> +        return 0;
>> +
>> +    if (qemuDomainObjBeginJob(qemu_driver, dom, QEMU_JOB_QUERY) < 0)
>> +        return -1;
>> +
>> +    qemuDomainObjEnterMonitor(qemu_driver, dom);
> 
> Data race. You cannot safely call qemuDomainObjEnterMonitor unless you
> have checked that the domain is active _after_ qemuDomainObjBeginJob.
> Checking for an active domain prior to beginning the job doesn't hurt,
> but then you have to repeat the check, so it is easier to just swap the
> two instead of checking twice.
>