qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Michael S. Tsirkin
Subject: Re: [Qemu-devel] [PATCH 54/74] pc: acpi: move remaining GPE handlers into SSDT
Date: Tue, 22 Dec 2015 16:47:18 +0200

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" <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

Just add ... at the end:

aml_method("_L0%X", argcount, AML_NOTSERIALIZED, i);

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.

As you say, will be gone after refactoring.

-- 
MST



reply via email to

[Prev in Thread] Current Thread [Next in Thread]