Re: [Qemu-devel] [PATCH] aml-build: don't modify child

[Igor Mammedov]

On Mon, 9 Mar 2015 10:30:31 +0100
"Michael S. Tsirkin" <address@hidden> 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" <address@hidden> 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 <address@hidden>
Reviewed-by: Igor Mammedov <address@hidden>

> > > ---
> > >  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 */