From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:56508) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YEaKE-0006nk-TX for qemu-devel@nongnu.org; Fri, 23 Jan 2015 04:14:47 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YEaKB-0006Mx-LK for qemu-devel@nongnu.org; Fri, 23 Jan 2015 04:14:46 -0500 Received: from mx1.redhat.com ([209.132.183.28]:33878) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YEaKB-0006Mt-E2 for qemu-devel@nongnu.org; Fri, 23 Jan 2015 04:14:43 -0500 Date: Fri, 23 Jan 2015 11:14:33 +0200 From: "Michael S. Tsirkin" Message-ID: <20150123091433.GG26711@redhat.com> References: <1421938231-25698-1-git-send-email-imammedo@redhat.com> <1421938231-25698-2-git-send-email-imammedo@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1421938231-25698-2-git-send-email-imammedo@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: pbonzini@redhat.com, drjones@redhat.com, claudio.fontana@huawei.com, qemu-devel@nongnu.org, marcel.a@redhat.com On Thu, Jan 22, 2015 at 02:49:45PM +0000, Igor Mammedov wrote: > Adds for dynamic AML creation, which will be used > for piecing ASL/AML primitives together and hiding > from user/caller details about how nested context > should be closed/packed leaving less space for > mistakes and necessity to know how AML should be > encoded, allowing user to concentrate on ASL > representation instead. > > For example it will allow to create AML like this: > > AcpiAml scope = acpi_scope("PCI0") > AcpiAml dev = acpi_device("PM") > aml_append(&dev, acpi_name_decl("_ADR", acpi_int(addr))) > aml_append(&scope, dev); > > Signed-off-by: Igor Mammedov > --- > hw/acpi/acpi-build-utils.c | 39 ++++++++++++++++++++++++++++++++++++++ > include/hw/acpi/acpi-build-utils.h | 16 ++++++++++++++++ > 2 files changed, 55 insertions(+) > > diff --git a/hw/acpi/acpi-build-utils.c b/hw/acpi/acpi-build-utils.c > index 602e68c..547ecaa 100644 > --- a/hw/acpi/acpi-build-utils.c > +++ b/hw/acpi/acpi-build-utils.c > @@ -267,3 +267,42 @@ void build_append_int(GArray *table, uint32_t value) > build_append_value(table, value, 4); > } > } > + > +static void build_prepend_int(GArray *array, uint32_t value) > +{ > + GArray *data = build_alloc_array(); > + > + build_append_int(data, value); > + g_array_prepend_vals(array, data->data, data->len); > + build_free_array(data); > +} > + > +void aml_append(AcpiAml *parent_ctx, AcpiAml child) > +{ > + switch (child.block_flags) { > + case EXT_PACKAGE: > + build_extop_package(child.buf, child.op); > + break; > + > + case PACKAGE: > + build_package(child.buf, child.op); > + break; > + > + case RES_TEMPLATE: > + build_append_byte(child.buf, 0x79); /* EndTag */ > + /* > + * checksum operations is treated as succeeded if checksum > + * field is zero. [ACPI Spec 5.0, 6.4.2.9 End Tag] > + */ > + build_append_byte(child.buf, 0); > + /* fall through, to pack resources in buffer */ > + case BUFFER: > + build_prepend_int(child.buf, child.buf->len); > + build_package(child.buf, child.op); > + break; > + default: > + break; > + } > + build_append_array(parent_ctx->buf, child.buf); > + build_free_array(child.buf); So looking at this, the API is a bit tricky to use to avoid use after free. For example: aml_append(&a, b); aml_append(&a, b); this is use after free. This is C, memory management should not be automatic. So just move free out of there, get child by const pointer. Same for alloc: split out allocation and init. You can still return pointer back to caller, this will allow chaining just like you do here. We'll get: AcpiAml dev = aml_alloc(); aml_append(&scope, acpi_device(&dev)); aml_free(&dev); which is more verbose, but makes it clear there's no use after free, additionally, compiler checks that you don't modify child where not necessary. > +} > diff --git a/include/hw/acpi/acpi-build-utils.h b/include/hw/acpi/acpi-build-utils.h > index 199f003..64e7ec3 100644 > --- a/include/hw/acpi/acpi-build-utils.h > +++ b/include/hw/acpi/acpi-build-utils.h > @@ -5,6 +5,22 @@ > #include > #include "qemu/compiler.h" > > +typedef enum { > + NON_BLOCK, > + PACKAGE, > + EXT_PACKAGE, > + BUFFER, > + RES_TEMPLATE, > +} AcpiBlockFlags; > + > +typedef struct AcpiAml { > + GArray *buf; > + uint8_t op; > + AcpiBlockFlags block_flags; > +} AcpiAml; > + > +void aml_append(AcpiAml *parent_ctx, AcpiAml child); > + > GArray *build_alloc_array(void); > void build_free_array(GArray *array); > void build_prepend_byte(GArray *array, uint8_t val); > -- > 1.8.3.1