* [Qemu-devel] [PATCH] aml-build: don't modify child
@ 2015-03-07 19:51 Michael S. Tsirkin
2015-03-09 9:27 ` Igor Mammedov
0 siblings, 1 reply; 4+ messages in thread
From: Michael S. Tsirkin @ 2015-03-07 19:51 UTC (permalink / raw)
To: qemu-devel; +Cc: marcel, imammedo
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.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
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 */
--
MST
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [PATCH] aml-build: don't modify child
2015-03-07 19:51 [Qemu-devel] [PATCH] aml-build: don't modify child Michael S. Tsirkin
@ 2015-03-09 9:27 ` Igor Mammedov
2015-03-09 9:30 ` Michael S. Tsirkin
0 siblings, 1 reply; 4+ messages in thread
From: Igor Mammedov @ 2015-03-09 9:27 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: marcel, qemu-devel
On Sat, 7 Mar 2015 20:51:33 +0100
"Michael S. Tsirkin" <mst@redhat.com> 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() ???
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
> 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 */
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [PATCH] aml-build: don't modify child
2015-03-09 9:27 ` Igor Mammedov
@ 2015-03-09 9:30 ` Michael S. Tsirkin
2015-03-09 9:36 ` Igor Mammedov
0 siblings, 1 reply; 4+ messages in thread
From: Michael S. Tsirkin @ 2015-03-09 9:30 UTC (permalink / raw)
To: Igor Mammedov; +Cc: marcel, qemu-devel
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" <mst@redhat.com> 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 <mst@redhat.com>
> > ---
> > 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 */
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [PATCH] aml-build: don't modify child
2015-03-09 9:30 ` Michael S. Tsirkin
@ 2015-03-09 9:36 ` Igor Mammedov
0 siblings, 0 replies; 4+ messages in thread
From: Igor Mammedov @ 2015-03-09 9:36 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: marcel, qemu-devel
On Mon, 9 Mar 2015 10:30:31 +0100
"Michael S. Tsirkin" <mst@redhat.com> wrote:
> 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" <mst@redhat.com> 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.
sure
>
>
> > >
> > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> > > ---
> > > 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 */
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2015-03-09 9:36 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-03-07 19:51 [Qemu-devel] [PATCH] aml-build: don't modify child Michael S. Tsirkin
2015-03-09 9:27 ` Igor Mammedov
2015-03-09 9:30 ` Michael S. Tsirkin
2015-03-09 9:36 ` Igor Mammedov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).