Re: [libvirt] [PATCHv2 1/5] lib: Introduce API for retrieving bulk domain stats
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
Attachments:
- signature.asc (application/pgp-signature — 539 bytes)