diff mbox

[03/10] qemu-ga: qmp_guest_file_*: improve error reporting

Message ID 1354021324-31561-4-git-send-email-lcapitulino@redhat.com
State New
Headers show

Commit Message

Luiz Capitulino Nov. 27, 2012, 1:01 p.m. UTC 
Use error_setg_errno() when possible with an improved error description.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 qga/commands-posix.c | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

Comments

Michael Roth Nov. 28, 2012, 5:54 p.m. UTC | #1 
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 <lcapitulino@redhat.com>
> ---
>  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.

<snip>
Luiz Capitulino Nov. 28, 2012, 7:31 p.m. UTC | #2 
On Wed, 28 Nov 2012 11:54:45 -0600
mdroth <mdroth@linux.vnet.ibm.com> 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 <lcapitulino@redhat.com>
> > ---
> >  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).
Eric Blake Nov. 28, 2012, 9:26 p.m. UTC | #3 
> > >      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

Correct that C99 doesn't require it, but POSIX _does_ require it.

Windows is the biggest system out there where errno is unreliable after
failure on FILE operations (but as we DO support mingw, we ARE impacted
by the lameness of Microsoft's C library being C89 but not POSIX).

> 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

Then you probably haven't experimented much with mingw :)
Michael Roth Nov. 28, 2012, 10:17 p.m. UTC | #4 
On Wed, Nov 28, 2012 at 04:26:29PM -0500, Eric Blake wrote:
> 
> > > >      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
> 
> Correct that C99 doesn't require it, but POSIX _does_ require it.
> 
> Windows is the biggest system out there where errno is unreliable after
> failure on FILE operations (but as we DO support mingw, we ARE impacted
> by the lameness of Microsoft's C library being C89 but not POSIX).

Well, if it's primarilly an issue with windows, then I think we're okay
relying on it for anything in qga/commands-posix.c at least, as those
implementations get swapped out for the implementations in
qga/commands-win32.o for win32/mingw builds. We'll need to be wary of
this in the future if we end up sharing more code with the win32 port
becomes more feature-full though.

The comment elsewhere about setmntent() might still apply however.

> 
> > 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
> 
> Then you probably haven't experimented much with mingw :)
>
Luiz Capitulino Nov. 29, 2012, noon UTC | #5 
On Wed, 28 Nov 2012 16:17:43 -0600
mdroth <mdroth@linux.vnet.ibm.com> wrote:

> On Wed, Nov 28, 2012 at 04:26:29PM -0500, Eric Blake wrote:
> > 
> > > > >      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
> > 
> > Correct that C99 doesn't require it, but POSIX _does_ require it.
> > 
> > Windows is the biggest system out there where errno is unreliable after
> > failure on FILE operations (but as we DO support mingw, we ARE impacted
> > by the lameness of Microsoft's C library being C89 but not POSIX).
> 
> Well, if it's primarilly an issue with windows, then I think we're okay
> relying on it for anything in qga/commands-posix.c at least, as those
> implementations get swapped out for the implementations in
> qga/commands-win32.o for win32/mingw builds. We'll need to be wary of
> this in the future if we end up sharing more code with the win32 port
> becomes more feature-full though.
> 
> The comment elsewhere about setmntent() might still apply however.

Ok, so can I respin only 05/10?

> > > 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
> > 
> > Then you probably haven't experimented much with mingw :)

Nope. But I was referring to other unixes...
diff mbox

Patch

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 {
         buf[read_count] = 0;
         read_data = g_malloc0(sizeof(GuestFileRead));
@@ -240,15 +243,16 @@  GuestFileWrite *qmp_guest_file_write(int64_t handle, const char *buf_b64,
     if (!has_count) {
         count = buf_len;
     } else if (count < 0 || count > buf_len) {
+        error_setg(err, "value '%" PRId64 "' is invalid for argument count",
+                   count);
         g_free(buf);
-        error_set(err, QERR_INVALID_PARAMETER, "count");
         return NULL;
     }
 
     write_count = fwrite(buf, 1, count, fh);
     if (ferror(fh)) {
+        error_setg_errno(err, errno, "failed to write to file");
         slog("guest-file-write failed, handle: %ld", handle);
-        error_set(err, QERR_QGA_COMMAND_FAILED, "fwrite() error");
     } else {
         write_data = g_malloc0(sizeof(GuestFileWrite));
         write_data->count = write_count;
@@ -275,7 +279,7 @@  struct GuestFileSeek *qmp_guest_file_seek(int64_t handle, int64_t offset,
     fh = gfh->fh;
     ret = fseek(fh, offset, whence);
     if (ret == -1) {
-        error_set(err, QERR_QGA_COMMAND_FAILED, strerror(errno));
+        error_setg_errno(err, errno, "failed to seek file");
     } else {
         seek_data = g_malloc0(sizeof(GuestFileRead));
         seek_data->position = ftell(fh);
@@ -299,7 +303,7 @@  void qmp_guest_file_flush(int64_t handle, Error **err)
     fh = gfh->fh;
     ret = fflush(fh);
     if (ret == EOF) {
-        error_set(err, QERR_QGA_COMMAND_FAILED, strerror(errno));
+        error_setg_errno(err, errno, "failed to flush file");
     }
 }