[libvirt] [PATCH 3/3] libvirt-domain: virConnectListDomains: Return 0 on zero-size buffer

[Erik Skultety]

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 at redhat.com>
> > > > ---
> > > >  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