Re: [libvirt] [PATCH] Improve invalid argument checks for the public API

On Tue, May 17, 2011 at 09:46:34AM -0600, Eric Blake wrote: > On 05/17/2011 07:14 AM, Daniel Veillard wrote: > > On Tue, May 17, 2011 at 02:45:44PM +0200, Matthias Bolte wrote: > >> --- > >>  src/libvirt.c |  100 +++++++++++++++++++++++++++++++++++++++++++++++++++++--- > >>  1 files changed, 94 insertions(+), 6 deletions(-) > > > >> @@ -5040,6 +5045,12 @@ virDomainGetSchedulerParameters(virDomainPtr domain, > >>          virDispatchError(NULL); > >>          return -1; > >>      } > >> + > >> +    if (params == NULL || nparams == NULL) { > >> +        virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__); > >> +        goto error; > >> +    } > > > >   I was just wondering here if params being NULL couldn't be used to > >   find out the correct value for nparams, but looking at least at the > >   qemu driver code we really expect the array there. > > Let's fix that as a separate patch. > > > > >>      conn = domain->conn; > >> > >>      if (conn->driver->domainGetSchedulerParameters) { > >> @@ -5084,6 +5095,12 @@ virDomainSetSchedulerParameters(virDomainPtr domain, > >>          virDispatchError(NULL); > >>          return -1; > >>      } > >> + > >> +    if (params == NULL) { > >> +        virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__); > >> +        goto error; > >> +    } > > Hmm, here we document that nparams can be <= the value returned by > virDomainGetSchedulerType; which means it can be 0, which means that > params can be NULL.  I think we should change this to allow NULL,0 in > input as a way of querying the proper nparams size on output. > > This affects Hu's patch series, where we are adding > virDomainSetSchedulerParametersFlags, which should have the same semantics. I was somehow remembering that we had let open the possibility to not provide the full array. Looking at the function comment which is the API contract there was nothing about it, so I looked at qemu backend and it fails if nparam != 1 , so at least that driver implemented the strict API.  We need to decide either way, possibly by looking first at all API entry points using that mechanism array pointer + array size, and see if we are likely to break any app if we make it strict. Or we could decide to allow 0 and NULL and in that case we need to document explicitely the semantic.  However for virDomainGetSchedulerParameters() it's relatively clear (well that could possibly be improved a bit) that the application must call virDomainGetSchedulerType() first to get nparams value, then allocate accordingly), so I would side on imposing the strict behaviour but make it a bit clearer. Daniel