| Message ID | 20150128075626.GA16660@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Wed, 28 Jan 2015 09:56:26 +0200 "Michael S. Tsirkin" <mst@redhat.com> 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. If possible user shouldn't care about it and concentrate on composing AML instead. Whole point of passing AmlPool and record every allocation is to avoid mistakes like: acpi_if(acpi_and(acpi_arg0(), acpi_int(0x1U << slot))); and forgetting to assign object returned by call anywhere, it's basically the same as calling malloc() without using result anywhere, however neither libc nor glib force user to pass allocator (in our case garbage collector) in every call that allocates memory. Let's just follow common convention here (#4) where an object is allocated by API call (i.e like object_new(FOO), gtk_widget_foo()). Hence is suggesting at least to hide AmlPool internally in API without exposing it to user. We can provide for user init/free API to manage internal AmlPool manually, allowing him to select when to do initialization and cleanup. Claudio, Marcel, Shannon, As the first API users, could you give your feedback on the topic.
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" <mst@redhat.com> 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.
Hello Igor, It's quite difficult to understand all the different options, there have been quite a few flying around. I'll comment this mail in particular, ignoring for the moment all the other exchanges (about QOM etc). On 28.01.2015 11:00, Igor Mammedov wrote: > On Wed, 28 Jan 2015 09:56:26 +0200 > "Michael S. Tsirkin" <mst@redhat.com> 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. I think both options are ok. The extra parameter is just basically passed around if I understand correctly, that doesn't seem terrible. > every usage of aml_foo()/build_append_pcihp_notify_entry() tags along > extra pointer which is not really necessary for user to know. If possible > user shouldn't care about it and concentrate on composing AML instead. > > Whole point of passing AmlPool and record every allocation is to avoid > mistakes like: > > acpi_if(acpi_and(acpi_arg0(), acpi_int(0x1U << slot))); > > and forgetting to assign object returned by call anywhere, > it's basically the same as calling malloc() without > using result anywhere, however neither libc nor glib > force user to pass allocator (in our case garbage collector) > in every call that allocates memory. Let's just follow common > convention here (#4) where an object is allocated by API call > (i.e like object_new(FOO), gtk_widget_foo()). > > Hence is suggesting at least to hide AmlPool internally in API > without exposing it to user. We can provide for user > init/free API to manage internal AmlPool manually, allowing > him to select when to do initialization and cleanup. > > Claudio, Marcel, Shannon, > As the first API users, could you give your feedback on the topic. > Personally I find as mentioned both options ok. Your (Igor's) proposal "looks better" when staring at the code which uses the API, Michael's suggestion is to avoid any confusion around when an object is allocated / freed, and that's a valid point. Sorry for being so neutral, but really both options seem to have their merits and both seem substantially fine to me. Ciao, Claudio
On Wed, Jan 28, 2015 at 09:56:26AM +0200, Michael S. Tsirkin 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: I'm not so sure. Looking at the version below, I find the acpi_arg1(p) the most distracting. That API call creates the simplest object, so should be the simplest looking. Actually, you suggested that acpi_arg1(), a wrapper to make things look even simpler, wasn't necessary, acpi_arg(1) would be fine. I agree with that, but now we'd have acpi_arg(p, 1), which is really starting to clutter an AML composition built with many such calls. > > 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))); ^ forgot your p here > + 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. It's probably just a personal preference thing. Igor and I prefer the sequence of AML composing calls to appear as simple as possible, i.e. develop the cleanest API as possible. To do this we need to find ways to hide the memory management, which comes at a cost of using a model that supports garbage collection, or adding a global variable to hide the pool. Your preference appears to be to keep memory management as simple and explicit as possible, at the expense of peppering each AML build function with a bunch of 'p's. I agree with Igor that we should get votes from the initial consumers of this API. Thanks, drew
On Wed, 28 Jan 2015 12:24:23 +0200 "Michael S. Tsirkin" <mst@redhat.com> 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" <mst@redhat.com> 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.
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" <mst@redhat.com> 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" <mst@redhat.com> 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.
On 2015/1/28 18:00, Igor Mammedov wrote: > On Wed, 28 Jan 2015 09:56:26 +0200 > "Michael S. Tsirkin" <mst@redhat.com> 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. If possible > user shouldn't care about it and concentrate on composing AML instead. > > Whole point of passing AmlPool and record every allocation is to avoid > mistakes like: > > acpi_if(acpi_and(acpi_arg0(), acpi_int(0x1U << slot))); > > and forgetting to assign object returned by call anywhere, > it's basically the same as calling malloc() without > using result anywhere, however neither libc nor glib > force user to pass allocator (in our case garbage collector) > in every call that allocates memory. Let's just follow common > convention here (#4) where an object is allocated by API call > (i.e like object_new(FOO), gtk_widget_foo()). > > Hence is suggesting at least to hide AmlPool internally in API > without exposing it to user. We can provide for user > init/free API to manage internal AmlPool manually, allowing > him to select when to do initialization and cleanup. > > Claudio, Marcel, Shannon, > As the first API users, could you give your feedback on the topic. > In my opinion, it's good to make users focused on ACPI table construction through auto memory management. And it makes the code clear. PS: We're talking about use-after-free, like below example. But this example really exist? During generating ACPI tables for virt machine, I don't encounter this case. For example: aml_append(&a, b); aml_append(&a, b); Thanks, Shannon
On Thu, 29 Jan 2015 15:46:32 +0800 Shannon Zhao <zhaoshenglong@huawei.com> wrote: > On 2015/1/28 18:00, Igor Mammedov wrote: > > On Wed, 28 Jan 2015 09:56:26 +0200 > > "Michael S. Tsirkin" <mst@redhat.com> 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. If possible > > user shouldn't care about it and concentrate on composing AML instead. > > > > Whole point of passing AmlPool and record every allocation is to avoid > > mistakes like: > > > > acpi_if(acpi_and(acpi_arg0(), acpi_int(0x1U << slot))); > > > > and forgetting to assign object returned by call anywhere, > > it's basically the same as calling malloc() without > > using result anywhere, however neither libc nor glib > > force user to pass allocator (in our case garbage collector) > > in every call that allocates memory. Let's just follow common > > convention here (#4) where an object is allocated by API call > > (i.e like object_new(FOO), gtk_widget_foo()). > > > > Hence is suggesting at least to hide AmlPool internally in API > > without exposing it to user. We can provide for user > > init/free API to manage internal AmlPool manually, allowing > > him to select when to do initialization and cleanup. > > > > Claudio, Marcel, Shannon, > > As the first API users, could you give your feedback on the topic. > > > > In my opinion, it's good to make users focused on ACPI table construction through > auto memory management. And it makes the code clear. > > PS: > We're talking about use-after-free, like below example. But this example really exist? > During generating ACPI tables for virt machine, I don't encounter this case. > > For example: > aml_append(&a, b); > aml_append(&a, b); Thanks for your reply, yesterday we've came to a compromise, where API will stay as it is externally only it will take all AML arguments by pointer. And we would introduce allocation list internally inside API, User only would need to initialize it before using API and free it after using API. > > Thanks, > Shannon > >
On 01/28/2015 09:56 AM, Michael S. Tsirkin 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. A matter of taste, but I personally don't like this p everywhere, it makes everything hard to read. I am still wondering about the static variable that will hold all of the instances and will be explicitly freed at the end. Thanks, Marcel > >
On 01/28/2015 12:00 PM, Igor Mammedov wrote: > On Wed, 28 Jan 2015 09:56:26 +0200 [...] > > Hence is suggesting at least to hide AmlPool internally in API > without exposing it to user. We can provide for user > init/free API to manage internal AmlPool manually, allowing > him to select when to do initialization and cleanup. This I like the most. > > Claudio, Marcel, Shannon, > As the first API users, could you give your feedback on the topic. As a happy API user, I really like it as it is. Since we do need to address the allocation/freeing issues I like the internal AmlPool as I stated already in a few places :) Thanks, Marcel >
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,