Re: [PATCH v2 05/22] qapi: Inline QERR_INVALID_PARAMETER definition (constant parameter)

[Markus Armbruster]

Philippe Mathieu-Daudé <philmd@linaro.org> writes:

> Address the comment added in commit 4629ed1e98
> ("qerror: Finally unused, clean up"), from 2015:
>
>   /*
>    * These macros will go away, please don't use
>    * in new code, and do not add new ones!
>    */
>
> Mechanical transformation using the following
> coccinelle semantic patch:
>
>     @match@
>     expression errp;
>     constant param;
>     @@
>          error_setg(errp, QERR_INVALID_PARAMETER, param);
>
>     @script:python strformat depends on match@
>     param << match.param;
>     fixedfmt; // new var
>     @@
>     fixedfmt = f'"Invalid parameter \'{param[1:-1]}\'"' # Format skipping '"'.
>     coccinelle.fixedfmt = cocci.make_ident(fixedfmt)
>
>     @replace@
>     expression match.errp;
>     constant match.param;
>     identifier strformat.fixedfmt;
>     @@
>     -    error_setg(errp, QERR_INVALID_PARAMETER, param);
>     +    error_setg(errp, fixedfmt);
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  dump/dump.c        | 6 +++---
>  qga/commands.c     | 2 +-
>  ui/ui-qmp-cmds.c   | 2 +-
>  util/qemu-option.c | 2 +-
>  4 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/dump/dump.c b/dump/dump.c
> index d4ef713cd0..e173f1f14c 100644
> --- a/dump/dump.c
> +++ b/dump/dump.c
> @@ -1810,7 +1810,7 @@ static void dump_init(DumpState *s, int fd, bool 
> has_format,
>  
>      s->fd = fd;
>      if (has_filter && !length) {
> -        error_setg(errp, QERR_INVALID_PARAMETER, "length");

Incorrect use of QERR_INVALID_PARAMETER: the parameter is perfectly
valid, the problem is its invalid value.  Better:

           error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "a non-zero size");

> +        error_setg(errp, "Invalid parameter 'length'");

Applying PATCH 12's transformation then results in

           error_setg(errp, "Parameter '%s' expects a non-zero size",
                      "length");

which you may want to contract to

           error_setg(errp, "Parameter 'length' expects a non-zero size");

But not in this patch.  Either before or after.  I'd pick before.

>          goto cleanup;
>      }
>      s->filter_area_begin = begin;
> @@ -1841,7 +1841,7 @@ static void dump_init(DumpState *s, int fd, bool 
> has_format,
>  
>      /* Is the filter filtering everything? */
>      if (validate_start_block(s) == -1) {
> -        error_setg(errp, QERR_INVALID_PARAMETER, "begin");
> +        error_setg(errp, "Invalid parameter 'begin'");
>          goto cleanup;
>      }
>  
> @@ -2145,7 +2145,7 @@ void qmp_dump_guest_memory(bool paging, const char 
> *file,
   #if !defined(WIN32)
       if (strstart(file, "fd:", &p)) {
           fd = monitor_get_fd(monitor_cur(), p, errp);
           if (fd == -1) {
               return;
           }
       }
   #endif

       if  (strstart(file, "file:", &p)) {
           fd = qemu_open_old(p, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY, 
S_IRUSR);
           if (fd < 0) {
               error_setg_file_open(errp, errno, p);
               return;
           }
>      }
>  
>      if (fd == -1) {
> -        error_setg(errp, QERR_INVALID_PARAMETER, "protocol");

This made me go "there is no parameter protocol", but then I
double-checked the schema, and realized there is.  It's just named @file
here.  We should rename it to match the schema.

Again, the use of QERR_INVALID_PARAMETER is wrong: @protocol is valid,
its value isn't.

More: we should use qemu_create() instead of qemu_open_old(), to not
throw away qemu_open_internal()'s error.


> +        error_setg(errp, "Invalid parameter 'protocol'");
>          return;
>      }
>  
> diff --git a/qga/commands.c b/qga/commands.c
> index 09c683e263..871210ab0b 100644
> --- a/qga/commands.c
> +++ b/qga/commands.c
> @@ -154,7 +154,7 @@ GuestExecStatus *qmp_guest_exec_status(int64_t pid, Error 
> **errp)
>  
>      gei = guest_exec_info_find(pid);
>      if (gei == NULL) {
> -        error_setg(errp, QERR_INVALID_PARAMETER, "pid");

Again, the use of QERR_INVALID_PARAMETER is wrong: @pid is valid, its
value isn't.

> +        error_setg(errp, "Invalid parameter 'pid'");
>          return NULL;
>      }
>  
> diff --git a/ui/ui-qmp-cmds.c b/ui/ui-qmp-cmds.c
> index debc07d678..41ca0100e7 100644
> --- a/ui/ui-qmp-cmds.c
> +++ b/ui/ui-qmp-cmds.c
> @@ -44,7 +44,7 @@ void qmp_set_password(SetPasswordOptions *opts, Error 
> **errp)
>          assert(opts->protocol == DISPLAY_PROTOCOL_VNC);
>          if (opts->connected != SET_PASSWORD_ACTION_KEEP) {
>              /* vnc supports "connected=keep" only */
> -            error_setg(errp, QERR_INVALID_PARAMETER, "connected");

Same misuse of QERR_INVALID_PARAMETER.

> +            error_setg(errp, "Invalid parameter 'connected'");
>              return;
>          }
>          /*
> diff --git a/util/qemu-option.c b/util/qemu-option.c
> index eedd08929b..fb391a7904 100644
> --- a/util/qemu-option.c
> +++ b/util/qemu-option.c
> @@ -612,7 +612,7 @@ QemuOpts *qemu_opts_create(QemuOptsList *list, const char 
> *id,
>  
>      if (list->merge_lists) {
>          if (id) {
> -            error_setg(errp, QERR_INVALID_PARAMETER, "id");

This one is correct.

> +            error_setg(errp, "Invalid parameter 'id'");
>              return NULL;
>          }
>          opts = qemu_opts_find(list, NULL);

Score: 1 out of 6 points :)

None of this is your patch's fault.