[libvirt] [PATCHv2 1/5] lib: Introduce API for retrieving bulk domain stats

[Eric Blake]

On 08/26/2014 11:28 AM, Peter Krempa wrote:
> On 08/26/14 18:49, Eric Blake wrote:
>> On 08/26/2014 08:14 AM, Peter Krempa wrote:
> 
> ....
> 
>>
>> Would it make sense for @flags to provide the same filtering as
>> virConnectListAllDomains(), for selecting a subset of domains to get
>> stats on?  But can definitely be added later.
> 
> I already thought about implementing the filtering capability for this
> function although I doubt that all of the filtering flags may be useful
> here.
> 
> Requesting stats for a machine that has (or doesn't have) snapshot
> doesn't seem useful at all.

True, not all of the listing filters make as much sense here.  At any
rate, adding filtering as a later extension is a fine approach; and we
can always play it by ear according to demand.


>>
>> virCheckNonNullArg(retStats)?  Matters depending on whether you plan to
>> allow NULL retStats across RPC.
> 
> Yep we always want to have retStats. I forgot to add the check.

Okay, I did my review of 2/5 before seeing this.  But there were still
some things to clean up there as a result of enforcing this requirement.

>>> +    virResetLastError();
>>> +
>>> +    if (!*doms) {
>>
>> Missing virCheckNonNullArgReturn(doms, -1) prior to dereferencing *doms.
>>  (I would have suggested virCheckNonNullArgGoto(doms, error), except
>> that the error label only works if we have validated the connection).
>>
>>> +        virReportError(VIR_ERR_INVALID_ARG,
>>> +                       _("doms array in %s must contain at least one domain"),
>>> +                       __FUNCTION__);
>>> +        goto cleanup;
>>
>> Ouch.  You can't use the cleanup label unless you know the conn is
>> valid, but you don't validate the conn until...
>>
> 
> Actually you can. The function that validates "conn" calls the same
> error dispatching function with NULL argument. Exactly what will happen
> here.

Ah, I was getting confused with our typical uses, which look somewhat like:

 cleanup:
    if (ret < 0)
        virDispatchError(dom->conn);

where we CANNOT dereference dom if it didn't validate; but in your case,
you made sure the local 'conn' is either NULL or grabbed from a
validated dom, and virDispatchError(NULL) does work.  Okay, false
negative on my side.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

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