[libvirt] [PATCH 1/2] virTypedParamsSerialize: minor fixes

[Marc Hartmayer]

On Wed, Jun 13, 2018 at 03:11 PM +0200, John Ferlan <jferlan at redhat.com> wrote:
> On 06/07/2018 08:17 AM, Marc Hartmayer wrote:
>> On Wed, May 09, 2018 at 09:51 PM +0200, John Ferlan <jferlan at redhat.com> wrote:
>>> On 05/07/2018 11:24 AM, Marc Hartmayer wrote:
>>>> On Mon, Apr 30, 2018 at 03:20 PM +0200, John Ferlan <jferlan at redhat.com> wrote:
>>>>> On 04/25/2018 11:55 AM, Marc Hartmayer wrote:
>>>>>> 1. Don't allocate if there is nothing that needs to be
>>>>>>    allocated. Especially as the result of calling calloc(0, ...) is
>>>>>>    implementation-defined.
>>>
>>> uh oh - another memory recollection challenge ;-)
>>>
>>>>>
>>>>> Following VIR_ALLOC_N one finds :
>>>>>
>>>>> VIR_ALLOC_N(params_val, nparams)
>>>>>
>>>>> which equates to
>>>>>
>>>>> # define VIR_ALLOC_N(ptr, count) \
>>>>>          virAllocN(&(ptr), sizeof(*(ptr)), (count), true, \
>>>>>                    VIR_FROM_THIS, __FILE__, __FUNCTION__, __LINE__)
>>>>>
>>>>> or
>>>>>
>>>>> virAllocN(&params_val, sizeof(params_val), nparams, true, ...)
>>>>>
>>>>> and eventually/essentially
>>>>>
>>>>> *params_val = calloc(sizeof(params_val), nparams)
>>>>>
>>>>> If the returned value is NULL then we error w/ OOM (4th param=true).
>>>>>
>>>>> So, unless @params_val had no elements, it won't be calloc(0,...) and
>>>>
>>>> I'm talking about the case that nparams == 0 => calloc(sizeof(…), 0).
>>>>
>>>
>>> And I was thinking is there a specific consumer/caller of
>>> virTypedParamsSerialize that was passing something incorrectly.
>>>
>>>>> thus the question becomes is there a more specific path you are
>>>>> referencing here?
>>>>
>>>> It’s a “generic” serializer so it should work for every (valid)
>>>> case. What happens, for example, if params == NULL and nparams == 0? In
>>>> that case I would expect *remote_params_val = NULL and
>>>> *remote_params_len = 0.
>>>>
>>>
>>> Going through the callers to virTypedParamsSerialize can/does anyone
>>> pass params == NULL and nparams == 0?  Would that be reasonable and/or
>>> expected?
>>>
>>> My look didn't find any - either some caller checks that the passed
>>> @params != NULL (usually via virCheckNonNullArgGoto in the external API
>>> call) or @params is built up on the fly and wouldn't be NULL because
>>> there'd be an error causing failure beforehand. Although I'll admit the
>>> migration ones are also crazy to look at.
>>>
>>> I could have made a mistake too; hence, the is there a specific instance
>>> that I missed? Or perhaps this is a result of some branched/private code
>>> that had that error which I don't have access to?
>>
>> It was the result of private code but actually it was intended and no
>> error :)
>>
>>>
>>> To answer your "What happens, for example, if params == NULL and nparams
>>> == 0?", well supposedly "VIR_ALLOC_N(params_val, 0)" should return NULL,
>>> so params_val and thus *remote_params_val == NULL.
>>
>> Did you try what 'VIR_ALLOC_N(params_val, 0)' actually returns? At least
>> for me, it doesn’t return a NULL pointer.
>>
>
> I think I already determined that NULL isn't returned; otherwise, we
> would have failed w/ OOM. I do recall trying this and seeing a non NULL
> value returned.
>
> IIRC: I found no way into [de]serialize w/ a 0 parameter, so having that
> guard didn't seem necessary,

I used the libvirt-python APIs for some testing. I called an (own) API
with 'None' as argument and this resulted in 0 parameters.

> but it's been 2 months since I looked at
> this and that level of detail has long been flushed out of main memory
> cache. ;-)
>
>> The man page for calloc-posix reads:
>>
>> “If either nelem or elsize is 0, then either a null pointer or a unique
>> pointer value that can be successfully passed to free() shall be
>> returned.” [1]
>>
>> [1] https://www.unix.com/man-page/POSIX/3posix/calloc/
>>
>
> perhaps then we avoid this conundrum and take Daniel's advice in his
> response:
>
>    "If there is any problem with this, then we should just change
> bootstrap.conf to use calloc-gnu instead of calloc-posix, which
> basically turns calloc(0) into calloc(1) for compat with glibc
> behaviour."

This would still require this patch, no? At least if we agree that the
following example should work:

virTypedParameterPtr params;
int nparams;

virTypedParamsDeserialize(NULL, 0, &params, &nparams);
assert(params == NULL && assert nparams == 0);

Do we?

[…snip]

--
Beste Grüße / Kind regards
   Marc Hartmayer

IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294