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

[Igor Mammedov]

On Wed, 28 Jan 2015 12:24:23 +0200
"Michael S. Tsirkin" <address@hidden> wrote:

> 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.
with pool hidden inside API, acpi_arg1() would crash
when accessing freed pool.

> 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.
it's just like above but more verbose with the same end result.
 
> Come on, it's 3 characters per call.  I think it's a reasonable
> compromize.
That characters will spread over all API usage and I don't really
wish to invent garbage collection and then rewrite everything
to a cleaner API afterwards.
I admit that internal global pool is not the best thing,
but that would be reasonable compromise to still get garbage
collection without polluting API.
Otherwise lets use common pattern and go QOM way, which also takes
care about use-after-free concern but lacks garbage collector.