| Message ID | 1425757887-4819-1-git-send-email-mst@redhat.com |
|---|---|
| State | New |
| Headers | show |
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 */
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 */
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 */
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 */
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(-)