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

2011/5/18 Daniel Veillard <veillard(a)redhat.com&gt;: > 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 I'm working on a series to clean this up and go for the stricter semantic of the Get*Parameters functions. Matthias