Re: [Qemu-devel] [PATCH v2 01/47] acpi: introduce AML composer aml_append()

[Michael S. Tsirkin]

On Wed, Jan 28, 2015 at 11:00:23AM +0100, Igor Mammedov wrote:
> On Wed, 28 Jan 2015 09:56:26 +0200
> "Michael S. Tsirkin" <address@hidden> wrote:
> 
> > > I've tried redo series with passing alloc list as first argument,
> > > looks ugly as hell
> > 
> > I tried too. Not too bad at all. See below:
> > 
> > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > index f66da5d..820504a 100644
> > --- a/hw/i386/acpi-build.c
> > +++ b/hw/i386/acpi-build.c
> > @@ -491,14 +491,14 @@ static void acpi_set_pci_info(void)
> >      }
> >  }
> >  
> > -static void build_append_pcihp_notify_entry(AcpiAml *method, int slot)
> > +static void build_append_pcihp_notify_entry(AmlPool *p, AcpiAml *method, 
> > int slot)
> >  {
> > -    AcpiAml if_ctx;
> > +    AcpiAml *if_ctx;
> >      int32_t devfn = PCI_DEVFN(slot, 0);
> >  
> > -    if_ctx = acpi_if(acpi_and(acpi_arg0(), acpi_int(0x1U << slot)));
> > -    aml_append(&if_ctx, acpi_notify(acpi_name("S%.02X", devfn), 
> > acpi_arg1()));
> > -    aml_append(method, if_ctx);
> > +    if_ctx = acpi_if(p, acpi_and(p, acpi_arg0(), acpi_int(p, 0x1U << 
> > slot)));
> > +    aml_append(p, if_ctx, acpi_notify(p, acpi_name(p, "S%.02X", devfn), 
> > acpi_arg1(p)));
> > +    aml_append(p, method, if_ctx);
> >  }
> >  
> >  static void build_append_pci_bus_devices(AcpiAml *parent_scope, PCIBus 
> > *bus,
> > 
> > What exactly is the problem?  A tiny bit more verbose but the lifetime
> > of all objects is now explicit.
> every usage of aml_foo()/build_append_pcihp_notify_entry() tags along
> extra pointer which is not really necessary for user to know.

It's necessary to make memory management explicit. Consider:

alloc_pool();
acpi_arg0();
free_pool();
acpi_arg1();

Looks ok but isn't because acpi_arg1 silently allocates memory.

p = alloc_pool();
acpi_arg0(p);
free_pool(p);
acpi_arg1(p);

It's obvious what's wrong here: p is used after it's freed.

Come on, it's 3 characters per call.  I think it's a reasonable
compromize.

-- 
MST