[Qemu-devel] [PATCH 00/10] qemu-ga: revamp error messages (for 1.4)

[Qemu-devel] [PATCH 00/10] qemu-ga: revamp error messages (for 1.4)

[Luiz Capitulino]

This series revamps qemu-ga error messages by:

 - Propagating errors down to functions that generate errors
 - Fixing UndefinedErrors
 - Getting rid of sprint() + error_set()
 - Other small fixes

Only the POSIX port is covered in this series, win32 is left to another day.

Luiz Capitulino (10):
  qemu-ga: guest_file_handle_find(): take an Error argument
  qemu-ga: qmp_guest_file_close(): fix fclose() error check
  qemu-ga: qmp_guest_file_*: improve error reporting
  qemu-ga: qmp_guest_shutdown(): improve error reporting
  qemu-ga: build_fs_mount_list(): take an Error argument
  qemu-ga: qmp_guest_fsfreeze_*(): get rid of sprintf() + error_set()
  qemu-ga: qmp_guest_fstrim(): get rid of sprintf() + error_set()
  qemu-ga: qmp_guest_network_get_interfaces(): get rid of snprintf() +
    error_set()
  qemu-ga: bios_supports_mode(): improve error reporting
  qemu-ga: guest_suspend(): improve error reporting

 qga/commands-posix.c | 244 ++++++++++++++++++++++++++++-----------------------
 1 file changed, 134 insertions(+), 110 deletions(-)

-- 
1.8.0

[Qemu-devel] [PATCH 01/10] qemu-ga: guest_file_handle_find(): take an Error argument

[Luiz Capitulino]

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

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 726930a..a2216f2 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -111,7 +111,7 @@ static void guest_file_handle_add(FILE *fh)
     QTAILQ_INSERT_TAIL(&guest_file_state.filehandles, gfh, next);
 }
 
-static GuestFileHandle *guest_file_handle_find(int64_t id)
+static GuestFileHandle *guest_file_handle_find(int64_t id, Error **err)
 {
     GuestFileHandle *gfh;
 
@@ -122,6 +122,7 @@ static GuestFileHandle *guest_file_handle_find(int64_t id)
         }
     }
 
+    error_setg(err, "handle '%" PRId64 "'has not been found", id);
     return NULL;
 }
 
