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

[Michael S. Tsirkin]

On Tue, Dec 09, 2014 at 11:32:45AM +0100, Igor Mammedov wrote:
> On Mon, 8 Dec 2014 23:15:32 +0200
> "Michael S. Tsirkin" <address@hidden> wrote:
> 
> > 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 :)
> That means, author MUST know about padding, if he/she doesn't
> or forget about it that would introduce error usually resulting
> in BSOD.

Not really. assert will trigger on qemu start.

> This patch allows to avoid such mistakes.

Even the most basic testing (e.g. make check) will find these mistakes.
It's more a question of which API we prefer.

Anyway, could you respond on g_assert vs assert please?

> > 
> > > ---
> > >  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