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

[Daniel Veillard]

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

-- 
Daniel Veillard      | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
daniel at veillard.com  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/