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
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list