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

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.