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

Fri Jul 6 09:11:21 UTC 2018

On Wed, Jul 04, 2018 at 11:24 AM +0200, Marc Hartmayer <mhartmay at linux.ibm.com> wrote:
> On Tue, Jul 03, 2018 at 09:14 PM +0200, John Ferlan <jferlan at redhat.com> wrote:
>> On 07/03/2018 06:32 AM, Marc Hartmayer wrote:
>>> On Thu, Jun 21, 2018 at 03:47 PM +0200, Marc Hartmayer <mhartmay at linux.ibm.com> wrote:
>>>> 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:
>>>
>>> Okay, the example wouldn’t even compile… but I hope the overall message
>>> is clear :)
>>>
>>>>
>>>> virTypedParameterPtr params;
>>>> int nparams;
>>>>
>>>> virTypedParamsDeserialize(NULL, 0, &params, &nparams);
>>>> assert(params == NULL && assert nparams == 0);
>>>>
>>>> Do we?
>>>>
>>>
>>> Polite ping.
>>>
>>> […ping]
>>>
>>
>> I hope I can politely say I've completely lost context of all of this in
>> my short term memory. :-).
>
> Yep, understand that :)
>
>>
>> Short answer, not sure. At least not without patch 2 of this series and
>> without checking each called non-remote driver callback function to
>> ensure it handles both 0 nparams and NULL params properly before calling
>> Deserialize.
>
> Which callback functions do you mean?
>
>>
>> Still the Deserialize side is documented to handle 2 modes of operation
>> - 1 a two pass model to get the nparams and then call again with a
>> preallocated params buffer and 2 relying on the deserializer for the
>> allocation.
>
> Can you please give me an example where the deserializer is called
> twice? And what is meant with “deserializer”? The function
> virTypedParamsDeserialize?
>
> For qemuDomainGetBlkioParameters/remoteDomainGetBlkioParameters, for
> example, not virTypedParamsDeserialize returns nparams - instead it’s
> hardcoded in qemuDomainGetBlkioParameters.
>
>>
>> "So far" at least things have been well behaved and I guess I'm still
>> not clear what problem is being chased from "existing" code. You
>> mentioned having "own" code, so perhaps that's where existing
>> assumptions haven't held true.
>>
>> In any case, from my perspective making changes here involves weighing
>> the risks of "fear" over changing algorithms that work and have made
>> some assumption for years against the possible advantage of just not
>> calloc'ing something for the nparams == 0 case.
>
> Hmm, makes sense.

Actually, it’s turned out there is/was a problem… virTypedParamsFilter
validates that params must be NON-NULL (IMHO, params == NULL is valid if
nparams == 0, but as you said, it’s not safe, since all other code paths
might assume other things).

Therefore, it is best to leave it as it is for nparams == 0. But then of
course we have to change it in the bootstrap.conf to calloc-gnu (as
suggested by Daniel)!

However, the rest of the patch should still be valid (the assignment of
*remote_params_len only at the end). If you want to I can resend an
appropriate version.

Note: The same applies to the second patch.

Thank you.

>
>>
>> FWIW: based on subsequent discussion at the very least the commit
>> message would need to be adjusted to indicate calloc(sizeof(...), 0)
>> instead of calloc(0, ...). I think it's been shown that it's not the
>> latter in this case.
>
> Okay.
>
>>
>> John
>>
> --
> 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
--
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