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

On Tue, Jul 03, 2018 at 09:14 PM +0200, John Ferlan <jferlan(a)redhat.com&gt; wrote: > On 07/03/2018 06:32 AM, Marc Hartmayer wrote: >> On Thu, Jun 21, 2018 at 03:47 PM +0200, Marc Hartmayer <mhartmay(a)linux.ibm.com&gt; wrote: >>> On Wed, Jun 13, 2018 at 03:11 PM +0200, John Ferlan <jferlan(a)redhat.com&gt; wrote: >>>> On 06/07/2018 08:17 AM, Marc Hartmayer wrote: >>>>> On Wed, May 09, 2018 at 09:51 PM +0200, John Ferlan <jferlan(a)redhat.com&gt; wrote: >>>>>> On 05/07/2018 11:24 AM, Marc Hartmayer wrote: >>>>>>> On Mon, Apr 30, 2018 at 03:20 PM +0200, John Ferlan <jferlan(a)redhat.com&gt; 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