From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:39930) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YGPRI-0002No-6B for qemu-devel@nongnu.org; Wed, 28 Jan 2015 05:01:37 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YGPRD-0001Rv-VW for qemu-devel@nongnu.org; Wed, 28 Jan 2015 05:01:36 -0500 Received: from mx1.redhat.com ([209.132.183.28]:38556) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YGPRD-0001Ro-Nl for qemu-devel@nongnu.org; Wed, 28 Jan 2015 05:01:31 -0500 Date: Wed, 28 Jan 2015 11:00:23 +0100 From: Igor Mammedov Message-ID: <20150128110023.160e5d24@nial.brq.redhat.com> In-Reply-To: <20150128075626.GA16660@redhat.com> References: <20150123132424.GA4579@redhat.com> <20150123144030.21142ed1@nial.brq.redhat.com> <20150123135511.GF4579@redhat.com> <20150123185620.604b83ab@nial.brq.redhat.com> <20150124163350.GC6293@redhat.com> <20150126105721.60641e59@nial.brq.redhat.com> <20150126160920.79a09b37@nial.brq.redhat.com> <20150126153400.GB3536@hawk.usersys.redhat.com> <20150126161755.GA5487@redhat.com> <20150127232909.6c045626@igors-macbook-pro.local> <20150128075626.GA16660@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v2 01/47] acpi: introduce AML composer aml_append() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Michael S. Tsirkin" Cc: Andrew Jones , marcel.a@redhat.com, claudio.fontana@huawei.com, qemu-devel@nongnu.org, Shannon Zhao , pbonzini@redhat.com On Wed, 28 Jan 2015 09:56:26 +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: > > 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.