| Message ID | 1474471622-12802-2-git-send-email-felipe@nutanix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 09/21/2016 10:27 AM, Felipe Franciosi wrote: > If building with GCC 3.4 or newer (and using -Werror=unused-result), > replay-internal.c will fail to compile due to a call to fwrite() where > the return value is not used. Since fwrite() is declared with WUR in > glibc, callers should check the return value or find other ways to > ignore it. The error message in this specific case is: > > replay/replay-internal.c: In function ‘replay_put_array’: > replay/replay-internal.c:68:15: error: ignoring return value of > ‘fwrite’, declared with attribute warn_unused_result [-Werror=unused-result] > fwrite(buf, 1, size, replay_file); > ^ > > This commit wraps the fwrite() call with the ignore_value() macro, which > currently suppresses the error for existing GCC versions. This explains what you did, but not quite why. In other words, convince me that ignoring the error is the right thing to do in the first place... > > Signed-off-by: Felipe Franciosi <felipe@nutanix.com> > --- > replay/replay-internal. | 0 > replay/replay-internal.c | 2 +- > 2 files changed, 1 insertion(+), 1 deletion(-) > create mode 100644 replay/replay-internal. > > diff --git a/replay/replay-internal. b/replay/replay-internal. > new file mode 100644 > index 0000000..e69de29 > diff --git a/replay/replay-internal.c b/replay/replay-internal.c > index 5835e8d..61de8f9 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); > + ignore_value(fwrite(buf, 1, size, replay_file)); I would be more convinced that this patch is correct if you added a comment here why fwrite() failures can be ignored in this situation, so that someone doesn't undo your commit to add in proper error handling.
> On 21 Sep 2016, at 19:17, Eric Blake <eblake@redhat.com> wrote: > > On 09/21/2016 10:27 AM, Felipe Franciosi wrote: >> If building with GCC 3.4 or newer (and using -Werror=unused-result), >> replay-internal.c will fail to compile due to a call to fwrite() where >> the return value is not used. Since fwrite() is declared with WUR in >> glibc, callers should check the return value or find other ways to >> ignore it. The error message in this specific case is: >> >> replay/replay-internal.c: In function ‘replay_put_array’: >> replay/replay-internal.c:68:15: error: ignoring return value of >> ‘fwrite’, declared with attribute warn_unused_result [-Werror=unused-result] >> fwrite(buf, 1, size, replay_file); >> ^ >> >> This commit wraps the fwrite() call with the ignore_value() macro, which >> currently suppresses the error for existing GCC versions. > > This explains what you did, but not quite why. In other words, convince > me that ignoring the error is the right thing to do in the first place... Sure, that's why I've linked these two in a patch series. :) > >> >> Signed-off-by: Felipe Franciosi <felipe@nutanix.com> >> --- >> replay/replay-internal. | 0 >> replay/replay-internal.c | 2 +- >> 2 files changed, 1 insertion(+), 1 deletion(-) >> create mode 100644 replay/replay-internal. >> >> diff --git a/replay/replay-internal. b/replay/replay-internal. >> new file mode 100644 >> index 0000000..e69de29 >> diff --git a/replay/replay-internal.c b/replay/replay-internal.c >> index 5835e8d..61de8f9 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); >> + ignore_value(fwrite(buf, 1, size, replay_file)); > > I would be more convinced that this patch is correct if you added a > comment here why fwrite() failures can be ignored in this situation, so > that someone doesn't undo your commit to add in proper error handling. Right, so there are two things to be discussed here: 1) This patch merely fixes the build. It doesn't change the current behaviour. 2) We need to check whether fwrite() succeed. The real question is: are we happy with 1? Or do we want to go down route 2? Pavel already flagged that we should probably be checking whether fwrite() succeed. This is something I briefly mentioned in another e-mail [1], but then the direction got changed to use ignore_value() instead. [1] https://lists.nongnu.org/archive/html/qemu-devel/2016-09/msg05039.html If we want to go down route 2, we should arguably revisit the whole replay implementation. For instance, other calls around "replay_file" (such as putc, fseek, etc) are all ignored today. Also worth noting, several checks while reading from "replay_file" result in an error_report(), but without any special handling. Maybe the underlying idea is that if you lost the ability to write to (or seek) a file stream, you have bigger problems to worry about and your host is catastrophically failing in another way. Thoughts? Felipe > > -- > Eric Blake eblake redhat com +1-919-301-3266 > Libvirt virtualization library http://libvirt.org >
diff --git a/replay/replay-internal. b/replay/replay-internal. new file mode 100644 index 0000000..e69de29 diff --git a/replay/replay-internal.c b/replay/replay-internal.c index 5835e8d..61de8f9 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); + ignore_value(fwrite(buf, 1, size, replay_file)); } }
If building with GCC 3.4 or newer (and using -Werror=unused-result), replay-internal.c will fail to compile due to a call to fwrite() where the return value is not used. Since fwrite() is declared with WUR in glibc, callers should check the return value or find other ways to ignore it. The error message in this specific case is: replay/replay-internal.c: In function ‘replay_put_array’: replay/replay-internal.c:68:15: error: ignoring return value of ‘fwrite’, declared with attribute warn_unused_result [-Werror=unused-result] fwrite(buf, 1, size, replay_file); ^ This commit wraps the fwrite() call with the ignore_value() macro, which currently suppresses the error for existing GCC versions. Signed-off-by: Felipe Franciosi <felipe@nutanix.com> --- replay/replay-internal. | 0 replay/replay-internal.c | 2 +- 2 files changed, 1 insertion(+), 1 deletion(-) create mode 100644 replay/replay-internal.