Re: [Qemu-devel] [PATCH 4/9] acpi: build_append_nameseg(): add padding if necessary

[Michael S. Tsirkin]

On Mon, Dec 08, 2014 at 04:08:03PM +0000, Igor Mammedov wrote:
> According to ACPI spec NameSeg shorter than 4 characters
> must be padded up to 4 characters with "_" symbol.
> ACPI 5.0:  20.2.2 "Name Objects Encoding"
> 
> Do it in build_append_nameseg() so that caller shouldn't know
> or care about it.
> 
> Signed-off-by: Igor Mammedov <address@hidden>

To me just doing it right in callers seems just as easy, but
I guess you disagree :)

> ---
>  hw/i386/acpi-build.c | 18 +++++++++++++-----
>  1 file changed, 13 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index f5ec66a..a8b7a2b 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -292,6 +292,8 @@ static inline void build_append_array(GArray *array, 
> GArray *val)
>      g_array_append_vals(array, val->data, val->len);
>  }
>  
> +#define ACPI_NAMESEG_LEN 4
> +
>  static void GCC_FMT_ATTR(2, 3)
>  build_append_nameseg(GArray *array, const char *format, ...)
>  {
> @@ -299,13 +301,19 @@ build_append_nameseg(GArray *array, const char *format, 
> ...)
>      char s[] = "XXXX";
>      int len;
>      va_list args;
> +    const char padding = '_';
>  
>      va_start(args, format);
>      len = vsnprintf(s, sizeof s, format, args);
>      va_end(args);
>  
> -    assert(len == 4);
> +    g_assert(len <= ACPI_NAMESEG_LEN);

I'm not sure when is g_assert preferable to assert.
What's the motivation here?


> +
>      g_array_append_vals(array, s, len);
> +    while (len != ACPI_NAMESEG_LEN) {
> +        g_array_append_val(array, padding);
> +        ++len;
> +    }

Easier

        /* Pad up to 4 characters if necessary. */
        g_array_append_vals(array, "____", 4 - len);



>  }
>  
>  /* 5.4 Definition Block Encoding */
> @@ -846,7 +854,7 @@ static void build_pci_bus_end(PCIBus *bus, void 
> *bus_state)
>  
>      if (bus->parent_dev) {
>          op = 0x82; /* DeviceOp */
> -        build_append_nameseg(bus_table, "S%.02X_",
> +        build_append_nameseg(bus_table, "S%.02X",
>                               bus->parent_dev->devfn);
>          build_append_byte(bus_table, 0x08); /* NameOp */
>          build_append_nameseg(bus_table, "_SUN");
> @@ -966,7 +974,7 @@ static void build_pci_bus_end(PCIBus *bus, void 
> *bus_state)
>              build_append_int(notify, 0x1U << i);
>              build_append_byte(notify, 0x00); /* NullName */
>              build_append_byte(notify, 0x86); /* NotifyOp */
> -            build_append_nameseg(notify, "S%.02X_", PCI_DEVFN(i, 0));
> +            build_append_nameseg(notify, "S%.02X", PCI_DEVFN(i, 0));
>              build_append_byte(notify, 0x69); /* Arg1Op */
>  
>              /* Pack it up */
> @@ -1023,7 +1031,7 @@ static void build_pci_bus_end(PCIBus *bus, void 
> *bus_state)
>          if (bus->parent_dev) {
>              build_append_byte(parent->notify_table, '^'); /* 
> ParentPrefixChar */
>              build_append_byte(parent->notify_table, 0x2E); /* DualNamePrefix 
> */
> -            build_append_nameseg(parent->notify_table, "S%.02X_",
> +            build_append_nameseg(parent->notify_table, "S%.02X",
>                                   bus->parent_dev->devfn);
>              build_append_nameseg(parent->notify_table, "PCNT");
>          }
> @@ -1093,7 +1101,7 @@ build_ssdt(GArray *table_data, GArray *linker,
>          GArray *sb_scope = build_alloc_array();
>          uint8_t op = 0x10; /* ScopeOp */
>  
> -        build_append_nameseg(sb_scope, "_SB_");
> +        build_append_nameseg(sb_scope, "_SB");
>  
>          /* build Processor object for each processor */
>          for (i = 0; i < acpi_cpus; i++) {
> -- 
> 1.8.3.1