| Message ID | 1474391326-871-1-git-send-email-felipe@nutanix.com |
|---|---|
| State | New |
| Headers | show |
Hi,
Your series seems to have some coding style problems. See output below for
more information:
Type: series
Message-id: 1474391326-871-1-git-send-email-felipe@nutanix.com
Subject: [Qemu-devel] [PATCH] replay: Fix build with -Werror=unused-result
=== TEST SCRIPT BEGIN ===
#!/bin/bash
BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0
# Useful git options
git config --local diff.renamelimit 0
git config --local diff.renames True
commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
echo "Checking PATCH $n/$total: $(git show --no-patch --format=%s $c)..."
if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
failed=1
echo
fi
n=$((n+1))
done
exit $failed
=== TEST SCRIPT END ===
Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
- [tag update] patchew/1473591360-13163-1-git-send-email-caoj.fnst@cn.fujitsu.com -> patchew/1473591360-13163-1-git-send-email-caoj.fnst@cn.fujitsu.com
* [new tag] patchew/1474391326-871-1-git-send-email-felipe@nutanix.com -> patchew/1474391326-871-1-git-send-email-felipe@nutanix.com
- [tag update] patchew/1474432046-325-1-git-send-email-famz@redhat.com -> patchew/1474432046-325-1-git-send-email-famz@redhat.com
Switched to a new branch 'test'
4439fa6 replay: Fix build with -Werror=unused-result
=== OUTPUT BEGIN ===
Checking PATCH 1/1: replay: Fix build with -Werror=unused-result...
ERROR: spaces required around that '+' (ctx:VxV)
#22: FILE: replay/replay-internal.c:68:
+ (void)(fwrite(buf, 1, size, replay_file)+1);
^
total: 1 errors, 0 warnings, 8 lines checked
Your patch has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
=== OUTPUT END ===
Test command exited with code: 1
---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@freelists.org
> On 21 Sep 2016, at 07:24, Markus Armbruster <armbru@redhat.com> wrote: > > "Pavel Dovgalyuk" <dovgaluk@ispras.ru> writes: > >>> From: Felipe Franciosi [mailto:felipe@nutanix.com] >>> If compiling with -Werror=unused-result, replay-internal.c won't build >>> due to a call to fwrite() where the returned value is ignored. A simple >>> cast to (void) is not sufficient on recent GCCs, so this fixes it. >>> >>> Signed-off-by: Felipe Franciosi <felipe@nutanix.com> >>> --- >>> replay/replay-internal.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/replay/replay-internal.c b/replay/replay-internal.c >>> index 5835e8d..6978d76 100644 >>> --- a/replay/replay-internal.c >>> +++ b/replay/replay-internal.c >>> @@ -65,7 +65,7 @@ void replay_put_array(const uint8_t *buf, size_t size) >>> { >>> if (replay_file) { >>> replay_put_dword(size); >>> - fwrite(buf, 1, size, replay_file); >>> + (void)(fwrite(buf, 1, size, replay_file)+1); >>> } >>> } >> >> This looks very weird. Oh I couldn't agree more. I hate this syntax. It is, however, the simplest way to get around this issue. See: http://stackoverflow.com/questions/11888594/ignoring-return-values-in-c That doesn't mean we should do it. I actually rather we didn't. :) I sent this patch to discuss how the maintainers feel about it given no one bumped into this yet (apparently). >> I think it would be better to check the return value and stop >> the simulation in case of error. We can also make replay_put_array() return an int indicating whether an error occurred. The callers can then decide whether to care or not. I'll send a v2 doing that instead for comments. > Ignoring errors is wrong more often than not. Whether it's wrong in > this case I can't say. True that. That's why I think a better first step is to make functions that can fail return an error code. Callers can make that decision later. Unfortunately, glibc decided that fwrite() should be tagged with WUR and that triggers this build failure. > What I can say is 1. the commit message needs to > explain *why* the error can be ignored, The error is already ignored today. I'll send a v2 that makes replay_put_array() reflect the error, the callers can then decide to ignore it. > and 2. the way you ignore the > error is scandalously ugly :) Oh we all agree on that. :-D > You say gcc doesn't honor the traditional > cast to void anymore. What's the exact error message you see? See this (lengthly discussion) for more details: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=25509 Thanks, Felipe
> From: Felipe Franciosi [mailto:felipe@nutanix.com] > > On 21 Sep 2016, at 07:24, Markus Armbruster <armbru@redhat.com> wrote: > > > > "Pavel Dovgalyuk" <dovgaluk@ispras.ru> writes: > > > >>> From: Felipe Franciosi [mailto:felipe@nutanix.com] > >>> If compiling with -Werror=unused-result, replay-internal.c won't build > >>> due to a call to fwrite() where the returned value is ignored. A simple > >>> cast to (void) is not sufficient on recent GCCs, so this fixes it. > >>> > >>> Signed-off-by: Felipe Franciosi <felipe@nutanix.com> > >>> --- > >>> replay/replay-internal.c | 2 +- > >>> 1 file changed, 1 insertion(+), 1 deletion(-) > >>> > >>> diff --git a/replay/replay-internal.c b/replay/replay-internal.c > >>> index 5835e8d..6978d76 100644 > >>> --- a/replay/replay-internal.c > >>> +++ b/replay/replay-internal.c > >>> @@ -65,7 +65,7 @@ void replay_put_array(const uint8_t *buf, size_t size) > >>> { > >>> if (replay_file) { > >>> replay_put_dword(size); > >>> - fwrite(buf, 1, size, replay_file); > >>> + (void)(fwrite(buf, 1, size, replay_file)+1); > >>> } > >>> } > >> > >> This looks very weird. > > >> I think it would be better to check the return value and stop > >> the simulation in case of error. > > We can also make replay_put_array() return an int indicating whether an error occurred. The > callers can then decide whether to care or not. I'll send a v2 doing that instead for > comments. > > > Ignoring errors is wrong more often than not. Whether it's wrong in > > this case I can't say. > > True that. That's why I think a better first step is to make functions that can fail return an > error code. Callers can make that decision later. Unfortunately, glibc decided that fwrite() > should be tagged with WUR and that triggers this build failure. > > > What I can say is 1. the commit message needs to > > explain *why* the error can be ignored, > > The error is already ignored today. I'll send a v2 that makes replay_put_array() reflect the > error, the callers can then decide to ignore it. There is no need to return an error. When rr log cannot be created there is no need in executing in record mode. Therefore execution has to be stopped in case of an error. Pavel Dovgalyuk
On Wed, Sep 21, 2016 at 10:00:23AM +0000, Felipe Franciosi wrote: > > > On 21 Sep 2016, at 07:24, Markus Armbruster <armbru@redhat.com> wrote: > > > > "Pavel Dovgalyuk" <dovgaluk@ispras.ru> writes: > > > >>> From: Felipe Franciosi [mailto:felipe@nutanix.com] > >>> If compiling with -Werror=unused-result, replay-internal.c won't build > >>> due to a call to fwrite() where the returned value is ignored. A simple > >>> cast to (void) is not sufficient on recent GCCs, so this fixes it. > >>> > >>> Signed-off-by: Felipe Franciosi <felipe@nutanix.com> > >>> --- > >>> replay/replay-internal.c | 2 +- > >>> 1 file changed, 1 insertion(+), 1 deletion(-) > >>> > >>> diff --git a/replay/replay-internal.c b/replay/replay-internal.c > >>> index 5835e8d..6978d76 100644 > >>> --- a/replay/replay-internal.c > >>> +++ b/replay/replay-internal.c > >>> @@ -65,7 +65,7 @@ void replay_put_array(const uint8_t *buf, size_t size) > >>> { > >>> if (replay_file) { > >>> replay_put_dword(size); > >>> - fwrite(buf, 1, size, replay_file); > >>> + (void)(fwrite(buf, 1, size, replay_file)+1); > >>> } > >>> } > >> > >> This looks very weird. > > Oh I couldn't agree more. I hate this syntax. It is, however, the simplest way to get around this issue. See: > http://stackoverflow.com/questions/11888594/ignoring-return-values-in-c If we want to ignore return value reliably, lets just pull in the ignore_value macro from gnulib which is known to work across GCC versions /* Normally casting an expression to void discards its value, but GCC versions 3.4 and newer have __attribute__ ((__warn_unused_result__)) which may cause unwanted diagnostics in that case. Use __typeof__ and __extension__ to work around the problem, if the workaround is known to be needed. */ #if 3 < __GNUC__ + (4 <= __GNUC_MINOR__) # define ignore_value(x) \ (__extension__ ({ __typeof__ (x) __x = (x); (void) __x; })) #else # define ignore_value(x) ((void) (x)) #endif GNULIB makes it availavble under LGPLv2.1+ eg used as: ignore_value(fwrite(buf, 1, size, replay_file)); Regards, Daniel
> On 21 Sep 2016, at 11:07, Daniel P. Berrange <berrange@redhat.com> wrote: > > On Wed, Sep 21, 2016 at 10:00:23AM +0000, Felipe Franciosi wrote: >> >>> On 21 Sep 2016, at 07:24, Markus Armbruster <armbru@redhat.com> wrote: >>> >>> "Pavel Dovgalyuk" <dovgaluk@ispras.ru> writes: >>> >>>>> From: Felipe Franciosi [mailto:felipe@nutanix.com] >>>>> If compiling with -Werror=unused-result, replay-internal.c won't build >>>>> due to a call to fwrite() where the returned value is ignored. A simple >>>>> cast to (void) is not sufficient on recent GCCs, so this fixes it. >>>>> >>>>> Signed-off-by: Felipe Franciosi <felipe@nutanix.com> >>>>> --- >>>>> replay/replay-internal.c | 2 +- >>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>> >>>>> diff --git a/replay/replay-internal.c b/replay/replay-internal.c >>>>> index 5835e8d..6978d76 100644 >>>>> --- a/replay/replay-internal.c >>>>> +++ b/replay/replay-internal.c >>>>> @@ -65,7 +65,7 @@ void replay_put_array(const uint8_t *buf, size_t size) >>>>> { >>>>> if (replay_file) { >>>>> replay_put_dword(size); >>>>> - fwrite(buf, 1, size, replay_file); >>>>> + (void)(fwrite(buf, 1, size, replay_file)+1); >>>>> } >>>>> } >>>> >>>> This looks very weird. >> >> Oh I couldn't agree more. I hate this syntax. It is, however, the simplest way to get around this issue. See: >> http://stackoverflow.com/questions/11888594/ignoring-return-values-in-c > > If we want to ignore return value reliably, lets just pull in the > ignore_value macro from gnulib which is known to work across GCC > versions I like that better. Do you want a series with a patch to add this macro to include/qemu/compiler.h and another making replay_put_array() use it? Cheers, Felipe > > > /* Normally casting an expression to void discards its value, but GCC > versions 3.4 and newer have __attribute__ ((__warn_unused_result__)) > which may cause unwanted diagnostics in that case. Use __typeof__ > and __extension__ to work around the problem, if the workaround is > known to be needed. */ > #if 3 < __GNUC__ + (4 <= __GNUC_MINOR__) > # define ignore_value(x) \ > (__extension__ ({ __typeof__ (x) __x = (x); (void) __x; })) > #else > # define ignore_value(x) ((void) (x)) > #endif > > GNULIB makes it availavble under LGPLv2.1+ > > eg used as: > > ignore_value(fwrite(buf, 1, size, replay_file)); > > Regards, > Daniel > -- > |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| > |: http://libvirt.org -o- http://virt-manager.org :| > |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| > |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
Actually, I just noticed Pavel is in the middle of submitting a "replay additions" series (currently at v4). Pavel: is this something you can address as part of that series? Thanks, Felipe > On 21 Sep 2016, at 11:12, Felipe Franciosi <felipe@nutanix.com> wrote: > > >> On 21 Sep 2016, at 11:07, Daniel P. Berrange <berrange@redhat.com> wrote: >> >> On Wed, Sep 21, 2016 at 10:00:23AM +0000, Felipe Franciosi wrote: >>> >>>> On 21 Sep 2016, at 07:24, Markus Armbruster <armbru@redhat.com> wrote: >>>> >>>> "Pavel Dovgalyuk" <dovgaluk@ispras.ru> writes: >>>> >>>>>> From: Felipe Franciosi [mailto:felipe@nutanix.com] >>>>>> If compiling with -Werror=unused-result, replay-internal.c won't build >>>>>> due to a call to fwrite() where the returned value is ignored. A simple >>>>>> cast to (void) is not sufficient on recent GCCs, so this fixes it. >>>>>> >>>>>> Signed-off-by: Felipe Franciosi <felipe@nutanix.com> >>>>>> --- >>>>>> replay/replay-internal.c | 2 +- >>>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>>> >>>>>> diff --git a/replay/replay-internal.c b/replay/replay-internal.c >>>>>> index 5835e8d..6978d76 100644 >>>>>> --- a/replay/replay-internal.c >>>>>> +++ b/replay/replay-internal.c >>>>>> @@ -65,7 +65,7 @@ void replay_put_array(const uint8_t *buf, size_t size) >>>>>> { >>>>>> if (replay_file) { >>>>>> replay_put_dword(size); >>>>>> - fwrite(buf, 1, size, replay_file); >>>>>> + (void)(fwrite(buf, 1, size, replay_file)+1); >>>>>> } >>>>>> } >>>>> >>>>> This looks very weird. >>> >>> Oh I couldn't agree more. I hate this syntax. It is, however, the simplest way to get around this issue. See: >>> http://stackoverflow.com/questions/11888594/ignoring-return-values-in-c >> >> If we want to ignore return value reliably, lets just pull in the >> ignore_value macro from gnulib which is known to work across GCC >> versions > > I like that better. Do you want a series with a patch to add this macro to include/qemu/compiler.h and another making replay_put_array() use it? > > Cheers, > Felipe > >> >> >> /* Normally casting an expression to void discards its value, but GCC >> versions 3.4 and newer have __attribute__ ((__warn_unused_result__)) >> which may cause unwanted diagnostics in that case. Use __typeof__ >> and __extension__ to work around the problem, if the workaround is >> known to be needed. */ >> #if 3 < __GNUC__ + (4 <= __GNUC_MINOR__) >> # define ignore_value(x) \ >> (__extension__ ({ __typeof__ (x) __x = (x); (void) __x; })) >> #else >> # define ignore_value(x) ((void) (x)) >> #endif >> >> GNULIB makes it availavble under LGPLv2.1+ >> >> eg used as: >> >> ignore_value(fwrite(buf, 1, size, replay_file)); >> >> Regards, >> Daniel >> -- >> |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| >> |: http://libvirt.org -o- http://virt-manager.org :| >> |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| >> |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :| >
"Daniel P. Berrange" <berrange@redhat.com> writes: > On Wed, Sep 21, 2016 at 10:00:23AM +0000, Felipe Franciosi wrote: >> >> > On 21 Sep 2016, at 07:24, Markus Armbruster <armbru@redhat.com> wrote: >> > >> > "Pavel Dovgalyuk" <dovgaluk@ispras.ru> writes: >> > >> >>> From: Felipe Franciosi [mailto:felipe@nutanix.com] >> >>> If compiling with -Werror=unused-result, replay-internal.c won't build >> >>> due to a call to fwrite() where the returned value is ignored. A simple >> >>> cast to (void) is not sufficient on recent GCCs, so this fixes it. >> >>> >> >>> Signed-off-by: Felipe Franciosi <felipe@nutanix.com> >> >>> --- >> >>> replay/replay-internal.c | 2 +- >> >>> 1 file changed, 1 insertion(+), 1 deletion(-) >> >>> >> >>> diff --git a/replay/replay-internal.c b/replay/replay-internal.c >> >>> index 5835e8d..6978d76 100644 >> >>> --- a/replay/replay-internal.c >> >>> +++ b/replay/replay-internal.c >> >>> @@ -65,7 +65,7 @@ void replay_put_array(const uint8_t *buf, size_t size) >> >>> { >> >>> if (replay_file) { >> >>> replay_put_dword(size); >> >>> - fwrite(buf, 1, size, replay_file); >> >>> + (void)(fwrite(buf, 1, size, replay_file)+1); >> >>> } >> >>> } >> >> >> >> This looks very weird. >> >> Oh I couldn't agree more. I hate this syntax. It is, however, the simplest way to get around this issue. See: >> http://stackoverflow.com/questions/11888594/ignoring-return-values-in-c > > If we want to ignore return value reliably, lets just pull in the > ignore_value macro from gnulib which is known to work across GCC > versions > > > /* Normally casting an expression to void discards its value, but GCC > versions 3.4 and newer have __attribute__ ((__warn_unused_result__)) > which may cause unwanted diagnostics in that case. Use __typeof__ > and __extension__ to work around the problem, if the workaround is > known to be needed. */ > #if 3 < __GNUC__ + (4 <= __GNUC_MINOR__) > # define ignore_value(x) \ > (__extension__ ({ __typeof__ (x) __x = (x); (void) __x; })) > #else > # define ignore_value(x) ((void) (x)) > #endif Casting a value to void is the traditional and obvious way to say "yes, I mean to ignore this value". Now compilers start to reply "no, you don't". We can invent new (and less obvious) ways to say "yes, I do", and compilers can then learn them so they can again reply "no, you don't". Why have compilers started to behave like two-year-olds? [...]
On 09/21/2016 07:31 AM, Markus Armbruster wrote: >> >> If we want to ignore return value reliably, lets just pull in the >> ignore_value macro from gnulib which is known to work across GCC >> versions >> >> >> /* Normally casting an expression to void discards its value, but GCC >> versions 3.4 and newer have __attribute__ ((__warn_unused_result__)) >> which may cause unwanted diagnostics in that case. Use __typeof__ >> and __extension__ to work around the problem, if the workaround is >> known to be needed. */ >> #if 3 < __GNUC__ + (4 <= __GNUC_MINOR__) >> # define ignore_value(x) \ >> (__extension__ ({ __typeof__ (x) __x = (x); (void) __x; })) >> #else >> # define ignore_value(x) ((void) (x)) >> #endif > > Casting a value to void is the traditional and obvious way to say "yes, > I mean to ignore this value". Now compilers start to reply "no, you > don't". We can invent new (and less obvious) ways to say "yes, I do", > and compilers can then learn them so they can again reply "no, you > don't". Why have compilers started to behave like two-year-olds? gcc has been doing the "__warn_unused_value__ means cast-to-void is insufficient" complaint for years (since at least 2008, per the gnulib history). But the gnulib workaround has also been effectively silencing it for years (it was actually my work in 2011, commit 939dedd, which came up with the form listed above). The other nice thing about "ignore_value(wur_function())" is that you are avoiding a cast in your local code, and the burden of shutting up the annoying compiler is hidden behind a macro that can easily be changed to affect all clients of the macro, should gcc regress yet again and we need some other formula to shut it up. And yes, the gnulib mailing list has threads complaining about gcc's behavior back when the macro had to be invented, and again when glibc added wur markings to functions that can legitimately be ignored (fread() is one of them; because there are valid programming paradigms where you check ferror() later on rather than having to check every intermediate fread(), at the expense of less-specific error messages).
> On 21 Sep 2016, at 14:55, Eric Blake <eblake@redhat.com> wrote: > > On 09/21/2016 07:31 AM, Markus Armbruster wrote: >>> >>> If we want to ignore return value reliably, lets just pull in the >>> ignore_value macro from gnulib which is known to work across GCC >>> versions >>> >>> >>> /* Normally casting an expression to void discards its value, but GCC >>> versions 3.4 and newer have __attribute__ ((__warn_unused_result__)) >>> which may cause unwanted diagnostics in that case. Use __typeof__ >>> and __extension__ to work around the problem, if the workaround is >>> known to be needed. */ >>> #if 3 < __GNUC__ + (4 <= __GNUC_MINOR__) >>> # define ignore_value(x) \ >>> (__extension__ ({ __typeof__ (x) __x = (x); (void) __x; })) >>> #else >>> # define ignore_value(x) ((void) (x)) >>> #endif >> >> Casting a value to void is the traditional and obvious way to say "yes, >> I mean to ignore this value". Now compilers start to reply "no, you >> don't". We can invent new (and less obvious) ways to say "yes, I do", >> and compilers can then learn them so they can again reply "no, you >> don't". Why have compilers started to behave like two-year-olds? > > gcc has been doing the "__warn_unused_value__ means cast-to-void is > insufficient" complaint for years (since at least 2008, per the gnulib > history). But the gnulib workaround has also been effectively silencing > it for years (it was actually my work in 2011, commit 939dedd, which > came up with the form listed above). The other nice thing about > "ignore_value(wur_function())" is that you are avoiding a cast in your > local code, and the burden of shutting up the annoying compiler is > hidden behind a macro that can easily be changed to affect all clients > of the macro, should gcc regress yet again and we need some other > formula to shut it up. > > And yes, the gnulib mailing list has threads complaining about gcc's > behavior back when the macro had to be invented, and again when glibc > added wur markings to functions that can legitimately be ignored > (fread() is one of them; because there are valid programming paradigms > where you check ferror() later on rather than having to check every > intermediate fread(), at the expense of less-specific error messages). What's the best way to bring gnulib's ignore-value.h into Qemu? I'd think we could just add to include/qemu/compiler.h something like: ----------------------8<---------------------- #if QEMU_GNUC_PREREQ(3, 4) /* From gnulib's ignore-value.h by Jim Meyering, Eric Blake and Padraig Brady */ # define ignore_value(x) \ (__extension__ ({ __typeof__ (x) __x = (x); (void) __x; })) #else # define ignore_value(x) ((void) (x)) #endif ----------------------8<---------------------- But I'm not sure if that suffices to meet GPL's requirements. Thanks, Felipe > > -- > Eric Blake eblake redhat com +1-919-301-3266 > Libvirt virtualization library http://libvirt.org >
On Wed, Sep 21, 2016 at 02:26:48PM +0000, Felipe Franciosi wrote: > > > On 21 Sep 2016, at 14:55, Eric Blake <eblake@redhat.com> wrote: > > > > On 09/21/2016 07:31 AM, Markus Armbruster wrote: > >>> > >>> If we want to ignore return value reliably, lets just pull in the > >>> ignore_value macro from gnulib which is known to work across GCC > >>> versions > >>> > >>> > >>> /* Normally casting an expression to void discards its value, but GCC > >>> versions 3.4 and newer have __attribute__ ((__warn_unused_result__)) > >>> which may cause unwanted diagnostics in that case. Use __typeof__ > >>> and __extension__ to work around the problem, if the workaround is > >>> known to be needed. */ > >>> #if 3 < __GNUC__ + (4 <= __GNUC_MINOR__) > >>> # define ignore_value(x) \ > >>> (__extension__ ({ __typeof__ (x) __x = (x); (void) __x; })) > >>> #else > >>> # define ignore_value(x) ((void) (x)) > >>> #endif > >> > >> Casting a value to void is the traditional and obvious way to say "yes, > >> I mean to ignore this value". Now compilers start to reply "no, you > >> don't". We can invent new (and less obvious) ways to say "yes, I do", > >> and compilers can then learn them so they can again reply "no, you > >> don't". Why have compilers started to behave like two-year-olds? > > > > gcc has been doing the "__warn_unused_value__ means cast-to-void is > > insufficient" complaint for years (since at least 2008, per the gnulib > > history). But the gnulib workaround has also been effectively silencing > > it for years (it was actually my work in 2011, commit 939dedd, which > > came up with the form listed above). The other nice thing about > > "ignore_value(wur_function())" is that you are avoiding a cast in your > > local code, and the burden of shutting up the annoying compiler is > > hidden behind a macro that can easily be changed to affect all clients > > of the macro, should gcc regress yet again and we need some other > > formula to shut it up. > > > > And yes, the gnulib mailing list has threads complaining about gcc's > > behavior back when the macro had to be invented, and again when glibc > > added wur markings to functions that can legitimately be ignored > > (fread() is one of them; because there are valid programming paradigms > > where you check ferror() later on rather than having to check every > > intermediate fread(), at the expense of less-specific error messages). > > What's the best way to bring gnulib's ignore-value.h into Qemu? I'd think we could just add to include/qemu/compiler.h something like: > > ----------------------8<---------------------- > #if QEMU_GNUC_PREREQ(3, 4) > /* From gnulib's ignore-value.h by Jim Meyering, Eric Blake and Padraig Brady */ > # define ignore_value(x) \, > (__extension__ ({ __typeof__ (x) __x = (x); (void) __x; })) > #else > # define ignore_value(x) ((void) (x)) > #endif > ----------------------8<---------------------- > > But I'm not sure if that suffices to meet GPL's requirements. The compiler.h file has no license header, just a comment saying "public domain", which is obviously not the case if you add this macro. Given that you'll need to explicitly mention the license terms for ignore_value. eg with a comment line like /* The ignore_value() macro is taken from GNULIB ignore-value.h, * licensed under the terms of the LGPLv2+ */ Regards, Daniel
> On 21 Sep 2016, at 15:35, Daniel P. Berrange <berrange@redhat.com> wrote: > > On Wed, Sep 21, 2016 at 02:26:48PM +0000, Felipe Franciosi wrote: >> >>> On 21 Sep 2016, at 14:55, Eric Blake <eblake@redhat.com> wrote: >>> >>> On 09/21/2016 07:31 AM, Markus Armbruster wrote: >>>>> >>>>> If we want to ignore return value reliably, lets just pull in the >>>>> ignore_value macro from gnulib which is known to work across GCC >>>>> versions >>>>> >>>>> >>>>> /* Normally casting an expression to void discards its value, but GCC >>>>> versions 3.4 and newer have __attribute__ ((__warn_unused_result__)) >>>>> which may cause unwanted diagnostics in that case. Use __typeof__ >>>>> and __extension__ to work around the problem, if the workaround is >>>>> known to be needed. */ >>>>> #if 3 < __GNUC__ + (4 <= __GNUC_MINOR__) >>>>> # define ignore_value(x) \ >>>>> (__extension__ ({ __typeof__ (x) __x = (x); (void) __x; })) >>>>> #else >>>>> # define ignore_value(x) ((void) (x)) >>>>> #endif >>>> >>>> Casting a value to void is the traditional and obvious way to say "yes, >>>> I mean to ignore this value". Now compilers start to reply "no, you >>>> don't". We can invent new (and less obvious) ways to say "yes, I do", >>>> and compilers can then learn them so they can again reply "no, you >>>> don't". Why have compilers started to behave like two-year-olds? >>> >>> gcc has been doing the "__warn_unused_value__ means cast-to-void is >>> insufficient" complaint for years (since at least 2008, per the gnulib >>> history). But the gnulib workaround has also been effectively silencing >>> it for years (it was actually my work in 2011, commit 939dedd, which >>> came up with the form listed above). The other nice thing about >>> "ignore_value(wur_function())" is that you are avoiding a cast in your >>> local code, and the burden of shutting up the annoying compiler is >>> hidden behind a macro that can easily be changed to affect all clients >>> of the macro, should gcc regress yet again and we need some other >>> formula to shut it up. >>> >>> And yes, the gnulib mailing list has threads complaining about gcc's >>> behavior back when the macro had to be invented, and again when glibc >>> added wur markings to functions that can legitimately be ignored >>> (fread() is one of them; because there are valid programming paradigms >>> where you check ferror() later on rather than having to check every >>> intermediate fread(), at the expense of less-specific error messages). >> >> What's the best way to bring gnulib's ignore-value.h into Qemu? I'd think we could just add to include/qemu/compiler.h something like: >> >> ----------------------8<---------------------- >> #if QEMU_GNUC_PREREQ(3, 4) >> /* From gnulib's ignore-value.h by Jim Meyering, Eric Blake and Padraig Brady */ >> # define ignore_value(x) \, >> (__extension__ ({ __typeof__ (x) __x = (x); (void) __x; })) >> #else >> # define ignore_value(x) ((void) (x)) >> #endif >> ----------------------8<---------------------- >> >> But I'm not sure if that suffices to meet GPL's requirements. > > The compiler.h file has no license header, just a comment > saying "public domain", which is obviously not the case > if you add this macro. > > Given that you'll need to explicitly mention the license terms > for ignore_value. eg with a comment line like > > /* The ignore_value() macro is taken from GNULIB ignore-value.h, > * licensed under the terms of the LGPLv2+ > */ Awesome, thanks! I think it's better to fit it in compiler.h than to add a separate header file, which would then need to be included by any potential user. Felipe > > Regards, > Daniel > -- > |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| > |: http://libvirt.org -o- http://virt-manager.org :| > |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| > |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 09/21/2016 09:26 AM, Felipe Franciosi wrote: > > What's the best way to bring gnulib's ignore-value.h into Qemu? I'd think we could just add to include/qemu/compiler.h something like: > > ----------------------8<---------------------- > #if QEMU_GNUC_PREREQ(3, 4) > /* From gnulib's ignore-value.h by Jim Meyering, Eric Blake and Padraig Brady */ > # define ignore_value(x) \ > (__extension__ ({ __typeof__ (x) __x = (x); (void) __x; })) > #else > # define ignore_value(x) ((void) (x)) > #endif > ----------------------8<---------------------- > > But I'm not sure if that suffices to meet GPL's requirements. As Dan already pointed out, gnulib's ignore_value() is available under LGPLv2+ (the full gnulib.git/lib/ignore-value.h file states GPLv3+, for ease of copying into other GPLv3 projects; but the modules/ignore-value file states what additional licenses it can be used under). Using 'gnulib-tool --lgpl=2 ignore-value' would give you the modified version; but it is probably overkill for this situation (we can skip straight to the end result instead of going through intermediate steps). Furthermore, I'm the original author of that code snippet as listed (as gnulib.git will show); where the only modification made after I wrote it was whitespace changes and the addition of __extension__, and I'm just fine with those lines being used as-is in qemu.git (there's benefits when an original author states intentions :) So copying and pasting the relevant snippet into compiler.h is indeed the way to go; in fact, you can probably get away with a comment that just says "/* From gnulib's LGPLv2+ ignore-value.h */" without having to call out particular authors. Don't bother creating any new header or worrying about copyright boilerplate changes; and you can point to this message-id in the commit message if you are worried about a paper trail; you are not violating the GPL in this instance.
On 09/21/2016 09:35 AM, Daniel P. Berrange wrote: >> But I'm not sure if that suffices to meet GPL's requirements. > > The compiler.h file has no license header, just a comment > saying "public domain", which is obviously not the case > if you add this macro. Oh, I missed that; I was assuming compiler.h was GPLv2+ per the usual project defaults; but that file is explicitly looser. > > Given that you'll need to explicitly mention the license terms > for ignore_value. eg with a comment line like > > /* The ignore_value() macro is taken from GNULIB ignore-value.h, > * licensed under the terms of the LGPLv2+ > */ Yes, adding that solves any questions of origin.
"Daniel P. Berrange" <berrange@redhat.com> writes: > On Wed, Sep 21, 2016 at 02:26:48PM +0000, Felipe Franciosi wrote: >> >> > On 21 Sep 2016, at 14:55, Eric Blake <eblake@redhat.com> wrote: >> > >> > On 09/21/2016 07:31 AM, Markus Armbruster wrote: >> >>> >> >>> If we want to ignore return value reliably, lets just pull in the >> >>> ignore_value macro from gnulib which is known to work across GCC >> >>> versions >> >>> >> >>> >> >>> /* Normally casting an expression to void discards its value, but GCC >> >>> versions 3.4 and newer have __attribute__ ((__warn_unused_result__)) >> >>> which may cause unwanted diagnostics in that case. Use __typeof__ >> >>> and __extension__ to work around the problem, if the workaround is >> >>> known to be needed. */ >> >>> #if 3 < __GNUC__ + (4 <= __GNUC_MINOR__) >> >>> # define ignore_value(x) \ >> >>> (__extension__ ({ __typeof__ (x) __x = (x); (void) __x; })) >> >>> #else >> >>> # define ignore_value(x) ((void) (x)) >> >>> #endif >> >> >> >> Casting a value to void is the traditional and obvious way to say "yes, >> >> I mean to ignore this value". Now compilers start to reply "no, you >> >> don't". We can invent new (and less obvious) ways to say "yes, I do", >> >> and compilers can then learn them so they can again reply "no, you >> >> don't". Why have compilers started to behave like two-year-olds? >> > >> > gcc has been doing the "__warn_unused_value__ means cast-to-void is >> > insufficient" complaint for years (since at least 2008, per the gnulib >> > history). But the gnulib workaround has also been effectively silencing >> > it for years (it was actually my work in 2011, commit 939dedd, which >> > came up with the form listed above). The other nice thing about >> > "ignore_value(wur_function())" is that you are avoiding a cast in your >> > local code, and the burden of shutting up the annoying compiler is >> > hidden behind a macro that can easily be changed to affect all clients >> > of the macro, should gcc regress yet again and we need some other >> > formula to shut it up. >> > >> > And yes, the gnulib mailing list has threads complaining about gcc's >> > behavior back when the macro had to be invented, and again when glibc >> > added wur markings to functions that can legitimately be ignored >> > (fread() is one of them; because there are valid programming paradigms >> > where you check ferror() later on rather than having to check every >> > intermediate fread(), at the expense of less-specific error messages). >> >> What's the best way to bring gnulib's ignore-value.h into Qemu? I'd think we could just add to include/qemu/compiler.h something like: >> >> ----------------------8<---------------------- >> #if QEMU_GNUC_PREREQ(3, 4) >> /* From gnulib's ignore-value.h by Jim Meyering, Eric Blake and Padraig Brady */ >> # define ignore_value(x) \, >> (__extension__ ({ __typeof__ (x) __x = (x); (void) __x; })) >> #else >> # define ignore_value(x) ((void) (x)) >> #endif >> ----------------------8<---------------------- >> >> But I'm not sure if that suffices to meet GPL's requirements. > > The compiler.h file has no license header, just a comment > saying "public domain", which is obviously not the case > if you add this macro. > > Given that you'll need to explicitly mention the license terms > for ignore_value. eg with a comment line like > > /* The ignore_value() macro is taken from GNULIB ignore-value.h, > * licensed under the terms of the LGPLv2+ > */ Our tree has a mix of licenses, which is enough of a pain. Mixing licenses within *files* is even worse, and might not even be legally sound. Relicense the whole file under our preferred license GPLv2+?
On 09/21/2016 10:28 AM, Markus Armbruster wrote: >> The compiler.h file has no license header, just a comment >> saying "public domain", which is obviously not the case >> if you add this macro. >> >> Given that you'll need to explicitly mention the license terms >> for ignore_value. eg with a comment line like >> >> /* The ignore_value() macro is taken from GNULIB ignore-value.h, >> * licensed under the terms of the LGPLv2+ >> */ > > Our tree has a mix of licenses, which is enough of a pain. Mixing > licenses within *files* is even worse, and might not even be legally > sound. > > Relicense the whole file under our preferred license GPLv2+? That works too. No one can legally complain - the current license is so permissive that marking the entire file LGPLv2+ is permitted by the current license. It's a one-way conversion (we can't go back once we do it), but I would be fine with that approach.
On Wed, Sep 21, 2016 at 01:18:58PM -0500, Eric Blake wrote: > On 09/21/2016 10:28 AM, Markus Armbruster wrote: > > >> The compiler.h file has no license header, just a comment > >> saying "public domain", which is obviously not the case > >> if you add this macro. > >> > >> Given that you'll need to explicitly mention the license terms > >> for ignore_value. eg with a comment line like > >> > >> /* The ignore_value() macro is taken from GNULIB ignore-value.h, > >> * licensed under the terms of the LGPLv2+ > >> */ > > > > Our tree has a mix of licenses, which is enough of a pain. Mixing > > licenses within *files* is even worse, and might not even be legally > > sound. > > > > Relicense the whole file under our preferred license GPLv2+? > > That works too. No one can legally complain - the current license is so > permissive that marking the entire file LGPLv2+ is permitted by the > current license. It's a one-way conversion (we can't go back once we do > it), but I would be fine with that approach. I think the file probably should not have been listed as public domain in the first place, as its initial contents were copied from qemu-common.h which is not public domain. Regards, Daniel
"Daniel P. Berrange" <berrange@redhat.com> writes: > On Wed, Sep 21, 2016 at 01:18:58PM -0500, Eric Blake wrote: >> On 09/21/2016 10:28 AM, Markus Armbruster wrote: >> >> >> The compiler.h file has no license header, just a comment >> >> saying "public domain", which is obviously not the case >> >> if you add this macro. >> >> >> >> Given that you'll need to explicitly mention the license terms >> >> for ignore_value. eg with a comment line like >> >> >> >> /* The ignore_value() macro is taken from GNULIB ignore-value.h, >> >> * licensed under the terms of the LGPLv2+ >> >> */ >> > >> > Our tree has a mix of licenses, which is enough of a pain. Mixing >> > licenses within *files* is even worse, and might not even be legally >> > sound. >> > >> > Relicense the whole file under our preferred license GPLv2+? >> >> That works too. No one can legally complain - the current license is so >> permissive that marking the entire file LGPLv2+ is permitted by the >> current license. It's a one-way conversion (we can't go back once we do >> it), but I would be fine with that approach. > > I think the file probably should not have been listed as public domain > in the first place, as its initial contents were copied from qemu-common.h > which is not public domain. Ewww! Needs fixing. Since qemu-common.h carries no license, GPLv2+ applies.
On 09/22/2016 06:51 AM, Markus Armbruster wrote: >> >> I think the file probably should not have been listed as public domain >> in the first place, as its initial contents were copied from qemu-common.h >> which is not public domain. > > Ewww! Needs fixing. Indeed. Commit 5c02632 shows the initial file creation, but Luiz used a different email address at that time. What would be the best line to list as a copyright holder on both qemu-common.h and on compiler.h? > > Since qemu-common.h carries no license, GPLv2+ applies. >
Eric Blake <eblake@redhat.com> writes: > On 09/22/2016 06:51 AM, Markus Armbruster wrote: >>> >>> I think the file probably should not have been listed as public domain >>> in the first place, as its initial contents were copied from qemu-common.h >>> which is not public domain. >> >> Ewww! Needs fixing. > > Indeed. Commit 5c02632 shows the initial file creation, but Luiz used a > different email address at that time. What would be the best line to > list as a copyright holder on both qemu-common.h and on compiler.h? Since qemu-common.h carried no licence notice back then[*], adding the line "/* public domain */" was to compiler.h was wrong, and is probably legally void. The immediate fix is therefore dropping that line. Then the project-wide license applies: GPLv2+. We can always copy the standard GPLv2+ licence notice to files that don't have one, if we think that's useful. >> Since qemu-common.h carries no license, GPLv2+ applies. [*] It still doesn't, but that's immaterial.
> On 23 Sep 2016, at 09:15, Markus Armbruster <armbru@redhat.com> wrote: > > Eric Blake <eblake@redhat.com> writes: > >> On 09/22/2016 06:51 AM, Markus Armbruster wrote: >>>> >>>> I think the file probably should not have been listed as public domain >>>> in the first place, as its initial contents were copied from qemu-common.h >>>> which is not public domain. >>> >>> Ewww! Needs fixing. >> >> Indeed. Commit 5c02632 shows the initial file creation, but Luiz used a >> different email address at that time. What would be the best line to >> list as a copyright holder on both qemu-common.h and on compiler.h? > > Since qemu-common.h carried no licence notice back then[*], adding the > line "/* public domain */" was to compiler.h was wrong, and is probably > legally void. The immediate fix is therefore dropping that line. Then > the project-wide license applies: GPLv2+. > > We can always copy the standard GPLv2+ licence notice to files that > don't have one, if we think that's useful. > >>> Since qemu-common.h carries no license, GPLv2+ applies. > > > [*] It still doesn't, but that's immaterial. Seems like all of us agree on that, so I sent another patch just for this. Once it's in, we can go back to adding the 'ignore_value()' macro and figuring out what to do with 'replay'. http://lists.nongnu.org/archive/html/qemu-devel/2016-09/msg06206.html Cheers, Felipe
diff --git a/replay/replay-internal.c b/replay/replay-internal.c index 5835e8d..6978d76 100644 --- a/replay/replay-internal.c +++ b/replay/replay-internal.c @@ -65,7 +65,7 @@ void replay_put_array(const uint8_t *buf, size_t size) { if (replay_file) { replay_put_dword(size); - fwrite(buf, 1, size, replay_file); + (void)(fwrite(buf, 1, size, replay_file)+1); } }
If compiling with -Werror=unused-result, replay-internal.c won't build due to a call to fwrite() where the returned value is ignored. A simple cast to (void) is not sufficient on recent GCCs, so this fixes it. Signed-off-by: Felipe Franciosi <felipe@nutanix.com> --- replay/replay-internal.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)