Re: [Qemu-devel] [PATCH 03/10] qemu-ga: qmp_guest_file_*: improve error reporting

[Luiz Capitulino]

On Wed, 28 Nov 2012 11:54:45 -0600
mdroth <address@hidden> 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 <address@hidden>
> > ---
> >  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).