Re: [Qemu-devel] [PATCH 7/9] acpi: replace opencoded notify codes with named values

[Michael S. Tsirkin]

On Mon, Dec 08, 2014 at 04:08:06PM +0000, Igor Mammedov wrote:
> Signed-off-by: Igor Mammedov <address@hidden>

I'm not sure this makes sense for these constants.

Device Check seems more readable than ACPI_DEV_CHK.
If your object here is readability, please do not
abbreviate.

Generally ability to match spec names exactly is
what made me prefer code comments to enums for one-off constants.
Looking up "Device Check" works in any spec version,
I don't have to dig up the exact one, find table by name,
break my eyes trying to locate the correct line in
a huge table.
Just text search, and the correct line is highlighted.

Why look in another spec version you might ask?
Well it's definitely helpful to understand legacy
guest quirks.


> ---
>  hw/i386/acpi-build.c             | 4 ++--
>  include/hw/acpi/acpi_gen_utils.h | 6 ++++++
>  2 files changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 0f6202d..a33d130 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -840,10 +840,10 @@ static void build_pci_bus_end(PCIBus *bus, void 
> *bus_state)
>              build_append_namestring(method, "BNUM");
>              build_append_namestring(method, "DVNT");
>              build_append_namestring(method, "PCIU");
> -            build_append_int(method, 1); /* Device Check */
> +            build_append_int(method, ACPI_DEV_CHK);
>              build_append_namestring(method, "DVNT");
>              build_append_namestring(method, "PCID");
> -            build_append_int(method, 3); /* Eject Request */
> +            build_append_int(method, ACPI_DEV_EJ);
>          }
>  
>          /* Notify about child bus events in any case */
> diff --git a/include/hw/acpi/acpi_gen_utils.h 
> b/include/hw/acpi/acpi_gen_utils.h
> index fd50625..ce76dc1 100644
> --- a/include/hw/acpi/acpi_gen_utils.h
> +++ b/include/hw/acpi/acpi_gen_utils.h
> @@ -5,6 +5,12 @@
>  #include <glib.h>
>  #include "qemu/compiler.h"
>  
> +/* ACPI 5.0: table "Device Object Notification Values" */
> +enum {
> +    ACPI_DEV_CHK = 1,
> +    ACPI_DEV_EJ = 3,
> +};
> +
>  GArray *build_alloc_array(void);
>  void build_free_array(GArray *array);
>  void build_prepend_byte(GArray *array, uint8_t val);
> -- 
> 1.8.3.1