On 07/06/2018 05:11 AM, Marc Hartmayer wrote: > On Wed, Jul 04, 2018 at 11:24 AM +0200, Marc Hartmayer <mhartmay@xxxxxxxxxxxxx> wrote: >> On Tue, Jul 03, 2018 at 09:14 PM +0200, John Ferlan <jferlan@xxxxxxxxxx> wrote: >>> On 07/03/2018 06:32 AM, Marc Hartmayer wrote: >>>> On Thu, Jun 21, 2018 at 03:47 PM +0200, Marc Hartmayer <mhartmay@xxxxxxxxxxxxx> wrote: >>>>> On Wed, Jun 13, 2018 at 03:11 PM +0200, John Ferlan <jferlan@xxxxxxxxxx> wrote: >>>>>> On 06/07/2018 08:17 AM, Marc Hartmayer wrote: >>>>>>> On Wed, May 09, 2018 at 09:51 PM +0200, John Ferlan <jferlan@xxxxxxxxxx> wrote: >>>>>>>> On 05/07/2018 11:24 AM, Marc Hartmayer wrote: >>>>>>>>> On Mon, Apr 30, 2018 at 03:20 PM +0200, John Ferlan <jferlan@xxxxxxxxxx> 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(¶ms_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, ¶ms, &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 > -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
