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

<snip>