Re: [Qemu-devel] [PATCH v2 10/16] vl.c: Convert error sentences to simpler phrases

[Markus Armbruster]

Eduardo Habkost <address@hidden> writes:

> Simplify some error messages by making them simple phrases instead of
> full sentences.
>
> Suggested-by: Andrew Jones <address@hidden>
> Signed-off-by: Eduardo Habkost <address@hidden>
> ---
>  vl.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/vl.c b/vl.c
> index 67147e0..b8c6c3c 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -1066,7 +1066,7 @@ 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_report("non-negative 'fd' option required");
>          return -1;
>      }
>  

We need to make up our mind how to call a QemuOpts option's key=value
part.  qemu-options.c (actually qerror.h, which I still haven't killed
off) calls it "parameter", to avoid confusion with the entire option,
i.e. the -opt key=value,...

The error reporting lazy here: you get the same message for a missing fd
as for a negative fd, because the programmer couldn't be bothered to
detect the difference.  Not this patch's problem.

> @@ -1081,12 +1081,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_report("fd not valid or already in use");
>          return -1;
>      }

As in PATCH 07, I'd prefer the phrasing with the verb.  More of the same
below.

Of course, the error message is crap either way.  "The order you gave is
invalid (but I'm not telling you why), or it clashes with some other
order I got (but I'm not telling you which one).  Pffft.  Not this
patch's problem.

>  
>      if (fdset_id < 0) {
> -        error_report("set option is required and must be non-negative");
> +        error_report("non-negative 'set' option required");
>          return -1;
>      }
>  

Lazy again.

> @@ -3751,7 +3751,7 @@ int main(int argc, char **argv, char **envp)
>                  option_rom[nb_option_roms].bootindex =
>                      qemu_opt_get_number(opts, "bootindex", -1);
>                  if (!option_rom[nb_option_roms].name) {
> -                    error_report("Option ROM file is not specified");
> +                    error_report("option ROM file not specified");
>                      exit(1);
>                  }
>                  nb_option_roms++;
> @@ -4225,11 +4225,11 @@ int main(int argc, char **argv, char **envp)
>      }
>  
>      if ((no_frame || alt_grab || ctrl_grab) && display_type != DT_SDL) {
> -        error_report("-no-frame, -alt-grab and -ctrl-grab are only valid "
> +        error_report("-no-frame, -alt-grab and -ctrl-grab only valid "
>                       "for SDL, ignoring option");
>      }
>      if (no_quit && (display_type != DT_GTK && display_type != DT_SDL)) {
> -        error_report("-no-quit is only valid for GTK and SDL, "
> +        error_report("-no-quit only valid for GTK and SDL, "
>                       "ignoring option");
>      }
>  
> @@ -4373,7 +4373,7 @@ int main(int argc, char **argv, char **envp)
>      cpu_ticks_init();
>      if (icount_opts) {
>          if (kvm_enabled() || xen_enabled()) {
> -            error_report("-icount is not allowed with kvm or xen");
> +            error_report("-icount not allowed with kvm or xen");
>              exit(1);
>          }
>          configure_icount(icount_opts, &error_abort);