7:56 a.m.
Ignoring a write failure is a big deal, so when you do that deliberately,
it's worth marking in such a way that code analyzers don't report a
false positive. Until a year or so ago, people thought using (void)
would be enough. But newer gcc warns in spite of that (for functions
marked with the warn_unused_result attribute), so now we use the
ignore_value function from gnulib.
A coverity warning prompted the change below, but if we declare
saferead with the warn_unused_result attribute, gcc would, too.
From 62d2c7f69608407afa9a4bf38f0d29ea3eec88f5 Mon Sep 17 00:00:00 2001
From: Jim Meyering <meyering(a)redhat.com>
Date: Mon, 18 Jan 2010 11:51:01 +0100
Subject: [PATCH] logging: confirm that we want to ignore a write error
* src/util/logging.c (virLogMessage): Include "ignore-value.h".
Use it to ignore the return value of safewrite.
Use STDERR_FILENO, rather than "2".
* bootstrap (modules): Add ignore-value.
* gnulib: Update to latest, for ignore-value that is now LGPLv2+.
---
.gnulib | 2 +-
bootstrap | 1 +
src/util/logging.c | 5 +++--
3 files changed, 5 insertions(+), 3 deletions(-)
diff --git a/.gnulib b/.gnulib
index 4c52807..146d914 160000
--- a/.gnulib
+++ b/.gnulib
@@ -1 +1 @@
-Subproject commit 4c52807f41f238cf0e352317b2dc54f9ba0f0c4f
+Subproject commit 146d9145073e62a2096a2d6b33f75e93908fedf3
diff --git a/bootstrap b/bootstrap
index c07d851..aec5d05 100755
--- a/bootstrap
+++ b/bootstrap
@@ -76,6 +76,7 @@ getpass
gettext
gitlog-to-changelog
gnumakefile
+ignore-value
inet_pton
ioctl
maintainer-makefile
diff --git a/src/util/logging.c b/src/util/logging.c
index 6bd8469..3b3c309 100644
--- a/src/util/logging.c
+++ b/src/util/logging.c
@@ -1,7 +1,7 @@
/*
* logging.c: internal logging and debugging
*
- * Copyright (C) 2008 Red Hat, Inc.
+ * Copyright (C) 2008, 2010 Red Hat, Inc.
*
* This library is free software; you can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public
@@ -34,6 +34,7 @@
#include <syslog.h>
#endif
+#include "ignore-value.h"
#include "logging.h"
#include "memory.h"
#include "util.h"
@@ -579,7 +580,7 @@ void virLogMessage(const char *category, int priority, const char
*funcname,
msg, len, virLogOutputs[i].data);
}
if ((virLogNbOutputs == 0) && (flags != 1))
- safewrite(2, msg, len);
+ ignore_value (safewrite(STDERR_FILENO, msg, len));
virLogUnlock();
VIR_FREE(msg);
--
1.6.6.638.g2bc54
1:33 p.m.
On Mon, Jan 18, 2010 at 02:56:36PM +0100, Jim Meyering wrote:
Ignoring a write failure is a big deal, so when you do that
deliberately,
it's worth marking in such a way that code analyzers don't report a
false positive. Until a year or so ago, people thought using (void)
would be enough. But newer gcc warns in spite of that (for functions
marked with the warn_unused_result attribute), so now we use the
ignore_value function from gnulib.
A coverity warning prompted the change below, but if we declare
saferead with the warn_unused_result attribute, gcc would, too.
>From 62d2c7f69608407afa9a4bf38f0d29ea3eec88f5 Mon Sep 17 00:00:00 2001
From: Jim Meyering <meyering(a)redhat.com>
Date: Mon, 18 Jan 2010 11:51:01 +0100
Subject: [PATCH] logging: confirm that we want to ignore a write error
* src/util/logging.c (virLogMessage): Include "ignore-value.h".
Use it to ignore the return value of safewrite.
Use STDERR_FILENO, rather than "2".
* bootstrap (modules): Add ignore-value.
* gnulib: Update to latest, for ignore-value that is now LGPLv2+.
---
.gnulib | 2 +-
bootstrap | 1 +
src/util/logging.c | 5 +++--
3 files changed, 5 insertions(+), 3 deletions(-)
diff --git a/.gnulib b/.gnulib
index 4c52807..146d914 160000
--- a/.gnulib
+++ b/.gnulib
@@ -1 +1 @@
-Subproject commit 4c52807f41f238cf0e352317b2dc54f9ba0f0c4f
+Subproject commit 146d9145073e62a2096a2d6b33f75e93908fedf3
diff --git a/bootstrap b/bootstrap
index c07d851..aec5d05 100755
--- a/bootstrap
+++ b/bootstrap
@@ -76,6 +76,7 @@ getpass
gettext
gitlog-to-changelog
gnumakefile
+ignore-value
inet_pton
ioctl
maintainer-makefile
diff --git a/src/util/logging.c b/src/util/logging.c
index 6bd8469..3b3c309 100644
--- a/src/util/logging.c
+++ b/src/util/logging.c
@@ -1,7 +1,7 @@
/*
* logging.c: internal logging and debugging
*
- * Copyright (C) 2008 Red Hat, Inc.
+ * Copyright (C) 2008, 2010 Red Hat, Inc.
*
* This library is free software; you can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public
@@ -34,6 +34,7 @@
#include <syslog.h>
#endif
+#include "ignore-value.h"
#include "logging.h"
#include "memory.h"
#include "util.h"
@@ -579,7 +580,7 @@ void virLogMessage(const char *category, int priority, const char
*funcname,
msg, len, virLogOutputs[i].data);
}
if ((virLogNbOutputs == 0) && (flags != 1))
- safewrite(2, msg, len);
+ ignore_value (safewrite(STDERR_FILENO, msg, len));
virLogUnlock();
VIR_FREE(msg);
--
ACK, took me a while to understand why 'ignore_value' works, but it is
worth it. We should add ATTRIBUTE_RETURN_CHECK to safewrite() sometime
soon too.
Daniel
--
|: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
2:30 p.m.
Daniel P. Berrange wrote:
On Mon, Jan 18, 2010 at 02:56:36PM +0100, Jim Meyering wrote:
> Ignoring a write failure is a big deal, so when you do that deliberately,
> it's worth marking in such a way that code analyzers don't report a
> false positive. Until a year or so ago, people thought using (void)
> would be enough. But newer gcc warns in spite of that (for functions
> marked with the warn_unused_result attribute), so now we use the
> ignore_value function from gnulib.
Pushed.
> A coverity warning prompted the change below, but if we declare
> saferead with the warn_unused_result attribute, gcc would, too.
ACK, took me a while to understand why 'ignore_value' works,
but it is
worth it. We should add ATTRIBUTE_RETURN_CHECK to safewrite() sometime
soon too.
I've gone ahead and marked a few "util.h" functions with that
attribute. This was the first bit of fall-outP
../daemon/event.c: In function 'virEventHandleWakeup':
../daemon/event.c:633:13: warning: ignoring return value of 'saferead',\
declared with attribute warn_unused_result
That's happening in an void-returning function.
Personally, I'd like to know if there's a problem that makes
the read return EBADF, ENOMEM, etc. immediately. But
even if we detect that, then what? Just log it?
Or do you want to change the entire structure of the
code for this one improbable event.
In the patch below, I propose to ignore it.
Here are two more new warnings, one for a read, and another
for a companion write:
remote/remote_driver.c: In function 'remoteIO':
remote/remote_driver.c:8444:18: warning: ignoring return value of 'safewrite',\
declared with attribute warn_unused_result
remote/remote_driver.c: In function 'remoteIOEventLoop':
remote/remote_driver.c:8300:21: warning: ignoring return value of 'saferead',\
declared with attribute warn_unused_result
FYI, here are the two calls:
/*
* Process all calls pending dispatch/receive until we
* get a reply to our own call. Then quit and pass the buck
* to someone else.
*/
static int
remoteIOEventLoop(virConnectPtr conn,
struct private_data *priv,
int in_open,
struct remote_thread_call *thiscall)
{
struct pollfd fds[2];
int ret;
fds[0].fd = priv->sock;
fds[1].fd = priv->wakeupReadFD;
for (;;) {
...
if (fds[1].revents) {
DEBUG0("Woken up from poll by other thread");
saferead(priv->wakeupReadFD, &ignore, sizeof(ignore));
<<========
}
...
static int
remoteIO(virConnectPtr conn,
struct private_data *priv,
int flags,
struct remote_thread_call *thiscall)
{
...
/* Force other thread to wakup from poll */
safewrite(priv->wakeupSendFD, &ignore, sizeof(ignore)); <<========
DEBUG("Going to sleep %d %p %p", thiscall->proc_nr,
priv->waitDispatch, thiscall);
/* Go to sleep while other thread is working... */
if (virCondWait(&thiscall->cond, &priv->lock) < 0) {
if (priv->waitDispatch == thiscall) {
--------------------------------------------------------------
Here is a patch to add the attributes
and deal with the warning in event.c:
From 672f18c84529e9b4069a8a0dd0d81c111f49b47a Mon Sep 17 00:00:00 2001
From: Jim Meyering <meyering(a)redhat.com>
Date: Tue, 19 Jan 2010 21:00:34 +0100
Subject: [PATCH] maint: require that each safewrite return value be used, ...
...and mark more functions with ATTRIBUTE_RETURN_CHECK.
* src/util/util.h (safewrite): Declare with ATTRIBUTE_RETURN_CHECK.
(saferead, safezero, virParseNumber): Likewise.
* daemon/event.c: Include "ignore-value.h".
(virEventHandleWakeup): Explicitly ignore saferead return value.
---
daemon/event.c | 5 +++--
src/util/util.h | 8 ++++----
2 files changed, 7 insertions(+), 6 deletions(-)
diff --git a/daemon/event.c b/daemon/event.c
index 2218a3e..2285dd7 100644
--- a/daemon/event.c
+++ b/daemon/event.c
@@ -2,7 +2,7 @@
* event.c: event loop for monitoring file handles
*
* Copyright (C) 2007 Daniel P. Berrange
- * Copyright (C) 2007 Red Hat, Inc.
+ * Copyright (C) 2007, 2010 Red Hat, Inc.
*
* This library is free software; you can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public
@@ -30,6 +30,7 @@
#include <errno.h>
#include <unistd.h>
+#include "ignore-value.h"
#include "threads.h"
#include "logging.h"
#include "event.h"
@@ -630,7 +631,7 @@ static void virEventHandleWakeup(int watch ATTRIBUTE_UNUSED,
{
char c;
virEventLock();
- saferead(fd, &c, sizeof(c));
+ ignore_value (saferead(fd, &c, sizeof(c)));
virEventUnlock();
}
diff --git a/src/util/util.h b/src/util/util.h
index d556daa..ab9c6a1 100644
--- a/src/util/util.h
+++ b/src/util/util.h
@@ -31,9 +31,9 @@
#include <sys/select.h>
#include <sys/types.h>
-int saferead(int fd, void *buf, size_t count);
-ssize_t safewrite(int fd, const void *buf, size_t count);
-int safezero(int fd, int flags, off_t offset, off_t len);
+int saferead(int fd, void *buf, size_t count) ATTRIBUTE_RETURN_CHECK;
+ssize_t safewrite(int fd, const void *buf, size_t count) ATTRIBUTE_RETURN_CHECK;
+int safezero(int fd, int flags, off_t offset, off_t len) ATTRIBUTE_RETURN_CHECK;
enum {
VIR_EXEC_NONE = 0,
@@ -166,7 +166,7 @@ int virStrToDouble(char const *s,
int virMacAddrCompare (const char *mac1, const char *mac2);
void virSkipSpaces(const char **str);
-int virParseNumber(const char **str);
+int virParseNumber(const char **str) ATTRIBUTE_RETURN_CHECK;
int virAsprintf(char **strp, const char *fmt, ...)
ATTRIBUTE_FMT_PRINTF(2, 3) ATTRIBUTE_RETURN_CHECK;
char *virStrncpy(char *dest, const char *src, size_t n, size_t destbytes)
--
1.6.6.444.g38f14
5451
days inactive
5452
days old
2 comments
2 participants
participants (2)
-
Daniel P. Berrange -
Jim Meyering