On 3/27/19 2:46 PM, Eric Blake wrote:
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 '¶meter_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.
Okay, thanks for detailed explanation. I'll probably go with what Dan sugested and make virQEMUCapsSetVAList inline then. It looks safer and more portable. Michal