src/qemu/qemu_capabilities.c | 6 +++--- src/qemu/qemu_capabilities.h | 2 +- tests/qemuxml2argvtest.c | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-)
There is one specific caller (testInfoSetArgs() in
qemuxml2argvtest.c) which expect the va_list argument to change
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)
I've seen this in action in fact: on i686 the qemuxml2argvtest
fails on the second test case because testInfoSetArgs() sees
ARG_QEMU_CAPS and callse virQEMUCapsSetVAList to process the
capabilities (in this case there's just one
QEMU_CAPS_SECCOMP_BLACKLIST). But since the changes are not
reflected in the caller, in the next iteration testInfoSetArgs()
sees the qemu capability and not ARG_END.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
This passed successfully on x86_64 and i686 in my testing.
src/qemu/qemu_capabilities.c | 6 +++---
src/qemu/qemu_capabilities.h | 2 +-
tests/qemuxml2argvtest.c | 2 +-
3 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index c5954edaf0..e182a2c2e5 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -1665,11 +1665,11 @@ virQEMUCapsSet(virQEMUCapsPtr qemuCaps,
void
virQEMUCapsSetVAList(virQEMUCapsPtr qemuCaps,
- va_list list)
+ va_list *list)
{
int flag;
- while ((flag = va_arg(list, int)) < QEMU_CAPS_LAST)
+ while ((flag = va_arg(*list, int)) < QEMU_CAPS_LAST)
ignore_value(virBitmapSetBit(qemuCaps->flags, flag));
}
@@ -1680,7 +1680,7 @@ virQEMUCapsSetList(virQEMUCapsPtr qemuCaps, ...)
va_list list;
va_start(list, qemuCaps);
- virQEMUCapsSetVAList(qemuCaps, list);
+ virQEMUCapsSetVAList(qemuCaps, &list);
va_end(list);
}
diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
index 7625d754a3..ceab8c9154 100644
--- a/src/qemu/qemu_capabilities.h
+++ b/src/qemu/qemu_capabilities.h
@@ -519,7 +519,7 @@ void virQEMUCapsSet(virQEMUCapsPtr qemuCaps,
virQEMUCapsFlags flag) ATTRIBUTE_NONNULL(1);
void virQEMUCapsSetVAList(virQEMUCapsPtr qemuCaps,
- va_list list) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
+ va_list *list) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
void virQEMUCapsSetList(virQEMUCapsPtr qemuCaps, ...) ATTRIBUTE_NONNULL(1);
void virQEMUCapsClear(virQEMUCapsPtr qemuCaps,
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index 6e7d1b0b9a..a7a8d77e1d 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -650,7 +650,7 @@ testInfoSetArgs(struct testInfo *info,
case ARG_QEMU_CAPS:
if (qemuCaps || !(qemuCaps = virQEMUCapsNew()))
goto cleanup;
- virQEMUCapsSetVAList(qemuCaps, argptr);
+ virQEMUCapsSetVAList(qemuCaps, &argptr);
break;
case ARG_GIC:
--
2.19.2
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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) > > I've seen this in action in fact: on i686 the qemuxml2argvtest > fails on the second test case because testInfoSetArgs() sees > ARG_QEMU_CAPS and callse virQEMUCapsSetVAList to process the s/callse/calls/ > capabilities (in this case there's just one > QEMU_CAPS_SECCOMP_BLACKLIST). But since the changes are not > reflected in the caller, in the next iteration testInfoSetArgs() > sees the qemu capability and not ARG_END. s/qemu/QEMU/ > > Signed-off-by: Michal Privoznik <mprivozn@redhat.com> > --- > > This passed successfully on x86_64 and i686 in my testing. > > src/qemu/qemu_capabilities.c | 6 +++--- > src/qemu/qemu_capabilities.h | 2 +- > tests/qemuxml2argvtest.c | 2 +- > 3 files changed, 5 insertions(+), 5 deletions(-) The changes look okay to me, so Reviewed-by: Andrea Bolognani <abologna@redhat.com> however I'd like to hear Eric's opinion before this gets merged. -- Andrea Bolognani / Red Hat / Virtualization -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Wed, Mar 27, 2019 at 01:02:07PM +0100, 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) > > > > I've seen this in action in fact: on i686 the qemuxml2argvtest > > fails on the second test case because testInfoSetArgs() sees > > ARG_QEMU_CAPS and callse virQEMUCapsSetVAList to process the > > s/callse/calls/ > > > capabilities (in this case there's just one > > QEMU_CAPS_SECCOMP_BLACKLIST). But since the changes are not > > reflected in the caller, in the next iteration testInfoSetArgs() > > sees the qemu capability and not ARG_END. > > s/qemu/QEMU/ > > > > > Signed-off-by: Michal Privoznik <mprivozn@redhat.com> > > --- > > > > This passed successfully on x86_64 and i686 in my testing. > > > > src/qemu/qemu_capabilities.c | 6 +++--- > > src/qemu/qemu_capabilities.h | 2 +- > > tests/qemuxml2argvtest.c | 2 +- > > 3 files changed, 5 insertions(+), 5 deletions(-) > > The changes look okay to me, so > > Reviewed-by: Andrea Bolognani <abologna@redhat.com> > > however I'd like to hear Eric's opinion before this gets merged. I'll trust Michal when he says it works, but it feels a bit odd to be passing a pointer to a va_list around. Personally I would have just inlined virQEMUCapsSetVAList() as it is only 2 lines of code. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
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. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
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 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 3/27/19 4:49 PM, Michal Privoznik wrote: > > Okay, thanks for detailed explanation. I'll probably go with what Dan > sugested and make virQEMUCapsSetVAList inline then. It looks safer and > more portable. Actually, that does not work. Because even if I mark the function 'inline' gcc still produces assembly code that calls the function. So I've tried to come around that using attribute always_inline. But for some reason it is still not working. I guess at this point, it's the best to just drop the virQEMUCapsSetVAList() completely. Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Wed, Mar 27, 2019 at 05:13:59PM +0100, Michal Privoznik wrote: > On 3/27/19 4:49 PM, Michal Privoznik wrote: > > > > > Okay, thanks for detailed explanation. I'll probably go with what Dan > > sugested and make virQEMUCapsSetVAList inline then. It looks safer and > > more portable. > > Actually, that does not work. Because even if I mark the function 'inline' > gcc still produces assembly code that calls the function. So I've tried to > come around that using attribute always_inline. But for some reason it is > still not working. > > I guess at this point, it's the best to just drop the virQEMUCapsSetVAList() > completely. Yeah I should have been clearer. WHen I said "inline" virQEMUCapsSetVAList, I really did mean drop this function entirely & copy its 2 lines of code into the caller. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2024 Red Hat, Inc.