Re: [Qemu-devel] [PATCH 54/74] pc: acpi: move remaining GPE handlers into SSDT

[Igor Mammedov]

On Tue, 22 Dec 2015 11:37:40 +0200
"Michael S. Tsirkin" <address@hidden> wrote:

[...]
> > > > +        for (i = 4; i <= 0xF; i++) {
> > > > +            char *name = g_strdup_printf("_L0%X", i);
> > > > +            method = aml_method(name, 0, AML_NOTSERIALIZED);
> > > > +            aml_append(scope, method);
> > > > +            g_free(name);
> > > > +        }
> > > 
> > > How about we make aml_method accept ... format instead?
> > actually instead of making aml_method(format,...) it would be
> > easier to make it accept Aml* so we could reuse aml_name(format,...)
> > in the end it would look like:
> > 
> >  Aml gpe_name = aml_name("_L0%X", i);
> >  aml_method(gpe_name, AML_NOTSERIALIZED);
> > 
> > in addition name object could be reused in other places
> > that reference that method.
> 
> Except most methods are simple, so maybe do both APIs.
> Avoiding string duplication is a good idea,
> but using string constants works just as well.
I don't like having 2 APIs, one with 'Aml* name' and other
with 'const char *name' in C. I'd prefer to have just one
that suits the majority use cases and

I'm not so comfortable with using format string here directly
as it would look weird (at least to me):
   aml_method("_L0%X", i, argcount, AML_NOTSERIALIZED);
     - static format string check at compile time won't work here
   or
   aml_method(argcount, AML_NOTSERIALIZED, "_L0%X", i);
     - that should be fine except of that argument order doesn't
       match the way it's in spec, which I'd prefer to keep

so it leaves for me 2 options:
  1: use aml_method(aml_name("_L0%X", i), argcount, AML_NOTSERIALIZED)
      It's a bit of code churn and not sure if we really need it
      but I can do if asked for it.

  2: just leave it as is for now, because the most of method names
     are just string constants. These "_L0%X" will be gone after
     cleanup, leaving us with only handlers that use string const and
     do some work.

     I can replace g_strdup_printf() with static buffer here if you
     dislike alloc/free in this patch.