From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:59904) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YGSR5-0003YW-Qz for qemu-devel@nongnu.org; Wed, 28 Jan 2015 08:13:36 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YGSR1-0007zJ-Lj for qemu-devel@nongnu.org; Wed, 28 Jan 2015 08:13:35 -0500 Received: from mx1.redhat.com ([209.132.183.28]:42534) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YGSR1-0007z3-EH for qemu-devel@nongnu.org; Wed, 28 Jan 2015 08:13:31 -0500 Date: Wed, 28 Jan 2015 15:12:53 +0200 From: "Michael S. Tsirkin" Message-ID: <20150128131253.GA31448@redhat.com> References: <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> <20150128110023.160e5d24@nial.brq.redhat.com> <20150128102423.GA30541@redhat.com> <20150128115014.1c8f580e@nial.brq.redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20150128115014.1c8f580e@nial.brq.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: Igor Mammedov Cc: Andrew Jones , marcel.a@redhat.com, claudio.fontana@huawei.com, qemu-devel@nongnu.org, Shannon Zhao , pbonzini@redhat.com 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" 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" 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.