Re: [PATCH v2 01/22] qapi: Inline and remove QERR_BUS_NO_HOTPLUG definition

[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 sed, manually
> removing the definition in include/qapi/qmp/qerror.h.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  include/qapi/qmp/qerror.h | 3 ---
>  hw/ppc/spapr_pci.c        | 4 ++--
>  softmmu/qdev-monitor.c    | 8 +++++---
>  3 files changed, 7 insertions(+), 8 deletions(-)
>
> diff --git a/include/qapi/qmp/qerror.h b/include/qapi/qmp/qerror.h
> index 8dd9fcb071..1a9c2d3502 100644
> --- a/include/qapi/qmp/qerror.h
> +++ b/include/qapi/qmp/qerror.h
> @@ -17,9 +17,6 @@
>   * add new ones!
>   */
>  
> -#define QERR_BUS_NO_HOTPLUG \
> -    "Bus '%s' does not support hotplugging"
> -
>  #define QERR_DEVICE_HAS_NO_MEDIUM \
>      "Device '%s' has no medium"
>  
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index 370c5a90f2..7f063f5852 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -1550,7 +1550,7 @@ static void spapr_pci_pre_plug(HotplugHandler 
> *plug_handler,
>           * we need to let them know it's not enabled
>           */
>          if (plugged_dev->hotplugged) {
> -            error_setg(errp, QERR_BUS_NO_HOTPLUG,
> +            error_setg(errp, "Bus '%s' does not support hotplugging",
>                         object_get_typename(OBJECT(phb)));

@phb is a SpaprPhbState *, thereforce object_get_typename() returns
"spapr-pci-host-bridge", which is not a bus.  It provides a bus: PCI bus
phb->parent_obj.bus.

Should be fixed on top, so this patch remains mechanical.

Well outside this patch's scope, but here goes anyway: I wonder why we
need this check.  Why is the generic check in
qdev_device_add_from_qdict() not enough?

>              return;
>          }
> @@ -1671,7 +1671,7 @@ static void spapr_pci_unplug_request(HotplugHandler 
> *plug_handler,
>      SpaprDrc *drc = drc_from_dev(phb, pdev);
>  
>      if (!phb->dr_enabled) {
> -        error_setg(errp, QERR_BUS_NO_HOTPLUG,
> +        error_setg(errp, "Bus '%s' does not support hotplugging",

Likewise.

>                     object_get_typename(OBJECT(phb)));
>          return;
>      }
> diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
> index 74f4e41338..3a9740dcbd 100644
> --- a/softmmu/qdev-monitor.c
> +++ b/softmmu/qdev-monitor.c
> @@ -656,7 +656,8 @@ DeviceState *qdev_device_add_from_qdict(const QDict *opts,
>  
>      if (qdev_should_hide_device(opts, from_json, errp)) {
>          if (bus && !qbus_is_hotpluggable(bus)) {
> -            error_setg(errp, QERR_BUS_NO_HOTPLUG, bus->name);
> +            error_setg(errp, "Bus '%s' does not support hotplugging",
> +                       bus->name);
>          }
>          return NULL;
>      } else if (*errp) {
> @@ -664,7 +665,7 @@ DeviceState *qdev_device_add_from_qdict(const QDict *opts,
>      }
>  
>      if (phase_check(PHASE_MACHINE_READY) && bus && 
> !qbus_is_hotpluggable(bus)) {
> -        error_setg(errp, QERR_BUS_NO_HOTPLUG, bus->name);
> +        error_setg(errp, "Bus '%s' does not support hotplugging", bus->name);
>          return NULL;
>      }
>  
> @@ -904,7 +905,8 @@ void qdev_unplug(DeviceState *dev, Error **errp)
>      }
>  
>      if (dev->parent_bus && !qbus_is_hotpluggable(dev->parent_bus)) {
> -        error_setg(errp, QERR_BUS_NO_HOTPLUG, dev->parent_bus->name);
> +        error_setg(errp, "Bus '%s' does not support hotplugging",
> +                   dev->parent_bus->name);
>          return;
>      }

Could factor out

    if (bus && !qbus_is_hotpluggable(bus)) {
        error_setg(errp, "Bus '%s' does not support hotplugging", bus->name);
        return false;
    }
    return true;

Idea, not a demand.