On Mon, 26 Jan 2015 10:57:21 +0100
Igor Mammedov <address@hidden> wrote:
On Sat, 24 Jan 2015 18:33:50 +0200
"Michael S. Tsirkin" <address@hidden> wrote:
On Fri, Jan 23, 2015 at 06:56:20PM +0100, Igor Mammedov wrote:
On Fri, 23 Jan 2015 15:55:11 +0200
"Michael S. Tsirkin" <address@hidden> wrote:
[...]
I refuse to give up on cleaner and simpler API yet :)
Your patches are almost there, they are pretty clean, the
only issue I think is this passing of AcpiAml by value,
sometimes freeing buffer in the process, sometimes not.
Currently buffer is allocated by API and is always freed
whenever it's passed to another API function.
That's why it makes user not to care about memory mgmt.
The only limitation of it is if you store AcpiAml return
value into some variable you are responsible to use it only
once for passing to another API function. Reusing this
variable's value (pass it to API function second time)
would cause cause use-after-free and freeing-freed bugs.
Like this: AcpiAml table =
acpi_definition_block("SSDT",...); AcpiAml scope =
acpi_scope("PCI0"); aml_append(&table, scope); // <- here
scope becomes invalid // a bug
aml_append(&table, scope); // use-after-free +
freeing-freed bugs
There are several approaches to look for resolving above
issues: 1. Adopt and use memory mgmt model used by GTK+
in nutshell:
http://www.cs.hunter.cuny.edu/~sweiss/course_materials/csci493.70/lecture_notes/GTK_memory_mngmt.pdf
In particular adopt behavior of GInitiallyUnowned usage
model
that will allow to keep convenient chained call style
and if necessary reuse objects returned by API by
explicitly referencing/dereferencing them if needed.
Hmm, it's still easy to misuse. I think I prefer option 2
below.
That's basically what we have/use in QOM with object_new(FOO) +
object_unref() I have no idea why we invented our own Object
infrastructure when we could just use GObject one from already
used glib.
2. It's possible to drop freeing inside API completely and
record(store in list) every new object inside a table
context. When table is constructed, list of created objects
could be safely freed.
With that it would be safe to reuse every AcpiAml object
and avoid free-after-use issues with limitation that
created AcpiAml objects shouldn't be used after table was
closed. It should cover all practical use of API, i.e. no
cross table AcpiAml objects.
So each aml_alloc function gets pointer to this list,
and adds the new element there.
Eventually we do free_all to free all elements,
so there isn't even an aml_free to mis-use.
I'm thinking a little bit different about implementation though.
I still don't like the use of explicit alloc/free being called
by API user since it doesn't allow chained API calls and
I think it's unnecessary complication see below why.
Here is what's true about current API and a I'd like to with it:
1. Every API call (except aml_append) makes aml_alloc(), it's
just like a wrapper about object_new(FOO). (current + new impl.)
2 Every API call that takes AML type as input argument
2.1 consumes (frees) it (current impl.)
(it's easy to fix use after free concern too,
just pass AML by pointer and zero-out memory before it's
freed and assert whenever one of input arguments is not correct,
i.e. it was reused second time)
There is no need for following steps after this one.
2.2 takes ownership of GInitiallyUnowned and adds it to its
list of its children.
3. Free children when AML object is destroyed (i.e. ref count
zero) That way when toplevel table object (definition block in
42/47) is added to ACPI blob we can unref it, which will cause
its whole children tree freed, except for AML objects where
API user explicitly took extra reference (i.e. wanted them
to reuse in another table)
I'd prefer:
* 2.1 way to address your current concern of use-after-free
as the most simplest one (no reuse is possible however)
or
* follow already used by QEMU QOM/GObject pattern of
implicit alloc/free
since they allow to construct AML in a more simple/manageable
way i.e.
aml_append(method,
aml_store(aml_string("foo"), aml_local(0)))
);
v.s. explicit headache of alloc/free, which doesn't fix
use-after-free anyway and just adds more boiler plate
plus makes code har to read read
str = aml_alloc();
aml_string(str, "foo");
loc0 = aml_alloc();
aml_local(loc0, 0);
store = aml_alloc();
aml_store(store, str, loc0);
aml_append(method, store);
aml_free(store);
aml_free(loc0);
aml_free(str);
Here is a compromise what I and Michael came to on a phone call:
Externally API usage would look like:
AmlAllocList *p = some_list_alloc();
Aml *ssdt = aml_def_block(p, "SSDT", ...);
Aml *dev = aml_device(p, "PCI0");
aml_append(dev,
aml_name_def(p, "_STA", aml_int(p, 0xF /* present */))
);
aml_append(ssdt, dev);
aml_append(acpi_tables_blob, ssdt);
free_aml_alloc_list(p);
Each of aml_foo() will take other Aml arguments by pointer.
Also every aml_foo(), except of aml_append() will allocate
Aml struct and return pointer to it and also add this pointer
into AmlAllocList which is passed as first argument to each
aml_foo() call.
aml_append() becomes nondestructive function and just adds
child(2nd arg) to the parent context (1st arg).
After API user is done with building table and pushed it
into tables blob, he/she calls free_aml_alloc_list() to free
all Aml objects created during process of building the table
content.