From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:49871) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YGQ7u-0003Ev-0c for qemu-devel@nongnu.org; Wed, 28 Jan 2015 05:45:38 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YGQ7p-0007w4-Qx for qemu-devel@nongnu.org; Wed, 28 Jan 2015 05:45:37 -0500 Received: from mx1.redhat.com ([209.132.183.28]:53119) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YGQ7p-0007vq-HJ for qemu-devel@nongnu.org; Wed, 28 Jan 2015 05:45:33 -0500 Date: Wed, 28 Jan 2015 11:45:17 +0100 From: Andrew Jones Message-ID: <20150128104516.GA3553@hawk.usersys.redhat.com> References: <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-Disposition: inline In-Reply-To: <20150128075626.GA16660@redhat.com> 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: Igor Mammedov , marcel.a@redhat.com, claudio.fontana@huawei.com, qemu-devel@nongnu.org, pbonzini@redhat.com 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