Re: [libvirt] [PATCH] qemu_capabilities: Pass pointer to va_list in virQEMUCapsSetVAList

On 3/27/19 7:02 AM, Andrea Bolognani wrote: > On Wed, 2019-03-27 at 10:50 +0100, Michal Privoznik wrote: >> There is one specific caller (testInfoSetArgs() in >> qemuxml2argvtest.c) which expect the va_list argument to change > > s/ / / > >> after returning from the virQEMUCapsSetVAList() function. >> However, since we are passing plain va_list this is not >> guaranteed. The man page of stdarg(3) says: >> >> If ap is passed to a function that uses va_arg(ap,type), then >> the value of ap is undefined after the return of that function. >> >> (ap is a variable of type va_list) > however I'd like to hear Eric's opinion before this gets merged. Passing a va_list *arg as a parameter is doable, but it comes with odd effects. That is because the C standard permits both of the following implementation styles: typedef struct __something *va_list; typedef struct __something va_list[]; but you get different semantics on what happens if you try to take the address of a va_list parameter, (C parameter lists undergo pointer decay, and taking the address of a parameter results in either the address of a pointer or the address of the first member of an array - which are different levels of dereferencing and thus different types). The type 'pointer to va_list' is sane, and the computation of '&local_va_list' is sane; it is only '&parameter_va_list' that gets you in trouble. Here's where I demonstrated it further for the qemu list: https://lists.gnu.org/archive/html/qemu-devel/2017-10/msg00171.html The only portable way to take va_list from one function to pass to a va_list * of another function is to va_copy() from the parameter into a local va_list, then take the address of the local. But if you are the original owner of a local va_list (because your function took ... instead of va_list), you don't have to worry about the hoop-jumping involved to stay portable. > > @@ -1680,7 +1680,7 @@ virQEMUCapsSetList(virQEMUCapsPtr qemuCaps, ...) > va_list list; > > va_start(list, qemuCaps); > - virQEMUCapsSetVAList(qemuCaps, list); > + virQEMUCapsSetVAList(qemuCaps, &list); > va_end(list); This usage is safe, because it is the address of a local variable. I don't see any instance where you are taking the address of a parameter, so while the patch is unusual, it doesn't look wrong.