Re: [PATCH v2 16/22] qapi: Inline QERR_MISSING_PARAMETER definition (constant parameter)

[Markus Armbruster <armbru@redhat.com>]

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 patches:
>
>     @match@
>     expression errp;
>     constant param;
>     @@
>          error_setg(errp, QERR_MISSING_PARAMETER, param);
>
>     @script:python strformat depends on match@
>     param << match.param;
>     fixedfmt; // new var
>     @@
>     if param[0] == '"': # Format skipping '"',
>         fixedfmt = f'"Parameter \'{param[1:-1]}\' is missing"'
>     else: # or use definition.
>         fixedfmt = f'"Parameter " {param} " is missing"'
>     coccinelle.fixedfmt = cocci.make_ident(fixedfmt)
>
>     @replace@
>     expression match.errp;
>     constant match.param;
>     identifier strformat.fixedfmt;
>     @@
>     -    error_setg(errp, QERR_MISSING_PARAMETER, param);
>     +    error_setg(errp, fixedfmt);
>
> and:
>
>     @match@
>     constant param;
>     @@
>          error_report(QERR_MISSING_PARAMETER, param);
>
>     @script:python strformat depends on match@
>     param << match.param;
>     fixedfmt; // new var
>     @@
>     fixedfmt = f'"Parameter \'{param[1:-1]}\' is missing"'
>     coccinelle.fixedfmt = cocci.make_ident(fixedfmt)
>
>     @replace@
>     constant match.param;
>     identifier strformat.fixedfmt;
>     @@
>     -    error_report(QERR_MISSING_PARAMETER, param);
>     +    error_report(fixedfmt);
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
> ---
>  backends/dbus-vmstate.c        |  2 +-
>  block/gluster.c                | 21 +++++++++++----------
>  block/monitor/block-hmp-cmds.c |  6 +++---
>  dump/dump.c                    |  4 ++--
>  hw/usb/redirect.c              |  2 +-
>  softmmu/qdev-monitor.c         |  2 +-
>  softmmu/tpm.c                  |  4 ++--
>  softmmu/vl.c                   |  4 ++--
>  ui/input-barrier.c             |  2 +-
>  ui/ui-qmp-cmds.c               |  2 +-
>  10 files changed, 25 insertions(+), 24 deletions(-)
>
> diff --git a/backends/dbus-vmstate.c b/backends/dbus-vmstate.c
> index 57369ec0f2..e781ded17c 100644
> --- a/backends/dbus-vmstate.c
> +++ b/backends/dbus-vmstate.c
> @@ -413,7 +413,7 @@ dbus_vmstate_complete(UserCreatable *uc, Error **errp)
>      }
>  
>      if (!self->dbus_addr) {
> -        error_setg(errp, QERR_MISSING_PARAMETER, "addr");

Misuse of QERR_MISSING_PARAMETER.

This function is interface UserCreatableClass method complete(), which
runs right after an object with this interface was created.
"Parameters" need not exist in this context.

The actual issue is property "addr" has not been set.  So let's report
that: "property 'addr' is required".

Separate patch, to keep this one mechanical.

> +        error_setg(errp, "Parameter 'addr' is missing");
>          return;
>      }
>  
> diff --git a/block/gluster.c b/block/gluster.c
> index ad5fadbe79..8d97d698c3 100644
> --- a/block/gluster.c
> +++ b/block/gluster.c
> @@ -530,20 +530,20 @@ static int qemu_gluster_parse_json(BlockdevOptionsGluster *gconf,
>  
>      num_servers = qdict_array_entries(options, GLUSTER_OPT_SERVER_PATTERN);
>      if (num_servers < 1) {
> -        error_setg(&local_err, QERR_MISSING_PARAMETER, "server");
> +        error_setg(&local_err, "Parameter 'server' is missing");
>          goto out;
>      }
>  
>      ptr = qemu_opt_get(opts, GLUSTER_OPT_VOLUME);
>      if (!ptr) {
> -        error_setg(&local_err, QERR_MISSING_PARAMETER, GLUSTER_OPT_VOLUME);
> +        error_setg(&local_err, "Parameter " GLUSTER_OPT_VOLUME " is missing");
>          goto out;
>      }
>      gconf->volume = g_strdup(ptr);
>  
>      ptr = qemu_opt_get(opts, GLUSTER_OPT_PATH);
>      if (!ptr) {
> -        error_setg(&local_err, QERR_MISSING_PARAMETER, GLUSTER_OPT_PATH);
> +        error_setg(&local_err, "Parameter " GLUSTER_OPT_PATH " is missing");
>          goto out;
>      }
>      gconf->path = g_strdup(ptr);
> @@ -562,7 +562,8 @@ static int qemu_gluster_parse_json(BlockdevOptionsGluster *gconf,
>  
>          ptr = qemu_opt_get(opts, GLUSTER_OPT_TYPE);
>          if (!ptr) {
> -            error_setg(&local_err, QERR_MISSING_PARAMETER, GLUSTER_OPT_TYPE);
> +            error_setg(&local_err,
> +                       "Parameter " GLUSTER_OPT_TYPE " is missing");
>              error_append_hint(&local_err, GERR_INDEX_HINT, i);
>              goto out;
>  
> @@ -592,16 +593,16 @@ static int qemu_gluster_parse_json(BlockdevOptionsGluster *gconf,
>  
>              ptr = qemu_opt_get(opts, GLUSTER_OPT_HOST);
>              if (!ptr) {
> -                error_setg(&local_err, QERR_MISSING_PARAMETER,
> -                           GLUSTER_OPT_HOST);
> +                error_setg(&local_err,
> +                           "Parameter " GLUSTER_OPT_HOST " is missing");
>                  error_append_hint(&local_err, GERR_INDEX_HINT, i);
>                  goto out;
>              }
>              gsconf->u.inet.host = g_strdup(ptr);
>              ptr = qemu_opt_get(opts, GLUSTER_OPT_PORT);
>              if (!ptr) {
> -                error_setg(&local_err, QERR_MISSING_PARAMETER,
> -                           GLUSTER_OPT_PORT);
> +                error_setg(&local_err,
> +                           "Parameter " GLUSTER_OPT_PORT " is missing");
>                  error_append_hint(&local_err, GERR_INDEX_HINT, i);
>                  goto out;
>              }
> @@ -648,8 +649,8 @@ static int qemu_gluster_parse_json(BlockdevOptionsGluster *gconf,
>                  goto out;
>              }
>              if (!ptr) {
> -                error_setg(&local_err, QERR_MISSING_PARAMETER,
> -                           GLUSTER_OPT_PATH);
> +                error_setg(&local_err,
> +                           "Parameter " GLUSTER_OPT_PATH " is missing");
>                  error_append_hint(&local_err, GERR_INDEX_HINT, i);
>                  goto out;
>              }

I suspect pretty much everything you patch in this file is bad, but I'm
running out of steam.

> diff --git a/block/monitor/block-hmp-cmds.c b/block/monitor/block-hmp-cmds.c
> index ca2599de44..90e593ed38 100644
> --- a/block/monitor/block-hmp-cmds.c
> +++ b/block/monitor/block-hmp-cmds.c
> @@ -252,7 +252,7 @@ void hmp_drive_mirror(Monitor *mon, const QDict *qdict)
>      };
>  
>      if (!filename) {
> -        error_setg(&err, QERR_MISSING_PARAMETER, "target");
> +        error_setg(&err, "Parameter 'target' is missing");
>          goto end;
>      }
>      qmp_drive_mirror(&mirror, &err);
> @@ -281,7 +281,7 @@ void hmp_drive_backup(Monitor *mon, const QDict *qdict)
>      };
>  
>      if (!filename) {
> -        error_setg(&err, QERR_MISSING_PARAMETER, "target");
> +        error_setg(&err, "Parameter 'target' is missing");
>          goto end;
>      }
>  
> @@ -356,7 +356,7 @@ void hmp_snapshot_blkdev(Monitor *mon, const QDict *qdict)
>           * In the future, if 'snapshot-file' is not specified, the snapshot
>           * will be taken internally. Today it's actually required.
>           */
> -        error_setg(&err, QERR_MISSING_PARAMETER, "snapshot-file");
> +        error_setg(&err, "Parameter 'snapshot-file' is missing");
>          goto end;
>      }
>  
> diff --git a/dump/dump.c b/dump/dump.c
> index e173f1f14c..642b952985 100644
> --- a/dump/dump.c
> +++ b/dump/dump.c
> @@ -2096,11 +2096,11 @@ void qmp_dump_guest_memory(bool paging, const char *file,
>          return;
>      }
>      if (has_begin && !has_length) {
> -        error_setg(errp, QERR_MISSING_PARAMETER, "length");
> +        error_setg(errp, "Parameter 'length' is missing");
>          return;
>      }
>      if (!has_begin && has_length) {
> -        error_setg(errp, QERR_MISSING_PARAMETER, "begin");
> +        error_setg(errp, "Parameter 'begin' is missing");
>          return;
>      }
>      if (has_detach) {
> diff --git a/hw/usb/redirect.c b/hw/usb/redirect.c
> index ac6fa34ad1..83bfc9dc19 100644
> --- a/hw/usb/redirect.c
> +++ b/hw/usb/redirect.c
> @@ -1426,7 +1426,7 @@ static void usbredir_realize(USBDevice *udev, Error **errp)
>      int i;
>  
>      if (!qemu_chr_fe_backend_connected(&dev->cs)) {
> -        error_setg(errp, QERR_MISSING_PARAMETER, "chardev");

Misuse of QERR_MISSING_PARAMETER.

This is a realize() method.  "Parameters" need not exist in this
context.

The actual issue is property "chardev" has not been set.  So let's
report that: "property 'filename' is required".

Separate patch, to keep this one mechanical.  Not mentioning this again
from now on.

> +        error_setg(errp, "Parameter 'chardev' is missing");
>          return;
>      }
>  
> diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
> index b17aec4357..b7b2bf18d4 100644
> --- a/softmmu/qdev-monitor.c
> +++ b/softmmu/qdev-monitor.c
> @@ -622,7 +622,7 @@ DeviceState *qdev_device_add_from_qdict(const QDict *opts,
>  
>      driver = qdict_get_try_str(opts, "driver");
>      if (!driver) {
> -        error_setg(errp, QERR_MISSING_PARAMETER, "driver");
> +        error_setg(errp, "Parameter 'driver' is missing");
>          return NULL;
>      }
>  
> diff --git a/softmmu/tpm.c b/softmmu/tpm.c
> index 8437c4efc3..3a7d4b5c67 100644
> --- a/softmmu/tpm.c
> +++ b/softmmu/tpm.c
> @@ -106,13 +106,13 @@ static int tpm_init_tpmdev(void *dummy, QemuOpts *opts, Error **errp)
>  
>      id = qemu_opts_id(opts);
>      if (id == NULL) {
> -        error_report(QERR_MISSING_PARAMETER, "id");
> +        error_report("Parameter 'id' is missing");
>          return 1;
>      }
>  
>      value = qemu_opt_get(opts, "type");
>      if (!value) {
> -        error_report(QERR_MISSING_PARAMETER, "type");
> +        error_report("Parameter 'type' is missing");
>          tpm_display_backend_drivers();
>          return 1;
>      }
> diff --git a/softmmu/vl.c b/softmmu/vl.c
> index 98e071e63b..840ac84069 100644
> --- a/softmmu/vl.c
> +++ b/softmmu/vl.c
> @@ -1801,7 +1801,7 @@ static void object_option_parse(const char *optarg)
>  
>          type = qemu_opt_get(opts, "qom-type");
>          if (!type) {
> -            error_setg(&error_fatal, QERR_MISSING_PARAMETER, "qom-type");
> +            error_setg(&error_fatal, "Parameter 'qom-type' is missing");
>          }
>          if (user_creatable_print_help(type, opts)) {
>              exit(0);
> @@ -2266,7 +2266,7 @@ static int do_configure_accelerator(void *opaque, QemuOpts *opts, Error **errp)
>      bool qtest_with_kvm;
>  
>      if (!acc) {
> -        error_setg(errp, QERR_MISSING_PARAMETER, "accel");
> +        error_setg(errp, "Parameter 'accel' is missing");
>          goto bad;
>      }
>  
> diff --git a/ui/input-barrier.c b/ui/input-barrier.c
> index 2d57ca7079..ecbba4adc2 100644
> --- a/ui/input-barrier.c
> +++ b/ui/input-barrier.c
> @@ -493,7 +493,7 @@ static void input_barrier_complete(UserCreatable *uc, Error **errp)
>      Error *local_err = NULL;
>  
>      if (!ib->name) {
> -        error_setg(errp, QERR_MISSING_PARAMETER, "name");

Misuse of QERR_MISSING_PARAMETER.

This function is interface UserCreatableClass method complete(), which
runs right after an object with this interface was created.
"Parameters" need not exist in this context.

The actual issue is property "name" has not been set.  So let's report
that: "property 'name' is required".

> +        error_setg(errp, "Parameter 'name' is missing");
>          return;
>      }
>  
> diff --git a/ui/ui-qmp-cmds.c b/ui/ui-qmp-cmds.c
> index a95fd35948..0e350fc333 100644
> --- a/ui/ui-qmp-cmds.c
> +++ b/ui/ui-qmp-cmds.c
> @@ -195,7 +195,7 @@ void qmp_client_migrate_info(const char *protocol, const char *hostname,
>          }
>  
>          if (!has_port && !has_tls_port) {
> -            error_setg(errp, QERR_MISSING_PARAMETER, "port/tls-port");

Bad error message.  The actual issue is both "port" and "tls-port" are
missing, but we need at least one.  So let's report that: "parameter
'port' or 'tls-port' is required".

> +            error_setg(errp, "Parameter 'port/tls-port' is missing");
>              return;
>          }