On Mon, Nov 18, 2019 at 01:50:03PM +0000, Daniel P. Berrangé wrote: > On Mon, Nov 18, 2019 at 02:26:31PM +0100, Erik Skultety wrote: > > On Mon, Nov 18, 2019 at 01:04:01PM +0000, Daniel P. Berrangé wrote: > > > On Mon, Nov 18, 2019 at 01:18:19PM +0100, Erik Skultety wrote: > > > > It doesn't make sense to pass a target buffer into an API, declaring its > > > > size as 0 and expect some meaningful result. Since this used to work > > > > pre-Glib era, we shouldn't end with an error, but we can return 0 > > > > for the number of domains immediately, instead of calling into the > > > > daemon, which is exactly what the daemon would have returned anyway. > > > > > > Passing in size as 0 is going to be normal practice, given the calling > > > convention of this API design. > > > > > > > > > > > Signed-off-by: Erik Skultety <eskultet@xxxxxxxxxx> > > > > --- > > > > src/libvirt-domain.c | 3 +++ > > > > 1 file changed, 3 insertions(+) > > > > > > > > diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c > > > > index 02622cb2ca..0def40fdf7 100644 > > > > --- a/src/libvirt-domain.c > > > > +++ b/src/libvirt-domain.c > > > > @@ -62,6 +62,9 @@ virConnectListDomains(virConnectPtr conn, int *ids, int maxids) > > > > virCheckNonNullArgGoto(ids, error); > > > > virCheckNonNegativeArgGoto(maxids, error); > > > > > > > > + if (maxids == 0) > > > > + return 0; > > > > > > This is too late really, as we alrady checked 'ids'. > > > > Why is ^this a problem? The pointer has to be allocated prior to calling into > > the API, so a failure on a NULL pointer on client-side is fine. On server side, > > the issue is remediated much earlier in the RPC dispatch code. > > The current regression is caused server-side, but I'm saying the > problem in general is pre-existing even client side. Chances are > that people have already had to work around this when calling it > client side. > > If we fix this though, we should fix it so that we aovid the > problem both client & server side. > > > IOW, rather than taking the approach to avoiding calling the > API when num==0, make it valid to do the call, so that our > public API's behaviour isn't dependant on whether the client's > malloc() returns NULL or not for a zero sized allocation. I could move the same check a few lines earlier above the virCheckNonNullArgGoto check, but that doesn't go with your idea of allowing the client to make the RPC call. Personally, I find it rather pointless to make an RPC call if we can deterministically tell that the result is going to be 0, effectively a NOP. Erik -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