@@ -160,12 +161,11 @@ int64_t qmp_guest_file_open(const char *path, bool has_mode, const char *mode, E
 
 void qmp_guest_file_close(int64_t handle, Error **err)
 {
-    GuestFileHandle *gfh = guest_file_handle_find(handle);
+    GuestFileHandle *gfh = guest_file_handle_find(handle, err);
     int ret;
 
     slog("guest-file-close called, handle: %ld", handle);
     if (!gfh) {
-        error_set(err, QERR_FD_NOT_FOUND, "handle");
         return;
     }
 
@@ -182,14 +182,13 @@ void qmp_guest_file_close(int64_t handle, Error **err)
 struct GuestFileRead *qmp_guest_file_read(int64_t handle, bool has_count,
                                           int64_t count, Error **err)
 {
-    GuestFileHandle *gfh = guest_file_handle_find(handle);
+    GuestFileHandle *gfh = guest_file_handle_find(handle, err);
     GuestFileRead *read_data = NULL;
     guchar *buf;
     FILE *fh;
     size_t read_count;
 
     if (!gfh) {
-        error_set(err, QERR_FD_NOT_FOUND, "handle");
         return NULL;
     }
 
@@ -228,11 +227,10 @@ GuestFileWrite *qmp_guest_file_write(int64_t handle, const char *buf_b64,
     guchar *buf;
     gsize buf_len;
     int write_count;
-    GuestFileHandle *gfh = guest_file_handle_find(handle);
+    GuestFileHandle *gfh = guest_file_handle_find(handle, err);
     FILE *fh;
 
     if (!gfh) {
-        error_set(err, QERR_FD_NOT_FOUND, "handle");
         return NULL;
     }
 
@@ -265,13 +263,12 @@ GuestFileWrite *qmp_guest_file_write(int64_t handle, const char *buf_b64,
 struct GuestFileSeek *qmp_guest_file_seek(int64_t handle, int64_t offset,
                                           int64_t whence, Error **err)
 {
-    GuestFileHandle *gfh = guest_file_handle_find(handle);
+    GuestFileHandle *gfh = guest_file_handle_find(handle, err);
     GuestFileSeek *seek_data = NULL;
     FILE *fh;
     int ret;
 
     if (!gfh) {
-        error_set(err, QERR_FD_NOT_FOUND, "handle");
         return NULL;
     }
 
@@ -291,12 +288,11 @@ struct GuestFileSeek *qmp_guest_file_seek(int64_t handle, int64_t offset,
 
 void qmp_guest_file_flush(int64_t handle, Error **err)
 {
-    GuestFileHandle *gfh = guest_file_handle_find(handle);
+    GuestFileHandle *gfh = guest_file_handle_find(handle, err);
     FILE *fh;
     int ret;
 
     if (!gfh) {
-        error_set(err, QERR_FD_NOT_FOUND, "handle");
         return;
     }
 
-- 
1.8.0

Re: [Qemu-devel] [PATCH 01/10] qemu-ga: guest_file_handle_find(): take an Error argument

[mdroth]

On Tue, Nov 27, 2012 at 11:01:55AM -0200, Luiz Capitulino wrote:
> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>

Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com>

> ---
>  qga/commands-posix.c | 18 +++++++-----------
>  1 file changed, 7 insertions(+), 11 deletions(-)
> 
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index 726930a..a2216f2 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -111,7 +111,7 @@ static void guest_file_handle_add(FILE *fh)
>      QTAILQ_INSERT_TAIL(&guest_file_state.filehandles, gfh, next);
>  }
> 
> -static GuestFileHandle *guest_file_handle_find(int64_t id)
> +static GuestFileHandle *guest_file_handle_find(int64_t id, Error **err)
>  {
>      GuestFileHandle *gfh;
> 
> @@ -122,6 +122,7 @@ static GuestFileHandle *guest_file_handle_find(int64_t id)
>          }
>      }
> 
> +    error_setg(err, "handle '%" PRId64 "'has not been found", id);
>      return NULL;
>  }
> 
> @@ -160,12 +161,11 @@ int64_t qmp_guest_file_open(const char *path, bool has_mode, const char *mode, E
> 
>  void qmp_guest_file_close(int64_t handle, Error **err)
>  {
> -    GuestFileHandle *gfh = guest_file_handle_find(handle);
> +    GuestFileHandle *gfh = guest_file_handle_find(handle, err);
>      int ret;
> 
>      slog("guest-file-close called, handle: %ld", handle);
>      if (!gfh) {
> -        error_set(err, QERR_FD_NOT_FOUND, "handle");
>          return;
>      }
> 
> @@ -182,14 +182,13 @@ void qmp_guest_file_close(int64_t handle, Error **err)
>  struct GuestFileRead *qmp_guest_file_read(int64_t handle, bool has_count,
>                                            int64_t count, Error **err)
>  {
> -    GuestFileHandle *gfh = guest_file_handle_find(handle);
> +    GuestFileHandle *gfh = guest_file_handle_find(handle, err);
>      GuestFileRead *read_data = NULL;
>      guchar *buf;
>      FILE *fh;
>      size_t read_count;
> 
>      if (!gfh) {
> -        error_set(err, QERR_FD_NOT_FOUND, "handle");
>          return NULL;
>      }
> 
> @@ -228,11 +227,10 @@ GuestFileWrite *qmp_guest_file_write(int64_t handle, const char *buf_b64,
>      guchar *buf;
>      gsize buf_len;
>      int write_count;
> -    GuestFileHandle *gfh = guest_file_handle_find(handle);
> +    GuestFileHandle *gfh = guest_file_handle_find(handle, err);
>      FILE *fh;
> 
>      if (!gfh) {
> -        error_set(err, QERR_FD_NOT_FOUND, "handle");
>          return NULL;
>      }
> 
> @@ -265,13 +263,12 @@ GuestFileWrite *qmp_guest_file_write(int64_t handle, const char *buf_b64,
>  struct GuestFileSeek *qmp_guest_file_seek(int64_t handle, int64_t offset,
>                                            int64_t whence, Error **err)
>  {
> -    GuestFileHandle *gfh = guest_file_handle_find(handle);
> +    GuestFileHandle *gfh = guest_file_handle_find(handle, err);
>      GuestFileSeek *seek_data = NULL;
>      FILE *fh;
>      int ret;
> 
>      if (!gfh) {
> -        error_set(err, QERR_FD_NOT_FOUND, "handle");
>          return NULL;
>      }
> 
> @@ -291,12 +288,11 @@ struct GuestFileSeek *qmp_guest_file_seek(int64_t handle, int64_t offset,
> 
>  void qmp_guest_file_flush(int64_t handle, Error **err)
>  {
> -    GuestFileHandle *gfh = guest_file_handle_find(handle);
> +    GuestFileHandle *gfh = guest_file_handle_find(handle, err);
>      FILE *fh;
>      int ret;
> 
>      if (!gfh) {
> -        error_set(err, QERR_FD_NOT_FOUND, "handle");
>          return;
>      }
> 
> -- 
> 1.8.0
> 

[Qemu-devel] [PATCH 02/10] qemu-ga: qmp_guest_file_close(): fix fclose() error check

[Luiz Capitulino]

fclose() returns EOF on error.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 qga/commands-posix.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index a2216f2..c284083 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -170,7 +170,7 @@ void qmp_guest_file_close(int64_t handle, Error **err)
     }
 
     ret = fclose(gfh->fh);
-    if (ret == -1) {
+    if (ret == EOF) {
         error_set(err, QERR_QGA_COMMAND_FAILED, "fclose() failed");
         return;
     }
-- 
1.8.0

Re: [Qemu-devel] [PATCH 02/10] qemu-ga: qmp_guest_file_close(): fix fclose() error check

[mdroth]

On Tue, Nov 27, 2012 at 11:01:56AM -0200, Luiz Capitulino wrote:
> fclose() returns EOF on error.
> 
> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>

Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com>

> ---
>  qga/commands-posix.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index a2216f2..c284083 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -170,7 +170,7 @@ void qmp_guest_file_close(int64_t handle, Error **err)
>      }
> 
>      ret = fclose(gfh->fh);
> -    if (ret == -1) {
> +    if (ret == EOF) {
>          error_set(err, QERR_QGA_COMMAND_FAILED, "fclose() failed");
>          return;
>      }
> -- 
> 1.8.0
> 

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

[Luiz Capitulino]

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 {
         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");
     }
 }
 
-- 
1.8.0

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

[mdroth]

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>

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 <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).

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

[Eric Blake]

> > >      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 :)

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

[mdroth]

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 :)
> 

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

[Luiz Capitulino]

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

[Qemu-devel] [PATCH 04/10] qemu-ga: qmp_guest_shutdown(): improve error reporting

[Luiz Capitulino]

Most errors are QERR_UNDEFINED_ERROR. Also, adds ga_wait_child() as
a future commit will use it too.

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

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 92fc550..b3a3f26 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -46,10 +46,29 @@ extern char **environ;
 #endif
 #endif
 
+static void ga_wait_child(pid_t pid, int *status, Error **err)
+{
+    pid_t rpid;
+
+    *status = 0;
+
+    do {
+        rpid = waitpid(pid, status, 0);
+    } while (rpid == -1 && errno == EINTR);
+
+    if (rpid == -1) {
+        error_setg_errno(err, errno, "failed to wait for child (pid: %d)", pid);
+        return;
+    }
+
+    g_assert(rpid == pid);
+}
+
 void qmp_guest_shutdown(bool has_mode, const char *mode, Error **err)
 {
     const char *shutdown_flag;
-    pid_t rpid, pid;
+    Error *local_err = NULL;
+    pid_t pid;
     int status;
 
     slog("guest-shutdown called, mode: %s", mode);
@@ -60,8 +79,8 @@ void qmp_guest_shutdown(bool has_mode, const char *mode, Error **err)
     } else if (strcmp(mode, "reboot") == 0) {
         shutdown_flag = "-r";
     } else {
-        error_set(err, QERR_INVALID_PARAMETER_VALUE, "mode",
-                  "halt|powerdown|reboot");
+        error_setg(err,
+                   "mode is invalid (valid values are: halt|powerdown|reboot");
         return;
     }
 
@@ -77,18 +96,27 @@ void qmp_guest_shutdown(bool has_mode, const char *mode, Error **err)
                "hypervisor initiated shutdown", (char*)NULL, environ);
         _exit(EXIT_FAILURE);
     } else if (pid < 0) {
-        goto exit_err;
+        error_setg_errno(err, errno, "failed to create child process");
+        return;
     }
 
-    do {
-        rpid = waitpid(pid, &status, 0);
-    } while (rpid == -1 && errno == EINTR);
-    if (rpid == pid && WIFEXITED(status) && !WEXITSTATUS(status)) {
+    ga_wait_child(pid, &status, &local_err);
+    if (error_is_set(&local_err)) {
+        error_propagate(err, local_err);
         return;
     }
 
-exit_err:
-    error_set(err, QERR_UNDEFINED_ERROR);
+    if (!WIFEXITED(status)) {
+        error_setg(err, "child process has terminated abnormally");
+        return;
+    }
+
+    if (WEXITSTATUS(status)) {
+        error_setg(err, "child process has failed to shutdown");
+        return;
+    }
+
+    /* succeded */
 }
 
 typedef struct GuestFileHandle {
-- 
1.8.0

Re: [Qemu-devel] [PATCH 04/10] qemu-ga: qmp_guest_shutdown(): improve error reporting

[mdroth]

On Tue, Nov 27, 2012 at 11:01:58AM -0200, Luiz Capitulino wrote:
> Most errors are QERR_UNDEFINED_ERROR. Also, adds ga_wait_child() as
> a future commit will use it too.
> 
> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>

Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com>

> ---
>  qga/commands-posix.c | 48 ++++++++++++++++++++++++++++++++++++++----------
>  1 file changed, 38 insertions(+), 10 deletions(-)
> 
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index 92fc550..b3a3f26 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -46,10 +46,29 @@ extern char **environ;
>  #endif
>  #endif
> 
> +static void ga_wait_child(pid_t pid, int *status, Error **err)
> +{
> +    pid_t rpid;
> +
> +    *status = 0;
> +
> +    do {
> +        rpid = waitpid(pid, status, 0);
> +    } while (rpid == -1 && errno == EINTR);
> +
> +    if (rpid == -1) {
> +        error_setg_errno(err, errno, "failed to wait for child (pid: %d)", pid);
> +        return;
> +    }
> +
> +    g_assert(rpid == pid);
> +}
> +
>  void qmp_guest_shutdown(bool has_mode, const char *mode, Error **err)
>  {
>      const char *shutdown_flag;
> -    pid_t rpid, pid;
> +    Error *local_err = NULL;
> +    pid_t pid;
>      int status;
> 
>      slog("guest-shutdown called, mode: %s", mode);
> @@ -60,8 +79,8 @@ void qmp_guest_shutdown(bool has_mode, const char *mode, Error **err)
>      } else if (strcmp(mode, "reboot") == 0) {
>          shutdown_flag = "-r";
>      } else {
> -        error_set(err, QERR_INVALID_PARAMETER_VALUE, "mode",
> -                  "halt|powerdown|reboot");
> +        error_setg(err,
> +                   "mode is invalid (valid values are: halt|powerdown|reboot");
>          return;
>      }
> 
> @@ -77,18 +96,27 @@ void qmp_guest_shutdown(bool has_mode, const char *mode, Error **err)
>                 "hypervisor initiated shutdown", (char*)NULL, environ);
>          _exit(EXIT_FAILURE);
>      } else if (pid < 0) {
> -        goto exit_err;
> +        error_setg_errno(err, errno, "failed to create child process");
> +        return;
>      }
> 
> -    do {
> -        rpid = waitpid(pid, &status, 0);
> -    } while (rpid == -1 && errno == EINTR);
> -    if (rpid == pid && WIFEXITED(status) && !WEXITSTATUS(status)) {
> +    ga_wait_child(pid, &status, &local_err);
> +    if (error_is_set(&local_err)) {
> +        error_propagate(err, local_err);
>          return;
>      }
> 
> -exit_err:
> -    error_set(err, QERR_UNDEFINED_ERROR);
> +    if (!WIFEXITED(status)) {
> +        error_setg(err, "child process has terminated abnormally");
> +        return;
> +    }
> +
> +    if (WEXITSTATUS(status)) {
> +        error_setg(err, "child process has failed to shutdown");
> +        return;
> +    }
> +
> +    /* succeded */
>  }
> 
>  typedef struct GuestFileHandle {
> -- 
> 1.8.0
> 

[Qemu-devel] [PATCH 05/10] qemu-ga: build_fs_mount_list(): take an Error argument

[Luiz Capitulino]

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

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index b3a3f26..59bb32d 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -371,7 +371,7 @@ static void free_fs_mount_list(FsMountList *mounts)
 /*
  * Walk the mount table and build a list of local file systems
  */
-static int build_fs_mount_list(FsMountList *mounts)
+static void build_fs_mount_list(FsMountList *mounts, Error **err)
 {
     struct mntent *ment;
     FsMount *mount;
@@ -380,8 +380,8 @@ static int build_fs_mount_list(FsMountList *mounts)
 
     fp = setmntent(mtab, "r");
     if (!fp) {
-        g_warning("fsfreeze: unable to read mtab");
-        return -1;
+        error_setg_errno(err, errno, "failed to open mtab file: '%s'", mtab);
+        return;
     }
 
     while ((ment = getmntent(fp))) {
@@ -405,8 +405,6 @@ static int build_fs_mount_list(FsMountList *mounts)
     }
 
     endmntent(fp);
-
-    return 0;
 }
 #endif
 
@@ -433,15 +431,17 @@ int64_t qmp_guest_fsfreeze_freeze(Error **err)
     int ret = 0, i = 0;
     FsMountList mounts;
     struct FsMount *mount;
+    Error *local_err = NULL;
     int fd;
     char err_msg[512];
 
     slog("guest-fsfreeze called");
 
     QTAILQ_INIT(&mounts);
-    ret = build_fs_mount_list(&mounts);
-    if (ret < 0) {
-        return ret;
+    build_fs_mount_list(&mounts, &local_err);
+    if (error_is_set(&local_err)) {
+        error_propagate(err, local_err);
+        return -1;
     }
 
     /* cannot risk guest agent blocking itself on a write in this state */
@@ -498,12 +498,12 @@ int64_t qmp_guest_fsfreeze_thaw(Error **err)
     FsMountList mounts;
     FsMount *mount;
     int fd, i = 0, logged;
+    Error *local_err = NULL;
 
     QTAILQ_INIT(&mounts);
-    ret = build_fs_mount_list(&mounts);
-    if (ret) {
-        error_set(err, QERR_QGA_COMMAND_FAILED,
-                  "failed to enumerate filesystems");
+    build_fs_mount_list(&mounts, &local_err);
+    if (error_is_set(&local_err)) {
+        error_propagate(err, local_err);
         return 0;
     }
 
@@ -568,6 +568,7 @@ void qmp_guest_fstrim(bool has_minimum, int64_t minimum, Error **err)
     FsMountList mounts;
     struct FsMount *mount;
     int fd;
+    Error *local_err = NULL;
     char err_msg[512];
     struct fstrim_range r = {
         .start = 0,
@@ -578,8 +579,9 @@ void qmp_guest_fstrim(bool has_minimum, int64_t minimum, Error **err)
     slog("guest-fstrim called");
 
     QTAILQ_INIT(&mounts);
-    ret = build_fs_mount_list(&mounts);
-    if (ret < 0) {
+    build_fs_mount_list(&mounts, &local_err);
+    if (error_is_set(&local_err)) {
+        error_propagate(err, local_err);
         return;
     }
 
-- 
1.8.0

Re: [Qemu-devel] [PATCH 05/10] qemu-ga: build_fs_mount_list(): take an Error argument

[mdroth]

On Tue, Nov 27, 2012 at 11:01:59AM -0200, Luiz Capitulino wrote:
> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> ---
>  qga/commands-posix.c | 30 ++++++++++++++++--------------
>  1 file changed, 16 insertions(+), 14 deletions(-)
> 
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index b3a3f26..59bb32d 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -371,7 +371,7 @@ static void free_fs_mount_list(FsMountList *mounts)
>  /*
>   * Walk the mount table and build a list of local file systems
>   */
> -static int build_fs_mount_list(FsMountList *mounts)
> +static void build_fs_mount_list(FsMountList *mounts, Error **err)
>  {
>      struct mntent *ment;
>      FsMount *mount;
> @@ -380,8 +380,8 @@ static int build_fs_mount_list(FsMountList *mounts)
> 
>      fp = setmntent(mtab, "r");
>      if (!fp) {
> -        g_warning("fsfreeze: unable to read mtab");
> -        return -1;
> +        error_setg_errno(err, errno, "failed to open mtab file: '%s'", mtab);
> +        return;

I don't see anything about errno getting set in the glibc documentation. Can
we confirm? Otherwise error_setg() is probably sufficient here.

<snip>

> 
> -- 
> 1.8.0
> 

[Qemu-devel] [PATCH v2 05/10] qemu-ga: build_fs_mount_list(): take an Error argument

[Luiz Capitulino]

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---

o v2: use error_setg() in build_fs_mount_list()

 qga/commands-posix.c | 30 ++++++++++++++++--------------
 1 file changed, 16 insertions(+), 14 deletions(-)

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index b3a3f26..ff14174 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -371,7 +371,7 @@ static void free_fs_mount_list(FsMountList *mounts)
 /*
  * Walk the mount table and build a list of local file systems
  */
-static int build_fs_mount_list(FsMountList *mounts)
+static void build_fs_mount_list(FsMountList *mounts, Error **err)
 {
     struct mntent *ment;
     FsMount *mount;
@@ -380,8 +380,8 @@ static int build_fs_mount_list(FsMountList *mounts)
 
     fp = setmntent(mtab, "r");
     if (!fp) {
-        g_warning("fsfreeze: unable to read mtab");
-        return -1;
+        error_setg(err, "failed to open mtab file: '%s'", mtab);
+        return;
     }
 
     while ((ment = getmntent(fp))) {
@@ -405,8 +405,6 @@ static int build_fs_mount_list(FsMountList *mounts)
     }
 
     endmntent(fp);
-
-    return 0;
 }
 #endif
 
@@ -433,15 +431,17 @@ int64_t qmp_guest_fsfreeze_freeze(Error **err)
     int ret = 0, i = 0;
     FsMountList mounts;
     struct FsMount *mount;
+    Error *local_err = NULL;
     int fd;
     char err_msg[512];
 
     slog("guest-fsfreeze called");
 
     QTAILQ_INIT(&mounts);
-    ret = build_fs_mount_list(&mounts);
-    if (ret < 0) {
-        return ret;
+    build_fs_mount_list(&mounts, &local_err);
+    if (error_is_set(&local_err)) {
+        error_propagate(err, local_err);
+        return -1;
     }
 
     /* cannot risk guest agent blocking itself on a write in this state */
@@ -498,12 +498,12 @@ int64_t qmp_guest_fsfreeze_thaw(Error **err)
     FsMountList mounts;
     FsMount *mount;
     int fd, i = 0, logged;
+    Error *local_err = NULL;
 
     QTAILQ_INIT(&mounts);
-    ret = build_fs_mount_list(&mounts);
-    if (ret) {
-        error_set(err, QERR_QGA_COMMAND_FAILED,
-                  "failed to enumerate filesystems");
+    build_fs_mount_list(&mounts, &local_err);
+    if (error_is_set(&local_err)) {
+        error_propagate(err, local_err);
         return 0;
     }
 
@@ -568,6 +568,7 @@ void qmp_guest_fstrim(bool has_minimum, int64_t minimum, Error **err)
     FsMountList mounts;
     struct FsMount *mount;
     int fd;
+    Error *local_err = NULL;
     char err_msg[512];
     struct fstrim_range r = {
         .start = 0,
@@ -578,8 +579,9 @@ void qmp_guest_fstrim(bool has_minimum, int64_t minimum, Error **err)
     slog("guest-fstrim called");
 
     QTAILQ_INIT(&mounts);
-    ret = build_fs_mount_list(&mounts);
-    if (ret < 0) {
+    build_fs_mount_list(&mounts, &local_err);
+    if (error_is_set(&local_err)) {
+        error_propagate(err, local_err);
         return;
     }
 
-- 
1.8.0

[Qemu-devel] [PATCH 06/10] qemu-ga: qmp_guest_fsfreeze_*(): get rid of sprintf() + error_set()

[Luiz Capitulino]

Convert them to error_setg_errno().

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

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 59bb32d..9325433 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -433,7 +433,6 @@ int64_t qmp_guest_fsfreeze_freeze(Error **err)
     struct FsMount *mount;
     Error *local_err = NULL;
     int fd;
-    char err_msg[512];
 
     slog("guest-fsfreeze called");
 
@@ -450,9 +449,7 @@ int64_t qmp_guest_fsfreeze_freeze(Error **err)
     QTAILQ_FOREACH(mount, &mounts, next) {
         fd = qemu_open(mount->dirname, O_RDONLY);
         if (fd == -1) {
-            sprintf(err_msg, "failed to open %s, %s", mount->dirname,
-                    strerror(errno));
-            error_set(err, QERR_QGA_COMMAND_FAILED, err_msg);
+            error_setg_errno(err, errno, "failed to open %s", mount->dirname);
             goto error;
         }
 
@@ -468,9 +465,8 @@ int64_t qmp_guest_fsfreeze_freeze(Error **err)
         ret = ioctl(fd, FIFREEZE);
         if (ret == -1) {
             if (errno != EOPNOTSUPP) {
-                sprintf(err_msg, "failed to freeze %s, %s",
-                        mount->dirname, strerror(errno));
-                error_set(err, QERR_QGA_COMMAND_FAILED, err_msg);
+                error_setg_errno(err, errno, "failed to freeze %s",
+                                 mount->dirname);
                 close(fd);
                 goto error;
             }
-- 
1.8.0

Re: [Qemu-devel] [PATCH 06/10] qemu-ga: qmp_guest_fsfreeze_*(): get rid of sprintf() + error_set()

[mdroth]

On Tue, Nov 27, 2012 at 11:02:00AM -0200, Luiz Capitulino wrote:
> Convert them to error_setg_errno().
> 
> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>

Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com>

> ---
>  qga/commands-posix.c | 10 +++-------
>  1 file changed, 3 insertions(+), 7 deletions(-)
> 
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index 59bb32d..9325433 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -433,7 +433,6 @@ int64_t qmp_guest_fsfreeze_freeze(Error **err)
>      struct FsMount *mount;
>      Error *local_err = NULL;
>      int fd;
> -    char err_msg[512];
> 
>      slog("guest-fsfreeze called");
> 
> @@ -450,9 +449,7 @@ int64_t qmp_guest_fsfreeze_freeze(Error **err)
>      QTAILQ_FOREACH(mount, &mounts, next) {
>          fd = qemu_open(mount->dirname, O_RDONLY);
>          if (fd == -1) {
> -            sprintf(err_msg, "failed to open %s, %s", mount->dirname,
> -                    strerror(errno));
> -            error_set(err, QERR_QGA_COMMAND_FAILED, err_msg);
> +            error_setg_errno(err, errno, "failed to open %s", mount->dirname);
>              goto error;
>          }
> 
> @@ -468,9 +465,8 @@ int64_t qmp_guest_fsfreeze_freeze(Error **err)
>          ret = ioctl(fd, FIFREEZE);
>          if (ret == -1) {
>              if (errno != EOPNOTSUPP) {
> -                sprintf(err_msg, "failed to freeze %s, %s",
> -                        mount->dirname, strerror(errno));
> -                error_set(err, QERR_QGA_COMMAND_FAILED, err_msg);
> +                error_setg_errno(err, errno, "failed to freeze %s",
> +                                 mount->dirname);
>                  close(fd);
>                  goto error;
>              }
> -- 
> 1.8.0
> 

[Qemu-devel] [PATCH 07/10] qemu-ga: qmp_guest_fstrim(): get rid of sprintf() + error_set()

[Luiz Capitulino]

Convert them to error_setg_errno().

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

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 9325433..72ef8ba 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -565,7 +565,6 @@ void qmp_guest_fstrim(bool has_minimum, int64_t minimum, Error **err)
     struct FsMount *mount;
     int fd;
     Error *local_err = NULL;
-    char err_msg[512];
     struct fstrim_range r = {
         .start = 0,
         .len = -1,
@@ -584,9 +583,7 @@ void qmp_guest_fstrim(bool has_minimum, int64_t minimum, Error **err)
     QTAILQ_FOREACH(mount, &mounts, next) {
         fd = qemu_open(mount->dirname, O_RDONLY);
         if (fd == -1) {
-            sprintf(err_msg, "failed to open %s, %s", mount->dirname,
-                    strerror(errno));
-            error_set(err, QERR_QGA_COMMAND_FAILED, err_msg);
+            error_setg_errno(err, errno, "failed to open %s", mount->dirname);
             goto error;
         }
 
@@ -599,9 +596,8 @@ void qmp_guest_fstrim(bool has_minimum, int64_t minimum, Error **err)
         ret = ioctl(fd, FITRIM, &r);
         if (ret == -1) {
             if (errno != ENOTTY && errno != EOPNOTSUPP) {
-                sprintf(err_msg, "failed to trim %s, %s",
-                        mount->dirname, strerror(errno));
-                error_set(err, QERR_QGA_COMMAND_FAILED, err_msg);
+                error_setg_errno(err, errno, "failed to trim %s",
+                                 mount->dirname);
                 close(fd);
                 goto error;
             }
-- 
1.8.0

Re: [Qemu-devel] [PATCH 07/10] qemu-ga: qmp_guest_fstrim(): get rid of sprintf() + error_set()

[mdroth]

On Tue, Nov 27, 2012 at 11:02:01AM -0200, Luiz Capitulino wrote:
> Convert them to error_setg_errno().
> 
> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>

Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com>

> ---
>  qga/commands-posix.c | 10 +++-------
>  1 file changed, 3 insertions(+), 7 deletions(-)
> 
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index 9325433..72ef8ba 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -565,7 +565,6 @@ void qmp_guest_fstrim(bool has_minimum, int64_t minimum, Error **err)
>      struct FsMount *mount;
>      int fd;
>      Error *local_err = NULL;
> -    char err_msg[512];
>      struct fstrim_range r = {
>          .start = 0,
>          .len = -1,
> @@ -584,9 +583,7 @@ void qmp_guest_fstrim(bool has_minimum, int64_t minimum, Error **err)
>      QTAILQ_FOREACH(mount, &mounts, next) {
>          fd = qemu_open(mount->dirname, O_RDONLY);
>          if (fd == -1) {
> -            sprintf(err_msg, "failed to open %s, %s", mount->dirname,
> -                    strerror(errno));
> -            error_set(err, QERR_QGA_COMMAND_FAILED, err_msg);
> +            error_setg_errno(err, errno, "failed to open %s", mount->dirname);
>              goto error;
>          }
> 
> @@ -599,9 +596,8 @@ void qmp_guest_fstrim(bool has_minimum, int64_t minimum, Error **err)
>          ret = ioctl(fd, FITRIM, &r);
>          if (ret == -1) {
>              if (errno != ENOTTY && errno != EOPNOTSUPP) {
> -                sprintf(err_msg, "failed to trim %s, %s",
> -                        mount->dirname, strerror(errno));
> -                error_set(err, QERR_QGA_COMMAND_FAILED, err_msg);
> +                error_setg_errno(err, errno, "failed to trim %s",
> +                                 mount->dirname);
>                  close(fd);
>                  goto error;
>              }
> -- 
> 1.8.0
> 

[Qemu-devel] [PATCH 08/10] qemu-ga: qmp_guest_network_get_interfaces(): get rid of snprintf() + error_set()

[Luiz Capitulino]

Convert them to error_setg_errno().

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

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 72ef8ba..c399f02 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -802,12 +802,9 @@ GuestNetworkInterfaceList *qmp_guest_network_get_interfaces(Error **errp)
 {
     GuestNetworkInterfaceList *head = NULL, *cur_item = NULL;
     struct ifaddrs *ifap, *ifa;
-    char err_msg[512];
 
     if (getifaddrs(&ifap) < 0) {
-        snprintf(err_msg, sizeof(err_msg),
-                 "getifaddrs failed: %s", strerror(errno));
-        error_set(errp, QERR_QGA_COMMAND_FAILED, err_msg);
+        error_setg_errno(errp, errno, "getifaddrs failed");
         goto error;
     }
 
@@ -843,20 +840,16 @@ GuestNetworkInterfaceList *qmp_guest_network_get_interfaces(Error **errp)
             /* we haven't obtained HW address yet */
             sock = socket(PF_INET, SOCK_STREAM, 0);
             if (sock == -1) {
-                snprintf(err_msg, sizeof(err_msg),
-                         "failed to create socket: %s", strerror(errno));
-                error_set(errp, QERR_QGA_COMMAND_FAILED, err_msg);
+                error_setg_errno(errp, errno, "failed to create socket");
                 goto error;
             }
 
             memset(&ifr, 0, sizeof(ifr));
             pstrcpy(ifr.ifr_name, IF_NAMESIZE, info->value->name);
             if (ioctl(sock, SIOCGIFHWADDR, &ifr) == -1) {
-                snprintf(err_msg, sizeof(err_msg),
-                         "failed to get MAC address of %s: %s",
-                         ifa->ifa_name,
-                         strerror(errno));
-                error_set(errp, QERR_QGA_COMMAND_FAILED, err_msg);
+                error_setg_errno(errp, errno,
+                                 "failed to get MAC address of %s",
+                                 ifa->ifa_name);
                 goto error;
             }
 
@@ -867,9 +860,7 @@ GuestNetworkInterfaceList *qmp_guest_network_get_interfaces(Error **errp)
                          (int) mac_addr[0], (int) mac_addr[1],
                          (int) mac_addr[2], (int) mac_addr[3],
                          (int) mac_addr[4], (int) mac_addr[5]) == -1) {
-                snprintf(err_msg, sizeof(err_msg),
-                         "failed to format MAC: %s", strerror(errno));
-                error_set(errp, QERR_QGA_COMMAND_FAILED, err_msg);
+                error_setg_errno(errp, errno, "failed to format MAC");
                 goto error;
             }
 
@@ -884,9 +875,7 @@ GuestNetworkInterfaceList *qmp_guest_network_get_interfaces(Error **errp)
             address_item->value = g_malloc0(sizeof(*address_item->value));
             p = &((struct sockaddr_in *)ifa->ifa_addr)->sin_addr;
             if (!inet_ntop(AF_INET, p, addr4, sizeof(addr4))) {
-                snprintf(err_msg, sizeof(err_msg),
-                         "inet_ntop failed : %s", strerror(errno));
-                error_set(errp, QERR_QGA_COMMAND_FAILED, err_msg);
+                error_setg_errno(errp, errno, "inet_ntop failed");
                 goto error;
             }
 
@@ -906,9 +895,7 @@ GuestNetworkInterfaceList *qmp_guest_network_get_interfaces(Error **errp)
             address_item->value = g_malloc0(sizeof(*address_item->value));
             p = &((struct sockaddr_in6 *)ifa->ifa_addr)->sin6_addr;
             if (!inet_ntop(AF_INET6, p, addr6, sizeof(addr6))) {
-                snprintf(err_msg, sizeof(err_msg),
-                         "inet_ntop failed : %s", strerror(errno));
-                error_set(errp, QERR_QGA_COMMAND_FAILED, err_msg);
+                error_setg_errno(errp, errno, "inet_ntop failed");
                 goto error;
             }
 
-- 
1.8.0

Re: [Qemu-devel] [PATCH 08/10] qemu-ga: qmp_guest_network_get_interfaces(): get rid of snprintf() + error_set()

[mdroth]

On Tue, Nov 27, 2012 at 11:02:02AM -0200, Luiz Capitulino wrote:
> Convert them to error_setg_errno().
> 
> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>

Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com>

> ---
>  qga/commands-posix.c | 29 ++++++++---------------------
>  1 file changed, 8 insertions(+), 21 deletions(-)
> 
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index 72ef8ba..c399f02 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -802,12 +802,9 @@ GuestNetworkInterfaceList *qmp_guest_network_get_interfaces(Error **errp)
>  {
>      GuestNetworkInterfaceList *head = NULL, *cur_item = NULL;
>      struct ifaddrs *ifap, *ifa;
> -    char err_msg[512];
> 
>      if (getifaddrs(&ifap) < 0) {
> -        snprintf(err_msg, sizeof(err_msg),
> -                 "getifaddrs failed: %s", strerror(errno));
> -        error_set(errp, QERR_QGA_COMMAND_FAILED, err_msg);
> +        error_setg_errno(errp, errno, "getifaddrs failed");
>          goto error;
>      }
> 
> @@ -843,20 +840,16 @@ GuestNetworkInterfaceList *qmp_guest_network_get_interfaces(Error **errp)
>              /* we haven't obtained HW address yet */
>              sock = socket(PF_INET, SOCK_STREAM, 0);
>              if (sock == -1) {
> -                snprintf(err_msg, sizeof(err_msg),
> -                         "failed to create socket: %s", strerror(errno));
> -                error_set(errp, QERR_QGA_COMMAND_FAILED, err_msg);
> +                error_setg_errno(errp, errno, "failed to create socket");
>                  goto error;
>              }
> 
>              memset(&ifr, 0, sizeof(ifr));
>              pstrcpy(ifr.ifr_name, IF_NAMESIZE, info->value->name);
>              if (ioctl(sock, SIOCGIFHWADDR, &ifr) == -1) {
> -                snprintf(err_msg, sizeof(err_msg),
> -                         "failed to get MAC address of %s: %s",
> -                         ifa->ifa_name,
> -                         strerror(errno));
> -                error_set(errp, QERR_QGA_COMMAND_FAILED, err_msg);
> +                error_setg_errno(errp, errno,
> +                                 "failed to get MAC address of %s",
> +                                 ifa->ifa_name);
>                  goto error;
>              }
> 
> @@ -867,9 +860,7 @@ GuestNetworkInterfaceList *qmp_guest_network_get_interfaces(Error **errp)
>                           (int) mac_addr[0], (int) mac_addr[1],
>                           (int) mac_addr[2], (int) mac_addr[3],
>                           (int) mac_addr[4], (int) mac_addr[5]) == -1) {
> -                snprintf(err_msg, sizeof(err_msg),
> -                         "failed to format MAC: %s", strerror(errno));
> -                error_set(errp, QERR_QGA_COMMAND_FAILED, err_msg);
> +                error_setg_errno(errp, errno, "failed to format MAC");
>                  goto error;
>              }
> 
> @@ -884,9 +875,7 @@ GuestNetworkInterfaceList *qmp_guest_network_get_interfaces(Error **errp)
>              address_item->value = g_malloc0(sizeof(*address_item->value));
>              p = &((struct sockaddr_in *)ifa->ifa_addr)->sin_addr;
>              if (!inet_ntop(AF_INET, p, addr4, sizeof(addr4))) {
> -                snprintf(err_msg, sizeof(err_msg),
> -                         "inet_ntop failed : %s", strerror(errno));
> -                error_set(errp, QERR_QGA_COMMAND_FAILED, err_msg);
> +                error_setg_errno(errp, errno, "inet_ntop failed");
>                  goto error;
>              }
> 
> @@ -906,9 +895,7 @@ GuestNetworkInterfaceList *qmp_guest_network_get_interfaces(Error **errp)
>              address_item->value = g_malloc0(sizeof(*address_item->value));
>              p = &((struct sockaddr_in6 *)ifa->ifa_addr)->sin6_addr;
>              if (!inet_ntop(AF_INET6, p, addr6, sizeof(addr6))) {
> -                snprintf(err_msg, sizeof(err_msg),
> -                         "inet_ntop failed : %s", strerror(errno));
> -                error_set(errp, QERR_QGA_COMMAND_FAILED, err_msg);
> +                error_setg_errno(errp, errno, "inet_ntop failed");
>                  goto error;
>              }
> 
> -- 
> 1.8.0
> 

[Qemu-devel] [PATCH 09/10] qemu-ga: bios_supports_mode(): improve error reporting

[Luiz Capitulino]

Most errors are QERR_UNDEFINED_ERROR today.

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

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index c399f02..5a7e308 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -618,8 +618,9 @@ error:
 static void bios_supports_mode(const char *pmutils_bin, const char *pmutils_arg,
                                const char *sysfile_str, Error **err)
 {
+    Error *local_err = NULL;
     char *pmutils_path;
-    pid_t pid, rpid;
+    pid_t pid;
     int status;
 
     pmutils_path = g_find_program_in_path(pmutils_bin);
@@ -664,31 +665,38 @@ static void bios_supports_mode(const char *pmutils_bin, const char *pmutils_arg,
         }
 
         _exit(SUSPEND_NOT_SUPPORTED);
+    } else if (pid < 0) {
+        error_setg_errno(err, errno, "failed to create child process");
+        goto out;
     }
 
-    g_free(pmutils_path);
+    ga_wait_child(pid, &status, &local_err);
+    if (error_is_set(&local_err)) {
+        error_propagate(err, local_err);
+        goto out;
+    }
 
-    if (pid < 0) {
-        goto undef_err;
+    if (!WIFEXITED(status)) {
+        error_setg(err, "child process has terminated abnormally");
+        goto out;
     }
 
-    do {
-        rpid = waitpid(pid, &status, 0);
-    } while (rpid == -1 && errno == EINTR);
-    if (rpid == pid && WIFEXITED(status)) {
-        switch (WEXITSTATUS(status)) {
-        case SUSPEND_SUPPORTED:
-            return;
-        case SUSPEND_NOT_SUPPORTED:
-            error_set(err, QERR_UNSUPPORTED);
-            return;
-        default:
-            goto undef_err;
-        }
+    switch (WEXITSTATUS(status)) {
+    case SUSPEND_SUPPORTED:
+        goto out;
+    case SUSPEND_NOT_SUPPORTED:
+        error_setg(err,
+                   "the requested suspend mode is not supported by the guest");
+        goto out;
+    default:
+        error_setg(err,
+                   "the helper program '%s' returned an unexpected exit status"
+                   " code (%d)", pmutils_path, WEXITSTATUS(status));
+        goto out;
     }
 
-undef_err:
-    error_set(err, QERR_UNDEFINED_ERROR);
+out:
+    g_free(pmutils_path);
 }
 
 static void guest_suspend(const char *pmutils_bin, const char *sysfile_str,
-- 
1.8.0

Re: [Qemu-devel] [PATCH 09/10] qemu-ga: bios_supports_mode(): improve error reporting

[mdroth]

On Tue, Nov 27, 2012 at 11:02:03AM -0200, Luiz Capitulino wrote:
> Most errors are QERR_UNDEFINED_ERROR today.
> 
> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>

Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com>

> ---
>  qga/commands-posix.c | 46 +++++++++++++++++++++++++++-------------------
>  1 file changed, 27 insertions(+), 19 deletions(-)
> 
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index c399f02..5a7e308 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -618,8 +618,9 @@ error:
>  static void bios_supports_mode(const char *pmutils_bin, const char *pmutils_arg,
>                                 const char *sysfile_str, Error **err)
>  {
> +    Error *local_err = NULL;
>      char *pmutils_path;
> -    pid_t pid, rpid;
> +    pid_t pid;
>      int status;
> 
>      pmutils_path = g_find_program_in_path(pmutils_bin);
> @@ -664,31 +665,38 @@ static void bios_supports_mode(const char *pmutils_bin, const char *pmutils_arg,
>          }
> 
>          _exit(SUSPEND_NOT_SUPPORTED);
> +    } else if (pid < 0) {
> +        error_setg_errno(err, errno, "failed to create child process");
> +        goto out;
>      }
> 
> -    g_free(pmutils_path);
> +    ga_wait_child(pid, &status, &local_err);
> +    if (error_is_set(&local_err)) {
> +        error_propagate(err, local_err);
> +        goto out;
> +    }
> 
> -    if (pid < 0) {
> -        goto undef_err;
> +    if (!WIFEXITED(status)) {
> +        error_setg(err, "child process has terminated abnormally");
> +        goto out;
>      }
> 
> -    do {
> -        rpid = waitpid(pid, &status, 0);
> -    } while (rpid == -1 && errno == EINTR);
> -    if (rpid == pid && WIFEXITED(status)) {
> -        switch (WEXITSTATUS(status)) {
> -        case SUSPEND_SUPPORTED:
> -            return;
> -        case SUSPEND_NOT_SUPPORTED:
> -            error_set(err, QERR_UNSUPPORTED);
> -            return;
> -        default:
> -            goto undef_err;
> -        }
> +    switch (WEXITSTATUS(status)) {
> +    case SUSPEND_SUPPORTED:
> +        goto out;
> +    case SUSPEND_NOT_SUPPORTED:
> +        error_setg(err,
> +                   "the requested suspend mode is not supported by the guest");
> +        goto out;
> +    default:
> +        error_setg(err,
> +                   "the helper program '%s' returned an unexpected exit status"
> +                   " code (%d)", pmutils_path, WEXITSTATUS(status));
> +        goto out;
>      }
> 
> -undef_err:
> -    error_set(err, QERR_UNDEFINED_ERROR);
> +out:
> +    g_free(pmutils_path);
>  }
> 
>  static void guest_suspend(const char *pmutils_bin, const char *sysfile_str,
> -- 
> 1.8.0
> 

[Qemu-devel] [PATCH 10/10] qemu-ga: guest_suspend(): improve error reporting

[Luiz Capitulino]

Most errors are QERR_UNDEFINED_ERROR today.

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

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 5a7e308..ce058b5 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -702,8 +702,9 @@ out:
 static void guest_suspend(const char *pmutils_bin, const char *sysfile_str,
                           Error **err)
 {
+    Error *local_err = NULL;
     char *pmutils_path;
-    pid_t rpid, pid;
+    pid_t pid;
     int status;
 
     pmutils_path = g_find_program_in_path(pmutils_bin);
@@ -741,23 +742,29 @@ static void guest_suspend(const char *pmutils_bin, const char *sysfile_str,
         }
 
         _exit(EXIT_SUCCESS);
+    } else if (pid < 0) {
+        error_setg_errno(err, errno, "failed to create child process");
+        goto out;
     }
 
-    g_free(pmutils_path);
+    ga_wait_child(pid, &status, &local_err);
+    if (error_is_set(&local_err)) {
+        error_propagate(err, local_err);
+        goto out;
+    }
 
-    if (pid < 0) {
-        goto exit_err;
+    if (!WIFEXITED(status)) {
+        error_setg(err, "child process has terminated abnormally");
+        goto out;
     }
 
-    do {
-        rpid = waitpid(pid, &status, 0);
-    } while (rpid == -1 && errno == EINTR);
-    if (rpid == pid && WIFEXITED(status) && !WEXITSTATUS(status)) {
-        return;
+    if (WEXITSTATUS(status)) {
+        error_setg(err, "child process has failed to suspend");
+        goto out;
     }
 
-exit_err:
-    error_set(err, QERR_UNDEFINED_ERROR);
+out:
+    g_free(pmutils_path);
 }
 
 void qmp_guest_suspend_disk(Error **err)
-- 
1.8.0

Re: [Qemu-devel] [PATCH 10/10] qemu-ga: guest_suspend(): improve error reporting

[mdroth]

On Tue, Nov 27, 2012 at 11:02:04AM -0200, Luiz Capitulino wrote:
> Most errors are QERR_UNDEFINED_ERROR today.
> 
> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>

Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com>

> ---
>  qga/commands-posix.c | 29 ++++++++++++++++++-----------
>  1 file changed, 18 insertions(+), 11 deletions(-)
> 
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index 5a7e308..ce058b5 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -702,8 +702,9 @@ out:
>  static void guest_suspend(const char *pmutils_bin, const char *sysfile_str,
>                            Error **err)
>  {
> +    Error *local_err = NULL;
>      char *pmutils_path;
> -    pid_t rpid, pid;
> +    pid_t pid;
>      int status;
> 
>      pmutils_path = g_find_program_in_path(pmutils_bin);
> @@ -741,23 +742,29 @@ static void guest_suspend(const char *pmutils_bin, const char *sysfile_str,
>          }
> 
>          _exit(EXIT_SUCCESS);
> +    } else if (pid < 0) {
> +        error_setg_errno(err, errno, "failed to create child process");
> +        goto out;
>      }
> 
> -    g_free(pmutils_path);
> +    ga_wait_child(pid, &status, &local_err);
> +    if (error_is_set(&local_err)) {
> +        error_propagate(err, local_err);
> +        goto out;
> +    }
> 
> -    if (pid < 0) {
> -        goto exit_err;
> +    if (!WIFEXITED(status)) {
> +        error_setg(err, "child process has terminated abnormally");
> +        goto out;
>      }
> 
> -    do {
> -        rpid = waitpid(pid, &status, 0);
> -    } while (rpid == -1 && errno == EINTR);
> -    if (rpid == pid && WIFEXITED(status) && !WEXITSTATUS(status)) {
> -        return;
> +    if (WEXITSTATUS(status)) {
> +        error_setg(err, "child process has failed to suspend");
> +        goto out;
>      }
> 
> -exit_err:
> -    error_set(err, QERR_UNDEFINED_ERROR);
> +out:
> +    g_free(pmutils_path);
>  }
> 
>  void qmp_guest_suspend_disk(Error **err)
> -- 
> 1.8.0
> 

Re: [Qemu-devel] [PATCH 00/10] qemu-ga: revamp error messages (for 1.4)

[mdroth]

On Tue, Nov 27, 2012 at 11:01:54AM -0200, Luiz Capitulino wrote:
> This series revamps qemu-ga error messages by:
> 
>  - Propagating errors down to functions that generate errors
>  - Fixing UndefinedErrors
>  - Getting rid of sprint() + error_set()
>  - Other small fixes

Hi Luiz,

Patches look good other than a few comments on usage of possibly-unset errno.

CC'ing Eric as a heads up, but based on previous comments (libvirt
always using 'desc' field over 'class' field and reporting it directly
to users) I don't think it should cause any compatibility issues on that
end.

> 
> Only the POSIX port is covered in this series, win32 is left to another day.
> 
> Luiz Capitulino (10):
>   qemu-ga: guest_file_handle_find(): take an Error argument
>   qemu-ga: qmp_guest_file_close(): fix fclose() error check
>   qemu-ga: qmp_guest_file_*: improve error reporting
>   qemu-ga: qmp_guest_shutdown(): improve error reporting
>   qemu-ga: build_fs_mount_list(): take an Error argument
>   qemu-ga: qmp_guest_fsfreeze_*(): get rid of sprintf() + error_set()
>   qemu-ga: qmp_guest_fstrim(): get rid of sprintf() + error_set()
>   qemu-ga: qmp_guest_network_get_interfaces(): get rid of snprintf() +
>     error_set()
>   qemu-ga: bios_supports_mode(): improve error reporting
>   qemu-ga: guest_suspend(): improve error reporting
> 
>  qga/commands-posix.c | 244 ++++++++++++++++++++++++++++-----------------------
>  1 file changed, 134 insertions(+), 110 deletions(-)
> 
> -- 
> 1.8.0
> 

Re: [Qemu-devel] [PATCH 00/10] qemu-ga: revamp error messages (for 1.4)

[mdroth]

On Wed, Nov 28, 2012 at 02:04:31PM -0600, mdroth wrote:
> On Tue, Nov 27, 2012 at 11:01:54AM -0200, Luiz Capitulino wrote:
> > This series revamps qemu-ga error messages by:
> > 
> >  - Propagating errors down to functions that generate errors
> >  - Fixing UndefinedErrors
> >  - Getting rid of sprint() + error_set()
> >  - Other small fixes
> 
> Hi Luiz,
> 
> Patches look good other than a few comments on usage of possibly-unset errno.
> 
> CC'ing Eric as a heads up, but based on previous comments (libvirt

Actually CC'ing this time

> always using 'desc' field over 'class' field and reporting it directly
> to users) I don't think it should cause any compatibility issues on that
> end.
> 
> > 
> > Only the POSIX port is covered in this series, win32 is left to another day.
> > 
> > Luiz Capitulino (10):
> >   qemu-ga: guest_file_handle_find(): take an Error argument
> >   qemu-ga: qmp_guest_file_close(): fix fclose() error check
> >   qemu-ga: qmp_guest_file_*: improve error reporting
> >   qemu-ga: qmp_guest_shutdown(): improve error reporting
> >   qemu-ga: build_fs_mount_list(): take an Error argument
> >   qemu-ga: qmp_guest_fsfreeze_*(): get rid of sprintf() + error_set()
> >   qemu-ga: qmp_guest_fstrim(): get rid of sprintf() + error_set()
> >   qemu-ga: qmp_guest_network_get_interfaces(): get rid of snprintf() +
> >     error_set()
> >   qemu-ga: bios_supports_mode(): improve error reporting
> >   qemu-ga: guest_suspend(): improve error reporting
> > 
> >  qga/commands-posix.c | 244 ++++++++++++++++++++++++++++-----------------------
> >  1 file changed, 134 insertions(+), 110 deletions(-)
> > 
> > -- 
> > 1.8.0
> > 

Re: [Qemu-devel] [PATCH 00/10] qemu-ga: revamp error messages (for 1.4)

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1065 bytes --]

On 11/28/2012 01:04 PM, mdroth wrote:
> On Tue, Nov 27, 2012 at 11:01:54AM -0200, Luiz Capitulino wrote:
>> This series revamps qemu-ga error messages by:
>>
>>  - Propagating errors down to functions that generate errors
>>  - Fixing UndefinedErrors
>>  - Getting rid of sprint() + error_set()
>>  - Other small fixes
> 
> Hi Luiz,
> 
> Patches look good other than a few comments on usage of possibly-unset errno.
> 
> CC'ing Eric as a heads up, but based on previous comments (libvirt
> always using 'desc' field over 'class' field and reporting it directly
> to users) I don't think it should cause any compatibility issues on that
> end.

And finally replying.  You are correct that libvirt favors the contents
of 'desc' and only if that is missing falls back to 'class' when passing
an error message through to the user.  I didn't spot any issues with the
changes in this series, although I admit my testing was rather light.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [PATCH 00/10] qemu-ga: revamp error messages (for 1.4)

[mdroth]

On Tue, Nov 27, 2012 at 11:01:54AM -0200, Luiz Capitulino wrote:
> This series revamps qemu-ga error messages by:
> 
>  - Propagating errors down to functions that generate errors
>  - Fixing UndefinedErrors
>  - Getting rid of sprint() + error_set()
>  - Other small fixes
> 
> Only the POSIX port is covered in this series, win32 is left to another day.

Thanks, applied to qga branch:

https://github.com/mdroth/qemu/commits/qga

with a small formatting fix for 01/10

> 
> Luiz Capitulino (10):
>   qemu-ga: guest_file_handle_find(): take an Error argument
>   qemu-ga: qmp_guest_file_close(): fix fclose() error check
>   qemu-ga: qmp_guest_file_*: improve error reporting
>   qemu-ga: qmp_guest_shutdown(): improve error reporting
>   qemu-ga: build_fs_mount_list(): take an Error argument
>   qemu-ga: qmp_guest_fsfreeze_*(): get rid of sprintf() + error_set()
>   qemu-ga: qmp_guest_fstrim(): get rid of sprintf() + error_set()
>   qemu-ga: qmp_guest_network_get_interfaces(): get rid of snprintf() +
>     error_set()
>   qemu-ga: bios_supports_mode(): improve error reporting
>   qemu-ga: guest_suspend(): improve error reporting
> 
>  qga/commands-posix.c | 244 ++++++++++++++++++++++++++++-----------------------
>  1 file changed, 134 insertions(+), 110 deletions(-)
> 
> -- 
> 1.8.0
>