[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 01/47] acpi: introduce AML composer aml_appen
From: |
Shannon Zhao |
Subject: |
Re: [Qemu-devel] [PATCH v2 01/47] acpi: introduce AML composer aml_append() |
Date: |
Thu, 29 Jan 2015 15:46:32 +0800 |
User-agent: |
Mozilla/5.0 (Windows NT 6.1; rv:24.0) Gecko/20100101 Thunderbird/24.4.0 |
On 2015/1/28 18:00, 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. 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
- [Qemu-devel] [PATCH 12/13] acpi: prepare for API internal collection of RSDT entries, (continued)
- [Qemu-devel] [PATCH 12/13] acpi: prepare for API internal collection of RSDT entries, Igor Mammedov, 2015/01/28
- [Qemu-devel] [PATCH 13/13] i386: acpi: mark SSDT as RSDT entry so API would add entry to RSDT automatically, Igor Mammedov, 2015/01/28
- [Qemu-devel] [PATCH 06/13] acpi: use TYPE_AML_OBJECT for toplevel ACPI tables blob, Igor Mammedov, 2015/01/28
- Re: [Qemu-devel] [PATCH 00/13] convert AML API to QOM, Andrew Jones, 2015/01/28
- Re: [Qemu-devel] [PATCH v2 01/47] acpi: introduce AML composer aml_append(), Michael S. Tsirkin, 2015/01/28
- Re: [Qemu-devel] [PATCH v2 01/47] acpi: introduce AML composer aml_append(), Igor Mammedov, 2015/01/28
- Re: [Qemu-devel] [PATCH v2 01/47] acpi: introduce AML composer aml_append(), Michael S. Tsirkin, 2015/01/28
- Re: [Qemu-devel] [PATCH v2 01/47] acpi: introduce AML composer aml_append(), Igor Mammedov, 2015/01/28
- Re: [Qemu-devel] [PATCH v2 01/47] acpi: introduce AML composer aml_append(), Michael S. Tsirkin, 2015/01/28
- Re: [Qemu-devel] [PATCH v2 01/47] acpi: introduce AML composer aml_append(), Claudio Fontana, 2015/01/28
- Re: [Qemu-devel] [PATCH v2 01/47] acpi: introduce AML composer aml_append(),
Shannon Zhao <=
- Re: [Qemu-devel] [PATCH v2 01/47] acpi: introduce AML composer aml_append(), Igor Mammedov, 2015/01/29
- Re: [Qemu-devel] [PATCH v2 01/47] acpi: introduce AML composer aml_append(), Andrew Jones, 2015/01/28
Re: [Qemu-devel] [PATCH v2 01/47] acpi: introduce AML composer aml_append(), Michael S. Tsirkin, 2015/01/23
[Qemu-devel] [PATCH v2 09/47] acpi: add acpi_int() term, Igor Mammedov, 2015/01/22
Re: [Qemu-devel] [PATCH v2 00/47] ACPI refactoring: replace template patching with C ASL API, Michael S. Tsirkin, 2015/01/28