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

[Matthias Bolte]

2011/5/18 Daniel Veillard <veillard at redhat.com>:
> 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