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

Daniel P. Berrange wrote: Pushed. 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: Jim Meyering <meyering(a)redhat.com&gt; 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