Re: [PATCH v24 01/10] acpi: nvdimm: change NVDIMM_UUID_LE to a common macro

[Peter Maydell]

On Mon, 17 Feb 2020 at 13:10, Dongjiu Geng <address@hidden> wrote:
>
> The little end UUID is used in many places, so make
> NVDIMM_UUID_LE to a common macro to convert the UUID
> to a little end array.
>
> Signed-off-by: Dongjiu Geng <address@hidden>
> Reviewed-by: Xiang Zheng <address@hidden>
> ---
>  hw/acpi/nvdimm.c    | 8 ++------
>  include/qemu/uuid.h | 5 +++++
>  2 files changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
> index 9fdad6d..232b701 100644
> --- a/hw/acpi/nvdimm.c
> +++ b/hw/acpi/nvdimm.c
> @@ -27,6 +27,7 @@
>   */
>
>  #include "qemu/osdep.h"
> +#include "qemu/uuid.h"
>  #include "hw/acpi/acpi.h"
>  #include "hw/acpi/aml-build.h"
>  #include "hw/acpi/bios-linker-loader.h"
> @@ -60,17 +61,12 @@ static GSList *nvdimm_get_device_list(void)
>      return list;
>  }
>
> -#define NVDIMM_UUID_LE(a, b, c, d0, d1, d2, d3, d4, d5, d6, d7)             \
> -   { (a) & 0xff, ((a) >> 8) & 0xff, ((a) >> 16) & 0xff, ((a) >> 24) & 0xff, \
> -     (b) & 0xff, ((b) >> 8) & 0xff, (c) & 0xff, ((c) >> 8) & 0xff,          \
> -     (d0), (d1), (d2), (d3), (d4), (d5), (d6), (d7) }
> -
>  /*
>   * define Byte Addressable Persistent Memory (PM) Region according to
>   * ACPI 6.0: 5.2.25.1 System Physical Address Range Structure.
>   */
>  static const uint8_t nvdimm_nfit_spa_uuid[] =
> -      NVDIMM_UUID_LE(0x66f0d379, 0xb4f3, 0x4074, 0xac, 0x43, 0x0d, 0x33,
> +      UUID_LE(0x66f0d379, 0xb4f3, 0x4074, 0xac, 0x43, 0x0d, 0x33,
>                       0x18, 0xb7, 0x8c, 0xdb);

You need to fix up the indentation on this following line.

>
>  /*
> diff --git a/include/qemu/uuid.h b/include/qemu/uuid.h
> index 129c45f..bd38af5 100644
> --- a/include/qemu/uuid.h
> +++ b/include/qemu/uuid.h
> @@ -34,6 +34,11 @@ typedef struct {
>      };
>  } QemuUUID;
>
> +#define UUID_LE(a, b, c, d0, d1, d2, d3, d4, d5, d6, d7)             \
> +  { (a) & 0xff, ((a) >> 8) & 0xff, ((a) >> 16) & 0xff, ((a) >> 24) & 0xff, \
> +     (b) & 0xff, ((b) >> 8) & 0xff, (c) & 0xff, ((c) >> 8) & 0xff,          \
> +     (d0), (d1), (d2), (d3), (d4), (d5), (d6), (d7) }
> +

If you want to make this a macro in a visible-to-the-rest-of-QEMU
header file, can you provide a documentation comment please that
describes what the macro is for? It would also be useful to
give the arguments (which should be documented in the doc comment)
more descriptive names than a, b, c...

>  #define UUID_FMT "%02hhx%02hhx%02hhx%02hhx-" \
>                   "%02hhx%02hhx-%02hhx%02hhx-" \
>                   "%02hhx%02hhx-" \
> --
> 1.8.3.1


thanks
-- PMM