[libvirt] [PATCH] logging: confirm that we want to ignore a write error

[Jim Meyering]

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 at 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