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

[Matthias Bolte]

2011/5/18 Matthias Bolte <matthias.bolte at googlemail.com>:
> 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
>

The initial patch has been superseded by this series:

https://www.redhat.com/archives/libvir-list/2011-May/msg01194.html

Matthias