Re: [Qemu-devel] [PATCH 17/31] vl: Clean up error reporting in parse_add_fd()

[Marc-André Lureau]

Hi

On Mon, Oct 8, 2018 at 9:44 PM Markus Armbruster <address@hidden> wrote:
>
> Calling error_report() in a function that takes an Error ** argument
> is suspicious.  parse_add_fd() does that, and then fails without
> setting an error.  Its caller main(), via qemu_opts_foreach(), is fine
> with it, but clean it up anyway.
>
> Also change call of cleanup_add_fd(), which can't fail, for symmetry.
>
> Signed-off-by: Markus Armbruster <address@hidden>

Reviewed-by: Marc-André Lureau <address@hidden>

> ---
>  vl.c | 22 +++++++++-------------
>  1 file changed, 9 insertions(+), 13 deletions(-)
>
> diff --git a/vl.c b/vl.c
> index 485c3fc008..7ce8299d9d 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -1059,12 +1059,12 @@ static int parse_add_fd(void *opaque, QemuOpts *opts, 
> Error **errp)
>      fd_opaque = qemu_opt_get(opts, "opaque");
>
>      if (fd < 0) {
> -        error_report("fd option is required and must be non-negative");
> +        error_setg(errp, "fd option is required and must be non-negative");
>          return -1;
>      }
>
>      if (fd <= STDERR_FILENO) {
> -        error_report("fd cannot be a standard I/O stream");
> +        error_setg(errp, "fd cannot be a standard I/O stream");
>          return -1;
>      }
>
> @@ -1074,12 +1074,12 @@ static int parse_add_fd(void *opaque, QemuOpts *opts, 
> Error **errp)
>       */
>      flags = fcntl(fd, F_GETFD);
>      if (flags == -1 || (flags & FD_CLOEXEC)) {
> -        error_report("fd is not valid or already in use");
> +        error_setg(errp, "fd is not valid or already in use");
>          return -1;
>      }
>
>      if (fdset_id < 0) {
> -        error_report("set option is required and must be non-negative");
> +        error_setg(errp, "set option is required and must be non-negative");
>          return -1;
>      }
>
> @@ -1092,7 +1092,7 @@ static int parse_add_fd(void *opaque, QemuOpts *opts, 
> Error **errp)
>      }
>  #endif
>      if (dupfd == -1) {
> -        error_report("error duplicating fd: %s", strerror(errno));
> +        error_setg(errp, "error duplicating fd: %s", strerror(errno));
>          return -1;
>      }
>
> @@ -3936,15 +3936,11 @@ int main(int argc, char **argv, char **envp)
>      }
>
>  #ifndef _WIN32
> -    if (qemu_opts_foreach(qemu_find_opts("add-fd"),
> -                          parse_add_fd, NULL, NULL)) {
> -        exit(1);
> -    }
> +    qemu_opts_foreach(qemu_find_opts("add-fd"),
> +                      parse_add_fd, NULL, &error_fatal);
>
> -    if (qemu_opts_foreach(qemu_find_opts("add-fd"),
> -                          cleanup_add_fd, NULL, NULL)) {
> -        exit(1);
> -    }
> +    qemu_opts_foreach(qemu_find_opts("add-fd"),
> +                      cleanup_add_fd, NULL, &error_fatal);
>  #endif
>
>      current_machine = MACHINE(object_new(object_class_get_name(
> --
> 2.17.1
>
>


-- 
Marc-André Lureau