From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:43421) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TdnMa-0005R4-4w for qemu-devel@nongnu.org; Wed, 28 Nov 2012 14:32:08 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TdnMY-0007V9-Vm for qemu-devel@nongnu.org; Wed, 28 Nov 2012 14:32:04 -0500 Received: from mx1.redhat.com ([209.132.183.28]:13344) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TdnMY-0007V5-Nk for qemu-devel@nongnu.org; Wed, 28 Nov 2012 14:32:02 -0500 Date: Wed, 28 Nov 2012 17:31:55 -0200 From: Luiz Capitulino Message-ID: <20121128173155.7612b1a4@doriath.home> In-Reply-To: <20121128175445.GE8690@vm> References: <1354021324-31561-1-git-send-email-lcapitulino@redhat.com> <1354021324-31561-4-git-send-email-lcapitulino@redhat.com> <20121128175445.GE8690@vm> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 03/10] qemu-ga: qmp_guest_file_*: improve error reporting List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: mdroth Cc: qemu-devel@nongnu.org On Wed, 28 Nov 2012 11:54:45 -0600 mdroth wrote: > On Tue, Nov 27, 2012 at 11:01:57AM -0200, Luiz Capitulino wrote: > > Use error_setg_errno() when possible with an improved error description. > > > > Signed-off-by: Luiz Capitulino > > --- > > qga/commands-posix.c | 22 +++++++++++++--------- > > 1 file changed, 13 insertions(+), 9 deletions(-) > > > > diff --git a/qga/commands-posix.c b/qga/commands-posix.c > > index c284083..92fc550 100644 > > --- a/qga/commands-posix.c > > +++ b/qga/commands-posix.c > > @@ -138,7 +138,8 @@ int64_t qmp_guest_file_open(const char *path, bool has_mode, const char *mode, E > > slog("guest-file-open called, filepath: %s, mode: %s", path, mode); > > fh = fopen(path, mode); > > if (!fh) { > > - error_set(err, QERR_OPEN_FILE_FAILED, path); > > + error_setg_errno(err, errno, "failed to open file '%s' (mode: '%s')", > > + path, mode); > > return -1; > > } > > > > @@ -149,7 +150,8 @@ int64_t qmp_guest_file_open(const char *path, bool has_mode, const char *mode, E > > ret = fcntl(fd, F_GETFL); > > ret = fcntl(fd, F_SETFL, ret | O_NONBLOCK); > > if (ret == -1) { > > - error_set(err, QERR_QGA_COMMAND_FAILED, "fcntl() failed"); > > + error_setg_errno(err, errno, "failed to make file '%s' non-blocking", > > + path); > > fclose(fh); > > return -1; > > } > > @@ -171,7 +173,7 @@ void qmp_guest_file_close(int64_t handle, Error **err) > > > > ret = fclose(gfh->fh); > > if (ret == EOF) { > > - error_set(err, QERR_QGA_COMMAND_FAILED, "fclose() failed"); > > + error_setg_errno(err, errno, "failed to close handle"); > > return; > > } > > > > @@ -195,7 +197,8 @@ struct GuestFileRead *qmp_guest_file_read(int64_t handle, bool has_count, > > if (!has_count) { > > count = QGA_READ_COUNT_DEFAULT; > > } else if (count < 0) { > > - error_set(err, QERR_INVALID_PARAMETER, "count"); > > + error_setg(err, "value '%" PRId64 "' is invalid for argument count", > > + count); > > return NULL; > > } > > > > @@ -203,8 +206,8 @@ struct GuestFileRead *qmp_guest_file_read(int64_t handle, bool has_count, > > buf = g_malloc0(count+1); > > read_count = fread(buf, 1, count, fh); > > if (ferror(fh)) { > > + error_setg_errno(err, errno, "failed to read file"); > > slog("guest-file-read failed, handle: %ld", handle); > > - error_set(err, QERR_QGA_COMMAND_FAILED, "fread() failed"); > > } else { > > I'm not sure about relying on errno for FILE/f*() functions. C99 doesn't > appear to require setting it for implementations (unfortunately), and > glibc doesn't document it for fwrite/fread (although it does for most others). > I'm guessing it's okay for glibc, but there be cases where we print random > error messages. We can do this portably by setting errno = 0 before the > fread()/f*()s, and using error_setg() if it's still 0 after we detect an > error, but that's pretty nasty. > > Unless we can confirm there's aren't any implementations out there we > care about where errno isn't set, maybe we should just stick to > error_setg() for the f* functions? Hate to throw away error messages, but > incorrect/random errors would be worse. fread() and fwrite() are used only once, so I think it would be fine to do what you suggest above wrt setting errno to 0 and checking it after the call. However, the other FILE functions seem safe to me. I'd be very surprised if some implementation doesn't set errno on fopen() failure for example (actually, I'd also be surprised about fread()/fwrite() doing this, but as this is not documented I'd agree on being cautious).