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

Wed Jul 4 09:24:56 UTC 2018

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.

>
> 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