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