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

[Michael S. Tsirkin]

On Wed, Jan 28, 2015 at 11:50:14AM +0100, Igor Mammedov wrote:
> 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.

If the cleaner API is just a removed parameter, a single sparse
patch will do it automatically.
Something like the following:

identifier func;
identifier call;
@@

-func(AmlPool *p, ...)
+func(...)
{
-call(p, ...)
+call(...)
}

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

The issue is lifetime of objects being implicit in the API,
it doesn't look like a global pool fixes that.


> Otherwise lets use common pattern and go QOM way, which also takes
> care about use-after-free concern but lacks garbage collector.