From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:36706) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YUu1G-0004GJ-CJ for qemu-devel@nongnu.org; Mon, 09 Mar 2015 05:30:39 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YUu1D-00051r-0N for qemu-devel@nongnu.org; Mon, 09 Mar 2015 05:30:38 -0400 Received: from mx1.redhat.com ([209.132.183.28]:41426) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YUu1C-00051i-Nr for qemu-devel@nongnu.org; Mon, 09 Mar 2015 05:30:34 -0400 Received: from int-mx14.intmail.prod.int.phx2.redhat.com (int-mx14.intmail.prod.int.phx2.redhat.com [10.5.11.27]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id t299UYfK032087 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL) for ; Mon, 9 Mar 2015 05:30:34 -0400 Date: Mon, 9 Mar 2015 10:30:31 +0100 From: "Michael S. Tsirkin" Message-ID: <20150309093031.GA23226@redhat.com> References: <1425757887-4819-1-git-send-email-mst@redhat.com> <20150309102715.706fbd6f@nial.brq.redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20150309102715.706fbd6f@nial.brq.redhat.com> Subject: Re: [Qemu-devel] [PATCH] aml-build: don't modify child List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Igor Mammedov Cc: marcel@redhat.com, qemu-devel@nongnu.org On Mon, Mar 09, 2015 at 10:27:15AM +0100, Igor Mammedov wrote: > On Sat, 7 Mar 2015 20:51:33 +0100 > "Michael S. Tsirkin" wrote: > > > this code: > > aml_append(foo, bar); > > > > might, non-intuitively, modify bar, which means that e.g. the following > > might not DTRT: > > > > c = ....; > > aml_append(a, c); > > aml_append(b, c); > > > > to fix, simply allocate an intermediate array, > > and always modify that. > While at it, could 'c' be made 'const Aml*' argument of aml_append() ??? I considered this but it's mostly useless since the buffer can still be modified. We can think about this using a patch on top. > > > > Signed-off-by: Michael S. Tsirkin > > --- > > hw/acpi/aml-build.c | 16 ++++++++++------ > > 1 file changed, 10 insertions(+), 6 deletions(-) > > > > diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c > > index 876cada..ff12b28 100644 > > --- a/hw/acpi/aml-build.c > > +++ b/hw/acpi/aml-build.c > > @@ -335,26 +335,29 @@ static void build_buffer(GArray *array, uint8_t op) > > > > void aml_append(Aml *parent_ctx, Aml *child) > > { > > + GArray *buf = build_alloc_array(); > > + build_append_array(buf, child->buf); > > + > > switch (child->block_flags) { > > case AML_OPCODE: > > build_append_byte(parent_ctx->buf, child->op); > > break; > > case AML_EXT_PACKAGE: > > - build_extop_package(child->buf, child->op); > > + build_extop_package(buf, child->op); > > break; > > case AML_PACKAGE: > > - build_package(child->buf, child->op); > > + build_package(buf, child->op); > > break; > > case AML_RES_TEMPLATE: > > - build_append_byte(child->buf, 0x79); /* EndTag */ > > + build_append_byte(buf, 0x79); /* EndTag */ > > /* > > * checksum operations are treated as succeeded if checksum > > * field is zero. [ACPI Spec 1.0b, 6.4.2.8 End Tag] > > */ > > - build_append_byte(child->buf, 0); > > + build_append_byte(buf, 0); > > /* fall through, to pack resources in buffer */ > > case AML_BUFFER: > > - build_buffer(child->buf, child->op); > > + build_buffer(buf, child->op); > > break; > > case AML_NO_OPCODE: > > break; > > @@ -362,7 +365,8 @@ void aml_append(Aml *parent_ctx, Aml *child) > > assert(0); > > break; > > } > > - build_append_array(parent_ctx->buf, child->buf); > > + build_append_array(parent_ctx->buf, buf); > > + build_free_array(buf); > > } > > > > /* ACPI 1.0b: 16.2.5.1 Namespace Modifier Objects Encoding: DefScope */