[libvirt] [PATCHv3 3/5] remote: Implement bulk domain stats APIs in the remote driver

[Peter Krempa]

On 08/27/14 23:02, Eric Blake wrote:
> On 08/27/2014 12:25 PM, Peter Krempa wrote:
>> Implement the remote driver support for shuffling the domain stats
>> around.
>> ---
> 
>> +static int
>> +remoteDispatchConnectGetAllDomainStats(virNetServerPtr server ATTRIBUTE_UNUSED,
>> +                                       virNetServerClientPtr client,
>> +                                       virNetMessagePtr msg ATTRIBUTE_UNUSED,
>> +                                       virNetMessageErrorPtr rerr,
>> +                                       remote_connect_get_all_domain_stats_args *args,
>> +                                       remote_connect_get_all_domain_stats_ret *ret)
>> +{
>> +    int rv = -1;
> 
>> +    if (nrecords) {
>> +        ret->retStats.retStats_len = nrecords;
>> +
>> +        if (VIR_ALLOC_N(ret->retStats.retStats_val, nrecords) < 0)
>> +            goto cleanup;
>> +
> 
> I'd switch these for safety (I don't know if the rpc cleanup code will
> try to dereference a NULL retStats.retStats_val if retStats_len is
> non-zero; while setting the length after the allocation should always be
> safe).
> 
>> +        for (i = 0; i < nrecords; i++) {
>> +            remote_domain_stats_record *dst = ret->retStats.retStats_val + i;
>> +
>> +            make_nonnull_domain(&dst->dom, retStats[i]->dom);
> 
> [Not your fault, and certainly not for this patch, but we REALLY ought
> to fix make_nonnull_domain and friends to properly return errors on OOM,
> instead of silently breaking things]
> 
>> +++ b/src/remote/remote_driver.c
>> @@ -7717,6 +7717,89 @@ remoteNetworkGetDHCPLeases(virNetworkPtr net,
>>  }
>>
>>
>> +static int
>> +remoteConnectGetAllDomainStats(virConnectPtr conn,
>> +                               virDomainPtr *doms,
>> +                               unsigned int ndoms,
>> +                               unsigned int stats,
>> +                               virDomainStatsRecordPtr **retStats,
>> +                               unsigned int flags)
>> +{
>> +    struct private_data *priv = conn->networkPrivateData;
>> +    int rv = -1;
>> +    size_t i;
>> +    remote_connect_get_all_domain_stats_args args;
>> +    remote_connect_get_all_domain_stats_ret ret;
>> +
>> +    virDomainStatsRecordPtr *tmpret = NULL;
>> +
>> +    if (ndoms) {
>> +        if (VIR_ALLOC_N(args.doms.doms_val, ndoms) < 0)
>> +            goto cleanup;
>> +
>> +        for (i = 0; i < ndoms; i++)
>> +            make_nonnull_domain(args.doms.doms_val + i, doms[i]);
> 
> [same complaint here about silent OOM bugs]
> 
> ACK with the statement reorder.
> 

I've fixed and pushed patches 1-3 and will follow up with v4 of the rest.

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/20140828/d4f20b75/attachment-0001.sig>