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

Sat Jul 7 12:56:50 UTC 2018

On 07/06/2018 05:11 AM, Marc Hartmayer wrote:
> 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?
>>

Perhaps callback is the wrong term here - still the relationship between
Serialize and Deserialize and expectations or long standing assumptions
of @nparams/@params is of great concern. Chasing each possible caller is
time consuming.

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

Well, good question.  The virTypedParamsDeserialize function comments
give me enough pause to wonder. When I looked at a couple of paths there
were checks that nparams == 0 prior to even getting to the Serializer
call (using cscope egrep on "nparams == 0" in various remote_*.c
callers, but there are a couple that get all the way to the driver
before returning some constant value (qemu_driver does this for
BlockIoTune and BlockStats and others for GetCPUStats).  I didn't dig
beyond that.

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

I think best to send something that's updated and go from there.
Hopefully we can get more engagement from others on the team.

John

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