[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: |
Claudio Fontana |
Subject: |
Re: [Qemu-devel] [PATCH v2 01/47] acpi: introduce AML composer aml_append() |
Date: |
Wed, 28 Jan 2015 11:32:28 +0100 |
User-agent: |
Mozilla/5.0 (Windows NT 6.1; rv:24.0) Gecko/20100101 Thunderbird/24.0.1 |
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" <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.
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
- [Qemu-devel] [PATCH 11/13] acpi: acpi_add_table() to common cross target file, (continued)
- [Qemu-devel] [PATCH 11/13] acpi: acpi_add_table() to common cross target file, Igor Mammedov, 2015/01/28
- [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 <=
- Re: [Qemu-devel] [PATCH v2 01/47] acpi: introduce AML composer aml_append(), Shannon Zhao, 2015/01/29
- 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