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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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

Maybe it was an empty dictionary.

[…snip]

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




[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux