Re: [Qemu-devel] [PATCH v2 02/15] qga: Consistently name Error ** objects errp, and not err

[Michael Roth]

Quoting Markus Armbruster (2014-04-28 15:27:41)
> Signed-off-by: Markus Armbruster <address@hidden>
> Reviewed-by: Eric Blake <address@hidden>
> ---
>  qga/commands-posix.c | 209 
> ++++++++++++++++++++++++++-------------------------
>  qga/commands-win32.c | 115 ++++++++++++++--------------
>  qga/commands.c       |   4 +-
>  qga/vss-win32.c      |   4 +-
>  qga/vss-win32.h      |   2 +-
>  5 files changed, 170 insertions(+), 164 deletions(-)
> 
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index 935a4ec..e49c7da 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -53,7 +53,7 @@ extern char **environ;
>  #endif
>  #endif
> 
> -static void ga_wait_child(pid_t pid, int *status, Error **err)
> +static void ga_wait_child(pid_t pid, int *status, Error **errp)
>  {
>      pid_t rpid;
> 
> @@ -64,14 +64,15 @@ static void ga_wait_child(pid_t pid, int *status, Error 
> **err)
>      } while (rpid == -1 && errno == EINTR);
> 
>      if (rpid == -1) {
> -        error_setg_errno(err, errno, "failed to wait for child (pid: %d)", 
> pid);
> +        error_setg_errno(errp, 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)
> +void qmp_guest_shutdown(bool has_mode, const char *mode, Error **errp)
>  {
>      const char *shutdown_flag;
>      Error *local_err = NULL;
> @@ -86,7 +87,7 @@ void qmp_guest_shutdown(bool has_mode, const char *mode, 
> Error **err)
>      } else if (strcmp(mode, "reboot") == 0) {
>          shutdown_flag = "-r";
>      } else {
> -        error_setg(err,
> +        error_setg(errp,
>                     "mode is invalid (valid values are: 
> halt|powerdown|reboot");
>          return;
>      }
> @@ -103,23 +104,23 @@ 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) {
> -        error_setg_errno(err, errno, "failed to create child process");
> +        error_setg_errno(errp, errno, "failed to create child process");
>          return;
>      }
> 
>      ga_wait_child(pid, &status, &local_err);
>      if (local_err) {
> -        error_propagate(err, local_err);
> +        error_propagate(errp, local_err);
>          return;
>      }
> 
>      if (!WIFEXITED(status)) {
> -        error_setg(err, "child process has terminated abnormally");
> +        error_setg(errp, "child process has terminated abnormally");
>          return;
>      }
> 
>      if (WEXITSTATUS(status)) {
> -        error_setg(err, "child process has failed to shutdown");
> +        error_setg(errp, "child process has failed to shutdown");
>          return;
>      }
> 
> @@ -234,7 +235,7 @@ static int64_t guest_file_handle_add(FILE *fh, Error 
> **errp)
>      return handle;
>  }
> 
> -static GuestFileHandle *guest_file_handle_find(int64_t id, Error **err)
> +static GuestFileHandle *guest_file_handle_find(int64_t id, Error **errp)
>  {
>      GuestFileHandle *gfh;
> 
> @@ -245,7 +246,7 @@ static GuestFileHandle *guest_file_handle_find(int64_t 
> id, Error **err)
>          }
>      }
> 
> -    error_setg(err, "handle '%" PRId64 "' has not been found", id);
> +    error_setg(errp, "handle '%" PRId64 "' has not been found", id);
>      return NULL;
>  }
> 
> @@ -275,7 +276,7 @@ static const struct {
>  };
> 
>  static int
> -find_open_flag(const char *mode_str, Error **err)
> +find_open_flag(const char *mode_str, Error **errp)
>  {
>      unsigned mode;
> 
> @@ -292,7 +293,7 @@ find_open_flag(const char *mode_str, Error **err)
>      }
> 
>      if (mode == ARRAY_SIZE(guest_file_open_modes)) {
> -        error_setg(err, "invalid file open mode '%s'", mode_str);
> +        error_setg(errp, "invalid file open mode '%s'", mode_str);
>          return -1;
>      }
>      return guest_file_open_modes[mode].oflag_base | O_NOCTTY | O_NONBLOCK;
> @@ -303,7 +304,7 @@ find_open_flag(const char *mode_str, Error **err)
>                                 S_IROTH | S_IWOTH)
> 
>  static FILE *
> -safe_open_or_create(const char *path, const char *mode, Error **err)
> +safe_open_or_create(const char *path, const char *mode, Error **errp)
>  {
>      Error *local_err = NULL;
>      int oflag;
> @@ -370,11 +371,12 @@ safe_open_or_create(const char *path, const char *mode, 
> Error **err)
>          }
>      }
> 
> -    error_propagate(err, local_err);
> +    error_propagate(errp, local_err);
>      return NULL;
>  }
> 
> -int64_t qmp_guest_file_open(const char *path, bool has_mode, const char 
> *mode, Error **err)
> +int64_t qmp_guest_file_open(const char *path, bool has_mode, const char 
> *mode,
> +                            Error **errp)
>  {
>      FILE *fh;
>      Error *local_err = NULL;
> @@ -387,7 +389,7 @@ 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 = safe_open_or_create(path, mode, &local_err);
>      if (local_err != NULL) {
> -        error_propagate(err, local_err);
> +        error_propagate(errp, local_err);
>          return -1;
>      }
> 
> @@ -398,14 +400,14 @@ 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_setg_errno(err, errno, "failed to make file '%s' non-blocking",
> +        error_setg_errno(errp, errno, "failed to make file '%s' 
> non-blocking",
>                           path);
>          fclose(fh);
>          return -1;
>      }
> 
> -    handle = guest_file_handle_add(fh, err);
> -    if (error_is_set(err)) {
> +    handle = guest_file_handle_add(fh, errp);
> +    if (error_is_set(errp)) {
>          fclose(fh);
>          return -1;
>      }
> @@ -414,9 +416,9 @@ int64_t qmp_guest_file_open(const char *path, bool 
> has_mode, const char *mode, E
>      return handle;
>  }
> 
> -void qmp_guest_file_close(int64_t handle, Error **err)
> +void qmp_guest_file_close(int64_t handle, Error **errp)
>  {
> -    GuestFileHandle *gfh = guest_file_handle_find(handle, err);
> +    GuestFileHandle *gfh = guest_file_handle_find(handle, errp);
>      int ret;
> 
>      slog("guest-file-close called, handle: %" PRId64, handle);
> @@ -426,7 +428,7 @@ void qmp_guest_file_close(int64_t handle, Error **err)
> 
>      ret = fclose(gfh->fh);
>      if (ret == EOF) {
> -        error_setg_errno(err, errno, "failed to close handle");
> +        error_setg_errno(errp, errno, "failed to close handle");
>          return;
>      }
> 
> @@ -435,9 +437,9 @@ 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)
> +                                          int64_t count, Error **errp)
>  {
> -    GuestFileHandle *gfh = guest_file_handle_find(handle, err);
> +    GuestFileHandle *gfh = guest_file_handle_find(handle, errp);
>      GuestFileRead *read_data = NULL;
>      guchar *buf;
>      FILE *fh;
> @@ -450,7 +452,7 @@ 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_setg(err, "value '%" PRId64 "' is invalid for argument count",
> +        error_setg(errp, "value '%" PRId64 "' is invalid for argument count",
>                     count);
>          return NULL;
>      }
> @@ -459,7 +461,7 @@ 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");
> +        error_setg_errno(errp, errno, "failed to read file");
>          slog("guest-file-read failed, handle: %" PRId64, handle);
>      } else {
>          buf[read_count] = 0;
> @@ -477,13 +479,14 @@ struct GuestFileRead *qmp_guest_file_read(int64_t 
> handle, bool has_count,
>  }
> 
>  GuestFileWrite *qmp_guest_file_write(int64_t handle, const char *buf_b64,
> -                                     bool has_count, int64_t count, Error 
> **err)
> +                                     bool has_count, int64_t count,
> +                                     Error **errp)
>  {
>      GuestFileWrite *write_data = NULL;
>      guchar *buf;
>      gsize buf_len;
>      int write_count;
> -    GuestFileHandle *gfh = guest_file_handle_find(handle, err);
> +    GuestFileHandle *gfh = guest_file_handle_find(handle, errp);
>      FILE *fh;
> 
>      if (!gfh) {
> @@ -496,7 +499,7 @@ 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",
> +        error_setg(errp, "value '%" PRId64 "' is invalid for argument count",
>                     count);
>          g_free(buf);
>          return NULL;
> @@ -504,7 +507,7 @@ GuestFileWrite *qmp_guest_file_write(int64_t handle, 
> const char *buf_b64,
> 
>      write_count = fwrite(buf, 1, count, fh);
>      if (ferror(fh)) {
> -        error_setg_errno(err, errno, "failed to write to file");
> +        error_setg_errno(errp, errno, "failed to write to file");
>          slog("guest-file-write failed, handle: %" PRId64, handle);
>      } else {
>          write_data = g_malloc0(sizeof(GuestFileWrite));
> @@ -518,9 +521,9 @@ 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)
> +                                          int64_t whence, Error **errp)
>  {
> -    GuestFileHandle *gfh = guest_file_handle_find(handle, err);
> +    GuestFileHandle *gfh = guest_file_handle_find(handle, errp);
>      GuestFileSeek *seek_data = NULL;
>      FILE *fh;
>      int ret;
> @@ -532,7 +535,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_setg_errno(err, errno, "failed to seek file");
> +        error_setg_errno(errp, errno, "failed to seek file");
>      } else {
>          seek_data = g_new0(GuestFileSeek, 1);
>          seek_data->position = ftell(fh);
> @@ -543,9 +546,9 @@ struct GuestFileSeek *qmp_guest_file_seek(int64_t handle, 
> int64_t offset,
>      return seek_data;
>  }
> 
> -void qmp_guest_file_flush(int64_t handle, Error **err)
> +void qmp_guest_file_flush(int64_t handle, Error **errp)
>  {
> -    GuestFileHandle *gfh = guest_file_handle_find(handle, err);
> +    GuestFileHandle *gfh = guest_file_handle_find(handle, errp);
>      FILE *fh;
>      int ret;
> 
> @@ -556,7 +559,7 @@ void qmp_guest_file_flush(int64_t handle, Error **err)
>      fh = gfh->fh;
>      ret = fflush(fh);
>      if (ret == EOF) {
> -        error_setg_errno(err, errno, "failed to flush file");
> +        error_setg_errno(errp, errno, "failed to flush file");
>      }
>  }
> 
> @@ -596,7 +599,7 @@ static void free_fs_mount_list(FsMountList *mounts)
>  /*
>   * Walk the mount table and build a list of local file systems
>   */
> -static void build_fs_mount_list(FsMountList *mounts, Error **err)
> +static void build_fs_mount_list(FsMountList *mounts, Error **errp)
>  {
>      struct mntent *ment;
>      FsMount *mount;
> @@ -605,7 +608,7 @@ static void build_fs_mount_list(FsMountList *mounts, 
> Error **err)
> 
>      fp = setmntent(mtab, "r");
>      if (!fp) {
> -        error_setg(err, "failed to open mtab file: '%s'", mtab);
> +        error_setg(errp, "failed to open mtab file: '%s'", mtab);
>          return;
>      }
> 
> @@ -645,7 +648,7 @@ const char *fsfreeze_hook_arg_string[] = {
>      "freeze",
>  };
> 
> -static void execute_fsfreeze_hook(FsfreezeHookArg arg, Error **err)
> +static void execute_fsfreeze_hook(FsfreezeHookArg arg, Error **errp)
>  {
>      int status;
>      pid_t pid;
> @@ -658,7 +661,7 @@ static void execute_fsfreeze_hook(FsfreezeHookArg arg, 
> Error **err)
>          return;
>      }
>      if (access(hook, X_OK) != 0) {
> -        error_setg_errno(err, errno, "can't access fsfreeze hook '%s'", 
> hook);
> +        error_setg_errno(errp, errno, "can't access fsfreeze hook '%s'", 
> hook);
>          return;
>      }
> 
> @@ -673,24 +676,24 @@ static void execute_fsfreeze_hook(FsfreezeHookArg arg, 
> Error **err)
>          execle(hook, hook, arg_str, NULL, environ);
>          _exit(EXIT_FAILURE);
>      } else if (pid < 0) {
> -        error_setg_errno(err, errno, "failed to create child process");
> +        error_setg_errno(errp, errno, "failed to create child process");
>          return;
>      }
> 
>      ga_wait_child(pid, &status, &local_err);
>      if (local_err) {
> -        error_propagate(err, local_err);
> +        error_propagate(errp, local_err);
>          return;
>      }
> 
>      if (!WIFEXITED(status)) {
> -        error_setg(err, "fsfreeze hook has terminated abnormally");
> +        error_setg(errp, "fsfreeze hook has terminated abnormally");
>          return;
>      }
> 
>      status = WEXITSTATUS(status);
>      if (status) {
> -        error_setg(err, "fsfreeze hook has failed with status %d", status);
> +        error_setg(errp, "fsfreeze hook has failed with status %d", status);
>          return;
>      }
>  }
> @@ -698,7 +701,7 @@ static void execute_fsfreeze_hook(FsfreezeHookArg arg, 
> Error **err)
>  /*
>   * Return status of freeze/thaw
>   */
> -GuestFsfreezeStatus qmp_guest_fsfreeze_status(Error **err)
> +GuestFsfreezeStatus qmp_guest_fsfreeze_status(Error **errp)
>  {
>      if (ga_is_frozen(ga_state)) {
>          return GUEST_FSFREEZE_STATUS_FROZEN;
> @@ -711,7 +714,7 @@ GuestFsfreezeStatus qmp_guest_fsfreeze_status(Error **err)
>   * Walk list of mounted file systems in the guest, and freeze the ones which
>   * are real local file systems.
>   */
> -int64_t qmp_guest_fsfreeze_freeze(Error **err)
> +int64_t qmp_guest_fsfreeze_freeze(Error **errp)
>  {
>      int ret = 0, i = 0;
>      FsMountList mounts;
> @@ -723,14 +726,14 @@ int64_t qmp_guest_fsfreeze_freeze(Error **err)
> 
>      execute_fsfreeze_hook(FSFREEZE_HOOK_FREEZE, &local_err);
>      if (local_err) {
> -        error_propagate(err, local_err);
> +        error_propagate(errp, local_err);
>          return -1;
>      }
> 
>      QTAILQ_INIT(&mounts);
>      build_fs_mount_list(&mounts, &local_err);
>      if (local_err) {
> -        error_propagate(err, local_err);
> +        error_propagate(errp, local_err);
>          return -1;
>      }
> 
> @@ -740,7 +743,7 @@ int64_t qmp_guest_fsfreeze_freeze(Error **err)
>      QTAILQ_FOREACH_REVERSE(mount, &mounts, FsMountList, next) {
>          fd = qemu_open(mount->dirname, O_RDONLY);
>          if (fd == -1) {
> -            error_setg_errno(err, errno, "failed to open %s", 
> mount->dirname);
> +            error_setg_errno(errp, errno, "failed to open %s", 
> mount->dirname);
>              goto error;
>          }
> 
> @@ -756,7 +759,7 @@ int64_t qmp_guest_fsfreeze_freeze(Error **err)
>          ret = ioctl(fd, FIFREEZE);
>          if (ret == -1) {
>              if (errno != EOPNOTSUPP) {
> -                error_setg_errno(err, errno, "failed to freeze %s",
> +                error_setg_errno(errp, errno, "failed to freeze %s",
>                                   mount->dirname);
>                  close(fd);
>                  goto error;
> @@ -779,7 +782,7 @@ error:
>  /*
>   * Walk list of frozen file systems in the guest, and thaw them.
>   */
> -int64_t qmp_guest_fsfreeze_thaw(Error **err)
> +int64_t qmp_guest_fsfreeze_thaw(Error **errp)
>  {
>      int ret;
>      FsMountList mounts;
> @@ -790,7 +793,7 @@ int64_t qmp_guest_fsfreeze_thaw(Error **err)
>      QTAILQ_INIT(&mounts);
>      build_fs_mount_list(&mounts, &local_err);
>      if (local_err) {
> -        error_propagate(err, local_err);
> +        error_propagate(errp, local_err);
>          return 0;
>      }
> 
> @@ -829,21 +832,21 @@ int64_t qmp_guest_fsfreeze_thaw(Error **err)
>      ga_unset_frozen(ga_state);
>      free_fs_mount_list(&mounts);
> 
> -    execute_fsfreeze_hook(FSFREEZE_HOOK_THAW, err);
> +    execute_fsfreeze_hook(FSFREEZE_HOOK_THAW, errp);
> 
>      return i;
>  }
> 
>  static void guest_fsfreeze_cleanup(void)
>  {
> -    Error *err = NULL;
> +    Error *errp = NULL;

Surprisingly, I actually got this one right the first time around.

Well, I guess Error *local_err is the convention for these, so maybe
not...

Looks good otherwise.