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

[Igor Mammedov <imammedo@redhat.com>]

On Tue, 22 Dec 2015 16:47:18 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Tue, Dec 22, 2015 at 03:38:24PM +0100, Igor Mammedov wrote:
> > On Tue, 22 Dec 2015 11:37:40 +0200
> > "Michael S. Tsirkin" <mst@redhat.com> 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
> 
> Just add ... at the end:
> 
> aml_method("_L0%X", argcount, AML_NOTSERIALIZED, i);
ok, I'll try to do it.

> 
> looks a bit weird if you have to use it, but
> it's uncommon, and the common case looks simple.
> 
> > 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.
> 
> So how about just open-coding this loop for now?
> That's just about 10 lines of code,
> not a big deal.
sure

> 
> As you say, will be gone after refactoring.
>