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

[Marc Hartmayer]

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

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

>
> FWIW: My f26 man page for calloc says:
>
> "The calloc() function allocates memory for an array of nmemb elements
> of size bytes each and returns a pointer to the allocated memory. The
> memory is set to zero.  If nmemb or size is 0, then calloc() returns
> either NULL, or a unique pointer value that can later be successfully
> passed to free()"
>
> We have so many places in the code that use VIR_ALLOC_N and do not check
> if the sizeof() or count is 0 - which makes me wonder even more about
> the specific case in which you are referencing.

It is not about the general use of VIR_ALLOC_N, but about the result of
the serializer and deserializer.

> I looked through the
> various remote_protocol.x structures that would use this and it seems
> they all use remote_typed_param for the structure being returned, so
> it's not clear how any of them could have a sizeof() returning 0.
>
>> 2. Update the length @remote_params_len only if the related
>>    @remote_params_val has also been set.
>
> This one changes the error case as the returned @remote_params_len
> changes from being set to @params_len on input to be undefined.

Hmm, that’s already the case for 'remote_params_val'? But indeed, we can
either initialize both of them to 0 and NULL or add an error label for
this.

“@remote_params_len: the final number of elements in
@remote_params_val.”

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