diff mbox

qom: add style guide

Message ID 1344883606-14854-1-git-send-email-aliguori@us.ibm.com
State New
Headers show

Commit Message

Anthony Liguori Aug. 13, 2012, 6:46 p.m. UTC
Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 docs/qom-style-guide.md |  489 +++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 489 insertions(+), 0 deletions(-)
 create mode 100644 docs/qom-style-guide.md

Comments

Michael S. Tsirkin Aug. 13, 2012, 7:55 p.m. UTC | #1
On Mon, Aug 13, 2012 at 01:46:46PM -0500, Anthony Liguori wrote:
> +    typedef struct MyType MyType;
> +    
> +    struct MyType
> +    {

This seems to violate our style:should be

> +    struct MyType {



> +        Object parent_obj;
> +    
> +        /*< private >*/
> +        int foo;
> +    };
> +
> +When declaring the structure, a forward declaration should be used.  This is
> +useful for consistency sake as it is required when defining classes.
> +
> +The first element must be the parent type and should be named 'parent_obj' or
> +just 'parent'.

Why should it? Why not use a descriptive name that
makes it easier to see what the object actually is?

>  When working with QOM types, you should avoid ever accessing
> +this member directly instead relying on casting macros.
> +
> +Casting macros hide the inheritence hierarchy from the implementation.  This
> +makes it easier to refactor code over time by changing the hierarchy without
> +changing the code in many places.

This seems like a weak motivation. Why do you expect to refactor
hierarchy all the time?  The cost is replacing compile time checks with
runtime ones.

So refactoring is easier to make but harder to make correct.
Sounds like a bad tradeoff.
Anthony Liguori Aug. 13, 2012, 8:57 p.m. UTC | #2
"Michael S. Tsirkin" <mst@redhat.com> writes:

> On Mon, Aug 13, 2012 at 01:46:46PM -0500, Anthony Liguori wrote:
>> +    typedef struct MyType MyType;
>> +    
>> +    struct MyType
>> +    {
>
> This seems to violate our style:should be
>
>> +    struct MyType {

That's a bug in CODING_STYLE.  Coding style only talks explicitly about
if but ought to make an exception for type declarations too.  If you
grep a bit, you'll see both styles are wildly used.

>> +        Object parent_obj;
>> +    
>> +        /*< private >*/
>> +        int foo;
>> +    };
>> +
>> +When declaring the structure, a forward declaration should be used.  This is
>> +useful for consistency sake as it is required when defining classes.
>> +
>> +The first element must be the parent type and should be named 'parent_obj' or
>> +just 'parent'.
>
> Why should it? Why not use a descriptive name that
> makes it easier to see what the object actually is?

Parent is a descriptive name.  That's all it is--the parent.  It is not
'bus' or 'bridge' or anything else you want to call it.  It's the parent
object.

If you want to interact with the object as the parent, you should cast.

>>  When working with QOM types, you should avoid ever accessing
>> +this member directly instead relying on casting macros.
>> +
>> +Casting macros hide the inheritence hierarchy from the implementation.  This
>> +makes it easier to refactor code over time by changing the hierarchy without
>> +changing the code in many places.
>
> This seems like a weak motivation. Why do you expect to refactor
> hierarchy all the time?  The cost is replacing compile time checks with
> runtime ones.

Unless you have a case where runtime checks have a measurable cost associated
with them, an appeal to performance is not valid.

It simply boils down to readability.

struct PCIDevice
{
    DeviceState qdev; // what do we call this?
};

struct E1000
{
    PCIDevice pci_dev;
};

E1000 *s = ...;

device_reset(&s->pci_dev->qdev);

Is not at all descriptive.  It's also hard to review for when people
introduce types like this.  And it's not clear why you can only have one
PCIDevice member.  Why isn't:

struct E1000
{
    PCIDevice pci_dev0;
    PCIDevice pci_dev1;
};

Not valid?  It's not obvious to a casual observer.  Using a name other
than 'parent' just allows people to have the wrong mental model.  It is
not a has-a relationship.  s->pci_dev leads a reader to think of things
in terms of a has-a relationship.  It's an is-a relationship.

> So refactoring is easier to make but harder to make correct.
> Sounds like a bad tradeoff.

99% of the work in introducing QOM was cleaning up direct references to
members by wrapping them in functions.

It's pretty darn hard to misuse cast macros.  I don't buy that they are
any less correct in practice.  Casting is done usually at the top of a
function and is unconditional.  Even the most basic testing should cover
100% of casts.

Regards,

Anthony Liguori

>
> -- 
> MST
Michael S. Tsirkin Aug. 13, 2012, 9:57 p.m. UTC | #3
On Mon, Aug 13, 2012 at 03:57:41PM -0500, Anthony Liguori wrote:
> "Michael S. Tsirkin" <mst@redhat.com> writes:
> 
> > On Mon, Aug 13, 2012 at 01:46:46PM -0500, Anthony Liguori wrote:
> >> +    typedef struct MyType MyType;
> >> +    
> >> +    struct MyType
> >> +    {
> >
> > This seems to violate our style:should be
> >
> >> +    struct MyType {
> 
> That's a bug in CODING_STYLE.  Coding style only talks explicitly about
> if but ought to make an exception for type declarations too.  If you
> grep a bit, you'll see both styles are wildly used.
> 
> >> +        Object parent_obj;
> >> +    
> >> +        /*< private >*/
> >> +        int foo;
> >> +    };
> >> +
> >> +When declaring the structure, a forward declaration should be used.  This is
> >> +useful for consistency sake as it is required when defining classes.
> >> +
> >> +The first element must be the parent type and should be named 'parent_obj' or
> >> +just 'parent'.
> >
> > Why should it? Why not use a descriptive name that
> > makes it easier to see what the object actually is?
> 
> Parent is a descriptive name.  That's all it is--the parent.  It is not
> 'bus' or 'bridge' or anything else you want to call it.  It's the parent
> object.
> 
> If you want to interact with the object as the parent, you should cast.
> 
> >>  When working with QOM types, you should avoid ever accessing
> >> +this member directly instead relying on casting macros.
> >> +
> >> +Casting macros hide the inheritence hierarchy from the implementation.  This
> >> +makes it easier to refactor code over time by changing the hierarchy without
> >> +changing the code in many places.
> >
> > This seems like a weak motivation. Why do you expect to refactor
> > hierarchy all the time?  The cost is replacing compile time checks with
> > runtime ones.
> 
> Unless you have a case where runtime checks have a measurable cost associated
> with them, an appeal to performance is not valid.
> 
> It simply boils down to readability.

Not performance and not readability.  It boils down to not introducing
bugs.  Build failures on bugs are better than runtime ones.

> struct PCIDevice
> {
>     DeviceState qdev; // what do we call this?
> };
> 
> struct E1000
> {
>     PCIDevice pci_dev;
> };
> 
> E1000 *s = ...;
> 
> device_reset(&s->pci_dev->qdev);
> 
> Is not at all descriptive.  It's also hard to review for when people
> introduce types like this.  And it's not clear why you can only have one
> PCIDevice member.  Why isn't:
> 
> struct E1000
> {
>     PCIDevice pci_dev0;
>     PCIDevice pci_dev1;
> };
> 
> Not valid?  It's not obvious to a casual observer.

It's a QOM bug that it wants zero offset, but since it does,
why doesn't QOM *check* zero offset? It should just fail build
if it isn't.

> Using a name other than 'parent' just allows people to have the wrong
> mental model.  It is not a has-a relationship.  s->pci_dev leads a
> reader to think of things in terms of a has-a relationship.  It's an
> is-a relationship.
> 
> > So refactoring is easier to make but harder to make correct.
> > Sounds like a bad tradeoff.
> 
> 99% of the work in introducing QOM was cleaning up direct references to
> members by wrapping them in functions.
> 
> It's pretty darn hard to misuse cast macros.  I don't buy that they are
> any less correct in practice.  Casting is done usually at the top of a
> function and is unconditional.  Even the most basic testing should cover
> 100% of casts.
> 
> Regards,
> 
> Anthony Liguori

Not if you don't call 100% of functions.

> >
> > -- 
> > MST
Jens Freimann Aug. 14, 2012, 7:14 a.m. UTC | #4
On Mon, Aug 13, 2012 at 01:46:46PM -0500, Anthony Liguori wrote:
> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> ---
>  docs/qom-style-guide.md |  489 +++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 489 insertions(+), 0 deletions(-)
>  create mode 100644 docs/qom-style-guide.md
> 
> diff --git a/docs/qom-style-guide.md b/docs/qom-style-guide.md
> new file mode 100644
> index 0000000..e7590e0
> --- /dev/null
> +++ b/docs/qom-style-guide.md
> @@ -0,0 +1,489 @@
> +QEMU Object Model Style Guide
> +=============================
> +
> +Overview
> +--------
> +This document is a step-by-step tutorial of QOM.  It is meant to be read from
> +top to bottom addressing the most common use-cases at the start.  This is a
> +living document and contributions are welcome but it should not attempt to be
> +an API reference.  There code contains inline documentation and API details

"There code contains" -> "The code contains"?

> +should be covered in the respective header files.
> +
> +Motivation
> +----------
> +QEMU makes extensive use of object oriented programming.  Since QEMU is written
> +in C, these OOP concepts often use very different mechanisms to achieve the
> +same goal.  The net effect is a lot of infrastructure duplication and a general
> +lack of consistency.
> +
> +The goal of QOM is to use a single infrastructure for all OOP within QEMU.  This
> +improves consistency and eases maintainability over the long term.
> +
> +QOM provides a common infrastructure for:
> +
> +- Type Management
> +  - Registering types
> +  - Enumerating registered types
> +- Inheritence
> +  - Single-parent inheritance
> +  - Introspection of inheritance hierarchy
> +  - Multiple inheritance through stateless interfaces
> +- Polymorphism
> +  - Class based polymorphism
> +  - Virtual and pure virtual methods
> +  - Constructor/destructor chaining
> +- Object Properties
> +  - Dynamic property registration (tied to Objects)
> +  - Property introspection
> +  - Access permissions
> +  - Accessor hooks
> +- Type Casting
> +  - Runtime checked upcast/downcast
> +  - Full support for casting up and down the chain (including interfaces)
> +- Object Enumeration
> +  - Expression of relationship between objects
> +  - Ability to reference objects with a symbolic path
> +  - Represented as a directed graph
> +
> +While QOM has a lot of high level concepts, the primary design goal has been to
> +keep simple concepts simple to implement.
> +
> +A Note on Consistency
> +---------------------
> +Much of the QOM types in QEMU have been converted through automated scripts.
> +Tremendous effort was made to make sure the resulting code was high quality and
> +adhered to all of the guidelines in this document.  However, there are many
> +cases where some of the conversion could not be scripted easily and some short
> +cuts where taken.
> +
> +Whenever possible, as code is refactored for other reasons, it should be brought
> +up fully to the guidelines expressed in this document.
> +
> +Creating a Simple Type
> +----------------------
> +The easiest way to understand QOM is to walk through an example.  This is a
> +typical example of creating a new type derived from Object as the parent class.
> +In this example, all code would live in a single C source file.
> +
> +Let's get started:
> +
> +    #include "qemu/object.h"
> +
> +This is the header file that contains the core QOM infrastructure.  It has
> +minimum dependencies to facilitate unit testing.
> +    
> +    #define TYPE_MY_TYPE "my-type"
> +    #define MY_TYPE(obj) OBJECT_CHECK(MyType, (obj), TYPE_MY_TYPE)
> +
> +All QOM types should define at least two macros.  The first macro is a symbolic
> +version of the type name.  It should always take the form
> +TYPE_ + upper(typename).  Type names should generally follow the naming rules of
> +QAPI which means dashes, '-', are preferred to underscores, '_'.
> +
> +The second macro is a cast macro.  The first argument is the type struct and the
> +remaining arguments are self-evident.  This form should always be followed even
> +if the cast macro isn't currently used by the C file.
> +    
> +    typedef struct MyType MyType;
> +    
> +    struct MyType
> +    {
> +        Object parent_obj;
> +    
> +        /*< private >*/
> +        int foo;
> +    };
> +
> +When declaring the structure, a forward declaration should be used.  This is
> +useful for consistency sake as it is required when defining classes.
> +
> +The first element must be the parent type and should be named 'parent_obj' or
> +just 'parent'.  When working with QOM types, you should avoid ever accessing
> +this member directly instead relying on casting macros.
> +
> +Casting macros hide the inheritence hierarchy from the implementation.  This
> +makes it easier to refactor code over time by changing the hierarchy without
> +changing the code in many places.
> +
> +    static TypeInfo my_type_info = {
> +        .name = TYPE_MY_TYPE,
> +        .parent = TYPE_OBJECT,
> +        .instance_size = sizeof(MyType),
> +    };
> +    
> +    static void register_types(void)
> +    {
> +        type_register_static(&my_type_info);
> +    }
> +    
> +    type_init(register_types);
> +
> +All QOM types must be registered with the QOM infrastructure.  Once registered,
> +the user has the ability to enumerate types, create objects, and interact with
> +objects without any additional code.
> +
> +All types must set the 'name' and 'parent' parameters.  The type macros should
> +always be used for these parameters.  Almost all types should set the
> +'instance_size' parameter although if it's not specified, it will be inherited
> +from its parent.
> +
> +Finally, a module init function should be provided.  The naming convention
> +shown here should be used in all new code.
> +
> +In general, one C file should register one type.  There are many valid
> +exceptions to this rule but whenever possible, types should be split out into
> +separate C files.
> +
> +Creating a Type with Methods
> +----------------------------
> +The next most common interaction with QOM will be to create a type that will be
> +inherited from another type.  This usually involves adding a class and
> +implementing a virtual method that can be overridden subclasses.  The

"overriden _by_ subclasses"?

> +following diff shows the changes we would need to extend our previous example to
> +allow inheritance with polymorphism.
> +
> +    @@ -1,10 +1,25 @@
> +    +#ifndef QEMU_MY_TYPE_H
> +    +#define QEMU_MY_TYPE_H
> +    +
> +     #include "qemu/object.h"
> +
> +This example assumes that the initial declarations will be split into a
> +separate header.  To simplify the example, guards are used to show where the
> +header file starts and ends.
> +
> +     #define TYPE_MY_TYPE "my-type"
> +     #define MY_TYPE(obj) \
> +         OBJECT_CHECK(MyType, (obj), TYPE_MY_TYPE)
> +    +#define MY_TYPE_CLASS(klass) \
> +    +    OBJECT_CLASS_CHECK(MyTypeClass, (klass), TYPE_MY_TYPE)
> +    +#define MY_TYPE_GET_CLASS(obj) \
> +    +    OBJECT_GET_CLASS(MyTypeClass, (obj), TYPE_MY_TYPE)
> +
> +When adding a class, we need to add two more macros to the type definition.  The
> +first macro is a class casting macro.  This looks very similar to an object cast
> +macro but instead takes a class as an argument.
> +
> +The second macro that we add allows a user to get a class pointer from an
> +object.  Method dispatch requires this last macro.
> +
> +     typedef struct MyType MyType;
> +    +typedef struct MyTypeClass MyTypeClass;
> +    +
> +    +struct MyTypeClass
> +    +{
> +    +    ObjectClass parent_klass;
> +    +
> +    +    void (*bar)(MyType *obj, int foo);
> +    +};
> +
> +A class looks very similar to an object in that it is expressed as a C structure
> +and the first member must be the class of the parent type.
> +
> +Typically classes will only contain function pointers but it is possible to have
> +data members of a class.  The first argument to each function pointer should
> +always be the object type.
> +     
> +     struct MyType
> +     {
> +    @@ -14,10 +29,35 @@ struct MyType
> +         int foo;
> +     };
> +     
> +    +void my_type_bar(MyType *obj, int foo);
> +    +
> +    +#endif
> +
> +A helper function should be provided for doing method dispatch.  This improves
> +readability and convenience.
> +
> +    +
> +    +static void my_type_default_bar(MyType *obj, int foo)
> +    +{
> +    +    /* do nothing */
> +    +}
> +    +
> +    +void my_type_bar(MyType *obj, int foo)
> +    +{
> +    +    MyTypeClass *mc = MY_TYPE_GET_CLASS(obj);
> +    +
> +    +    mc->bar(obj, foo);
> +    +}
> +    +
> +    +static void my_type_class_init(ObjectClass *klass, void *data)
> +    +{
> +    +    MyTypeClass *mc = MY_TYPE_CLASS(klass);
> +    +
> +    +    mc->bar = my_type_default_bar;
> +    +}
> +    +
> +     static TypeInfo my_type_info = {
> +         .name = TYPE_MY_TYPE,
> +         .parent = TYPE_OBJECT,
> +         .instance_size = sizeof(MyType),
> +    +    .class_size = sizeof(MyTypeClass),
> +    +    .class_init = my_type_class_init,
> +     };
> +     
> +     static void register_types(void)
> +
> +In order to add a new class for an type, we need to specific the size of the

"we need to specify"?

Jens

> +class in the TypeInfo.  We also need to provide a function to initialize the
> +class.  Classes are only ever created and initialized once for any type so this
> +function will be called once regardless of how many objects are created of this
> +type.
> +
> +The class init function should follow a naming convention of
> +typename + '_class_init'.  The class init function should cast the klass
> +parameter to the appropriate type and then overload the methods appropriately.
> +
> +In this example, we're initializing the method to a dummy function that does
> +nothing useful.  This is because 'foo' is a virtual method meaning that base
> +classes do not need to implement the function if they don't want to override
> +the behavior.
> +
> +If we did not initialize the method, the function would be a pure virtual method
> +meaning that the subclass must implement the function.  QOM cannot enforce this
> +requirement so care should be taken in the wrapper function to check for NULL.
> +
> +The wrapper function simply dispatches the method.  It should not implement any
> +logic or behavior beyond just dispatching the method.  Checking for NULL and
> +either returning an error or asserting is acceptable behavior for a wrapper
> +function.
> +
> +Implementing Devices and Overloading Methods
> +--------------------------------------------
> +Most QOM users will not implement objects that derive from TYPE_OBJECT.
> +Instead, usually QOM users will derive from TYPE_DEVICE or some other base
> +class and will also have to implement virtual methods.  
> +
> +In this example, we change MyType to inherit from TYPE_DEVICE and then implement
> +the required pure virtual method.
> +
> +    @@ -16,14 +16,14 @@ typedef struct MyTypeClass MyTypeClass;
> +     
> +     struct MyTypeClass
> +     {
> +    -    ObjectClass parent_klass;
> +    +    DeviceClass parent_klass;
> +     
> +         void (*bar)(MyType *obj, int foo);
> +     };
> +     
> +     struct MyType
> +     {
> +    -    Object parent_obj;
> +    +    DeviceState parent_obj;
> +     
> +         /*< private >*/
> +         int foo;
> +
> +Changing the parent type is trivial as it just requires modifying the
> +structures.  This is one of the benefits of doing all casts through the cast
> +macro.  It simplifies the process of refactoring.
> +
> +    @@ -45,16 +45,27 @@ void my_type_bar(MyType *obj, int foo)
> +         mc->bar(obj, foo);
> +     }
> +     
> +    +static int my_type_realize(DeviceState *dev)
> +    +{
> +    +    MyType *my = MY_TYPE(dev);
> +    +
> +    +    my->foo = 1;
> +    +
> +    +    return 0;
> +    +}
> +    +
> +     static void my_type_class_init(ObjectClass *klass, void *data)
> +     {
> +         MyTypeClass *mc = MY_TYPE_CLASS(klass);
> +    +    DeviceClass *dc = DEVICE_CLASS(klass);
> +     
> +         mc->bar = my_type_default_bar;
> +    +    dc->init = my_type_realize;
> +     }
> +
> +TYPE_DEVICE has a pure virtual method 'init' which is a bit of a misnomer.  The
> +'init' method is called after construction but before the guest is started for
> +the first time.  In QOM nomenclature, we call this realize.  At some point in
> +time, TYPE_DEVICE will be refactored to rename the init method to realize but
> +for now, we have to live with this inconsistency.
> +
> +     static TypeInfo my_type_info = {
> +         .name = TYPE_MY_TYPE,
> +    -    .parent = TYPE_OBJECT,
> +    +    .parent = TYPE_DEVICE,
> +         .instance_size = sizeof(MyType),
> +         .class_size = sizeof(MyTypeClass),
> +         .class_init = my_type_class_init,
> +
> +Using Instance Initialization
> +-----------------------------
> +
> +QDev required all initialization and destruction to happen through 'init' and
> +'exit' methods.  Since QDev didn't have a concept of constructors and
> +destructors, it was up to the type implementors to implement chaining which
> +often was done in an inconsistent fashion.
> +
> +As part of the TypeInfo structure, QOM has a instance_init and instance_finalize
> +method which acts as the constructor and destructor respectively.  These
> +functions are called starting with the subclass and working up the type
> +hierarchy by QOM.
> +
> +Any state that can be initialized independently of user supplied state should be
> +initialized as part of the constructor.
> +
> +    @@ -33,6 +33,13 @@ void my_type_bar(MyType *obj, int foo);
> +     
> +     #endif
> +     
> +    +static void my_type_initfn(Object *obj)
> +    +{
> +    +    MyType *my = MY_TYPE(obj);
> +    +
> +    +    my->foo = 1;
> +    +}
> +    +
> +     static void my_type_default_bar(MyType *obj, int foo)
> +     {
> +         /* do nothing */
> +    @@ -47,10 +54,6 @@ void my_type_bar(MyType *obj, int foo)
> +     
> +     static int my_type_realize(DeviceState *dev)
> +     {
> +    -    MyType *my = MY_TYPE(dev);
> +    -
> +    -    my->foo = 1;
> +    -
> +         return 0;
> +     }
> +     
> +    @@ -69,6 +72,7 @@ static TypeInfo my_type_info = {
> +         .instance_size = sizeof(MyType),
> +         .class_size = sizeof(MyTypeClass),
> +         .class_init = my_type_class_init,
> +    +    .instance_init = my_type_initfn,
> +     };
> +     
> +     static void register_types(void)
> +
> +Since 'foo' can be initialized without relying on user provided state, we can
> +move that logic entirely to the constructor.  Unfortunately, the DeviceState
> +init function must remain since it is pure virtual but it is now trivial.
> +
> +User Provided State (Properties)
> +--------------------------------
> +
> +A common property of most objects within QEMU is that there is a desire to
> +allow users to adjust parameters of the object either during initial creation or
> +at run time.  Properties provide a common framework for doing this.
> +
> +Properties are rich and complex and will not be covered exhaustively here.
> +Refer to the documentation in the qemu/object.h header file for exhaustive
> +documentation.
> +
> +Most interactions with properties will happen through convenience functions
> +that make adding properties easier for typical users.  In the case of our
> +example, we'll add properties using the qdev static property interface.
> +
> +    @@ -27,6 +27,7 @@ struct MyType
> +     
> +         /*< private >*/
> +         int foo;
> +    +    int max_vectors;
> +     };
> +
> +With static properties, a specific property corresponds to a member of the
> +object structure.  The infrastructure in qdev may change this member's value
> +automatically at any time before calling DeviceState::init().  That means any
> +initialization that depends on members exposed as properties must be done in
> +the DeviceState::init method.
> +     
> +     void my_type_bar(MyType *obj, int foo);
> +    @@ -54,9 +55,20 @@ void my_type_bar(MyType *obj, int foo)
> +     
> +     static int my_type_realize(DeviceState *dev)
> +     {
> +    +    MyType *mt = MY_TYPE(dev);
> +    +
> +    +    if (mt->max_vectors > 100) {
> +    +        return -EINVAL;
> +    +    }
> +    +
> +         return 0;
> +     }
> +
> +For this example, we simply validate the property contains a sane value and fail
> +realize.
> +     
> +    +static Property my_type_properties[] = {
> +    +    DEFINE_PROP_INT("max-vectors", MyType, max_vectors, 0),
> +    +    DEFINE_PROP_END_OF_LIST(),
> +    +};
> +    +
> +     static void my_type_class_init(ObjectClass *klass, void *data)
> +     {
> +         MyTypeClass *mc = MY_TYPE_CLASS(klass);
> +    @@ -64,6 +76,7 @@ static void my_type_class_init(ObjectClass *klass,
> +     
> +         mc->bar = my_type_default_bar;
> +         dc->init = my_type_realize;
> +    +    dc->props = my_type_properties;
> +     }
> +     
> +     static TypeInfo my_type_info = {
> +
> +Static properties are registered using a static class variable in the
> +TYPE_DEVICE class.  Base classes can add static properties using this approach
> +and subclasses will automatically inherit them.
> +
> +Child and Link Properties
> +-------------------------
> +
> +The other common types of properties in QOM are child and link properties.  Like
> +static properties, there are special helpers to add these properties to an
> +object.
> +
> +    @@ -25,6 +25,9 @@ struct MyType
> +     {
> +         DeviceState parent_obj;
> +     
> +    +    Pin *in;
> +    +    Pin out;
> +    +
> +         /*< private >*/
> +         int foo;
> +         int max_vectors;
> +
> +To begin with, we have to add struct members in that object that will hold the
> +properties.  A link is a pointer to another object and is represented as a
> +pointer in C.  A child property is an embedded object and is represented by
> +embedding a struct member in the object structure.
> +
> +A child property has its lifecycle tied to the parent object.  IOW, when an
> +object of MyType is destroyed, the 'out' object embedded within it will be
> +automatically destroyed.
> +
> +A link property will hold a reference to the object it points to, but does not
> +control the life cycle of the object it points to.  That is, when an object of
> +MyType is destroyed, the object pointed to by 'in' does not necessarily get
> +destroyed although its reference count will be decreased.
> +
> +    @@ -39,6 +42,11 @@ static void my_type_initfn(Object *obj)
> +         MyType *my = MY_TYPE(obj);
> +     
> +         my->foo = 1;
> +    +
> +    +    object_initialize(&my->out, TYPE_PIN);
> +    +    object_property_add_child(obj, "out", OBJECT(&my->out), NULL);
> +    +
> +    +    object_property_add_link(obj, "in", TYPE_PIN,
> +    +                             (Object **)&my->in, NULL);
> +     }
> +     
> +     static void my_type_default_bar(MyType *obj, int foo)
> +
> +To add the properties to our object, we need to first initialize our child
> +object and then add the properties.  This should always be done in the
> +constructor whenever possible.
> -- 
> 1.7.5.4
> 
>
diff mbox

Patch

diff --git a/docs/qom-style-guide.md b/docs/qom-style-guide.md
new file mode 100644
index 0000000..e7590e0
--- /dev/null
+++ b/docs/qom-style-guide.md
@@ -0,0 +1,489 @@ 
+QEMU Object Model Style Guide
+=============================
+
+Overview
+--------
+This document is a step-by-step tutorial of QOM.  It is meant to be read from
+top to bottom addressing the most common use-cases at the start.  This is a
+living document and contributions are welcome but it should not attempt to be
+an API reference.  There code contains inline documentation and API details
+should be covered in the respective header files.
+
+Motivation
+----------
+QEMU makes extensive use of object oriented programming.  Since QEMU is written
+in C, these OOP concepts often use very different mechanisms to achieve the
+same goal.  The net effect is a lot of infrastructure duplication and a general
+lack of consistency.
+
+The goal of QOM is to use a single infrastructure for all OOP within QEMU.  This
+improves consistency and eases maintainability over the long term.
+
+QOM provides a common infrastructure for:
+
+- Type Management
+  - Registering types
+  - Enumerating registered types
+- Inheritence
+  - Single-parent inheritance
+  - Introspection of inheritance hierarchy
+  - Multiple inheritance through stateless interfaces
+- Polymorphism
+  - Class based polymorphism
+  - Virtual and pure virtual methods
+  - Constructor/destructor chaining
+- Object Properties
+  - Dynamic property registration (tied to Objects)
+  - Property introspection
+  - Access permissions
+  - Accessor hooks
+- Type Casting
+  - Runtime checked upcast/downcast
+  - Full support for casting up and down the chain (including interfaces)
+- Object Enumeration
+  - Expression of relationship between objects
+  - Ability to reference objects with a symbolic path
+  - Represented as a directed graph
+
+While QOM has a lot of high level concepts, the primary design goal has been to
+keep simple concepts simple to implement.
+
+A Note on Consistency
+---------------------
+Much of the QOM types in QEMU have been converted through automated scripts.
+Tremendous effort was made to make sure the resulting code was high quality and
+adhered to all of the guidelines in this document.  However, there are many
+cases where some of the conversion could not be scripted easily and some short
+cuts where taken.
+
+Whenever possible, as code is refactored for other reasons, it should be brought
+up fully to the guidelines expressed in this document.
+
+Creating a Simple Type
+----------------------
+The easiest way to understand QOM is to walk through an example.  This is a
+typical example of creating a new type derived from Object as the parent class.
+In this example, all code would live in a single C source file.
+
+Let's get started:
+
+    #include "qemu/object.h"
+
+This is the header file that contains the core QOM infrastructure.  It has
+minimum dependencies to facilitate unit testing.
+    
+    #define TYPE_MY_TYPE "my-type"
+    #define MY_TYPE(obj) OBJECT_CHECK(MyType, (obj), TYPE_MY_TYPE)
+
+All QOM types should define at least two macros.  The first macro is a symbolic
+version of the type name.  It should always take the form
+TYPE_ + upper(typename).  Type names should generally follow the naming rules of
+QAPI which means dashes, '-', are preferred to underscores, '_'.
+
+The second macro is a cast macro.  The first argument is the type struct and the
+remaining arguments are self-evident.  This form should always be followed even
+if the cast macro isn't currently used by the C file.
+    
+    typedef struct MyType MyType;
+    
+    struct MyType
+    {
+        Object parent_obj;
+    
+        /*< private >*/
+        int foo;
+    };
+
+When declaring the structure, a forward declaration should be used.  This is
+useful for consistency sake as it is required when defining classes.
+
+The first element must be the parent type and should be named 'parent_obj' or
+just 'parent'.  When working with QOM types, you should avoid ever accessing
+this member directly instead relying on casting macros.
+
+Casting macros hide the inheritence hierarchy from the implementation.  This
+makes it easier to refactor code over time by changing the hierarchy without
+changing the code in many places.
+
+    static TypeInfo my_type_info = {
+        .name = TYPE_MY_TYPE,
+        .parent = TYPE_OBJECT,
+        .instance_size = sizeof(MyType),
+    };
+    
+    static void register_types(void)
+    {
+        type_register_static(&my_type_info);
+    }
+    
+    type_init(register_types);
+
+All QOM types must be registered with the QOM infrastructure.  Once registered,
+the user has the ability to enumerate types, create objects, and interact with
+objects without any additional code.
+
+All types must set the 'name' and 'parent' parameters.  The type macros should
+always be used for these parameters.  Almost all types should set the
+'instance_size' parameter although if it's not specified, it will be inherited
+from its parent.
+
+Finally, a module init function should be provided.  The naming convention
+shown here should be used in all new code.
+
+In general, one C file should register one type.  There are many valid
+exceptions to this rule but whenever possible, types should be split out into
+separate C files.
+
+Creating a Type with Methods
+----------------------------
+The next most common interaction with QOM will be to create a type that will be
+inherited from another type.  This usually involves adding a class and
+implementing a virtual method that can be overridden subclasses.  The
+following diff shows the changes we would need to extend our previous example to
+allow inheritance with polymorphism.
+
+    @@ -1,10 +1,25 @@
+    +#ifndef QEMU_MY_TYPE_H
+    +#define QEMU_MY_TYPE_H
+    +
+     #include "qemu/object.h"
+
+This example assumes that the initial declarations will be split into a
+separate header.  To simplify the example, guards are used to show where the
+header file starts and ends.
+
+     #define TYPE_MY_TYPE "my-type"
+     #define MY_TYPE(obj) \
+         OBJECT_CHECK(MyType, (obj), TYPE_MY_TYPE)
+    +#define MY_TYPE_CLASS(klass) \
+    +    OBJECT_CLASS_CHECK(MyTypeClass, (klass), TYPE_MY_TYPE)
+    +#define MY_TYPE_GET_CLASS(obj) \
+    +    OBJECT_GET_CLASS(MyTypeClass, (obj), TYPE_MY_TYPE)
+
+When adding a class, we need to add two more macros to the type definition.  The
+first macro is a class casting macro.  This looks very similar to an object cast
+macro but instead takes a class as an argument.
+
+The second macro that we add allows a user to get a class pointer from an
+object.  Method dispatch requires this last macro.
+
+     typedef struct MyType MyType;
+    +typedef struct MyTypeClass MyTypeClass;
+    +
+    +struct MyTypeClass
+    +{
+    +    ObjectClass parent_klass;
+    +
+    +    void (*bar)(MyType *obj, int foo);
+    +};
+
+A class looks very similar to an object in that it is expressed as a C structure
+and the first member must be the class of the parent type.
+
+Typically classes will only contain function pointers but it is possible to have
+data members of a class.  The first argument to each function pointer should
+always be the object type.
+     
+     struct MyType
+     {
+    @@ -14,10 +29,35 @@ struct MyType
+         int foo;
+     };
+     
+    +void my_type_bar(MyType *obj, int foo);
+    +
+    +#endif
+
+A helper function should be provided for doing method dispatch.  This improves
+readability and convenience.
+
+    +
+    +static void my_type_default_bar(MyType *obj, int foo)
+    +{
+    +    /* do nothing */
+    +}
+    +
+    +void my_type_bar(MyType *obj, int foo)
+    +{
+    +    MyTypeClass *mc = MY_TYPE_GET_CLASS(obj);
+    +
+    +    mc->bar(obj, foo);
+    +}
+    +
+    +static void my_type_class_init(ObjectClass *klass, void *data)
+    +{
+    +    MyTypeClass *mc = MY_TYPE_CLASS(klass);
+    +
+    +    mc->bar = my_type_default_bar;
+    +}
+    +
+     static TypeInfo my_type_info = {
+         .name = TYPE_MY_TYPE,
+         .parent = TYPE_OBJECT,
+         .instance_size = sizeof(MyType),
+    +    .class_size = sizeof(MyTypeClass),
+    +    .class_init = my_type_class_init,
+     };
+     
+     static void register_types(void)
+
+In order to add a new class for an type, we need to specific the size of the
+class in the TypeInfo.  We also need to provide a function to initialize the
+class.  Classes are only ever created and initialized once for any type so this
+function will be called once regardless of how many objects are created of this
+type.
+
+The class init function should follow a naming convention of
+typename + '_class_init'.  The class init function should cast the klass
+parameter to the appropriate type and then overload the methods appropriately.
+
+In this example, we're initializing the method to a dummy function that does
+nothing useful.  This is because 'foo' is a virtual method meaning that base
+classes do not need to implement the function if they don't want to override
+the behavior.
+
+If we did not initialize the method, the function would be a pure virtual method
+meaning that the subclass must implement the function.  QOM cannot enforce this
+requirement so care should be taken in the wrapper function to check for NULL.
+
+The wrapper function simply dispatches the method.  It should not implement any
+logic or behavior beyond just dispatching the method.  Checking for NULL and
+either returning an error or asserting is acceptable behavior for a wrapper
+function.
+
+Implementing Devices and Overloading Methods
+--------------------------------------------
+Most QOM users will not implement objects that derive from TYPE_OBJECT.
+Instead, usually QOM users will derive from TYPE_DEVICE or some other base
+class and will also have to implement virtual methods.  
+
+In this example, we change MyType to inherit from TYPE_DEVICE and then implement
+the required pure virtual method.
+
+    @@ -16,14 +16,14 @@ typedef struct MyTypeClass MyTypeClass;
+     
+     struct MyTypeClass
+     {
+    -    ObjectClass parent_klass;
+    +    DeviceClass parent_klass;
+     
+         void (*bar)(MyType *obj, int foo);
+     };
+     
+     struct MyType
+     {
+    -    Object parent_obj;
+    +    DeviceState parent_obj;
+     
+         /*< private >*/
+         int foo;
+
+Changing the parent type is trivial as it just requires modifying the
+structures.  This is one of the benefits of doing all casts through the cast
+macro.  It simplifies the process of refactoring.
+
+    @@ -45,16 +45,27 @@ void my_type_bar(MyType *obj, int foo)
+         mc->bar(obj, foo);
+     }
+     
+    +static int my_type_realize(DeviceState *dev)
+    +{
+    +    MyType *my = MY_TYPE(dev);
+    +
+    +    my->foo = 1;
+    +
+    +    return 0;
+    +}
+    +
+     static void my_type_class_init(ObjectClass *klass, void *data)
+     {
+         MyTypeClass *mc = MY_TYPE_CLASS(klass);
+    +    DeviceClass *dc = DEVICE_CLASS(klass);
+     
+         mc->bar = my_type_default_bar;
+    +    dc->init = my_type_realize;
+     }
+
+TYPE_DEVICE has a pure virtual method 'init' which is a bit of a misnomer.  The
+'init' method is called after construction but before the guest is started for
+the first time.  In QOM nomenclature, we call this realize.  At some point in
+time, TYPE_DEVICE will be refactored to rename the init method to realize but
+for now, we have to live with this inconsistency.
+
+     static TypeInfo my_type_info = {
+         .name = TYPE_MY_TYPE,
+    -    .parent = TYPE_OBJECT,
+    +    .parent = TYPE_DEVICE,
+         .instance_size = sizeof(MyType),
+         .class_size = sizeof(MyTypeClass),
+         .class_init = my_type_class_init,
+
+Using Instance Initialization
+-----------------------------
+
+QDev required all initialization and destruction to happen through 'init' and
+'exit' methods.  Since QDev didn't have a concept of constructors and
+destructors, it was up to the type implementors to implement chaining which
+often was done in an inconsistent fashion.
+
+As part of the TypeInfo structure, QOM has a instance_init and instance_finalize
+method which acts as the constructor and destructor respectively.  These
+functions are called starting with the subclass and working up the type
+hierarchy by QOM.
+
+Any state that can be initialized independently of user supplied state should be
+initialized as part of the constructor.
+
+    @@ -33,6 +33,13 @@ void my_type_bar(MyType *obj, int foo);
+     
+     #endif
+     
+    +static void my_type_initfn(Object *obj)
+    +{
+    +    MyType *my = MY_TYPE(obj);
+    +
+    +    my->foo = 1;
+    +}
+    +
+     static void my_type_default_bar(MyType *obj, int foo)
+     {
+         /* do nothing */
+    @@ -47,10 +54,6 @@ void my_type_bar(MyType *obj, int foo)
+     
+     static int my_type_realize(DeviceState *dev)
+     {
+    -    MyType *my = MY_TYPE(dev);
+    -
+    -    my->foo = 1;
+    -
+         return 0;
+     }
+     
+    @@ -69,6 +72,7 @@ static TypeInfo my_type_info = {
+         .instance_size = sizeof(MyType),
+         .class_size = sizeof(MyTypeClass),
+         .class_init = my_type_class_init,
+    +    .instance_init = my_type_initfn,
+     };
+     
+     static void register_types(void)
+
+Since 'foo' can be initialized without relying on user provided state, we can
+move that logic entirely to the constructor.  Unfortunately, the DeviceState
+init function must remain since it is pure virtual but it is now trivial.
+
+User Provided State (Properties)
+--------------------------------
+
+A common property of most objects within QEMU is that there is a desire to
+allow users to adjust parameters of the object either during initial creation or
+at run time.  Properties provide a common framework for doing this.
+
+Properties are rich and complex and will not be covered exhaustively here.
+Refer to the documentation in the qemu/object.h header file for exhaustive
+documentation.
+
+Most interactions with properties will happen through convenience functions
+that make adding properties easier for typical users.  In the case of our
+example, we'll add properties using the qdev static property interface.
+
+    @@ -27,6 +27,7 @@ struct MyType
+     
+         /*< private >*/
+         int foo;
+    +    int max_vectors;
+     };
+
+With static properties, a specific property corresponds to a member of the
+object structure.  The infrastructure in qdev may change this member's value
+automatically at any time before calling DeviceState::init().  That means any
+initialization that depends on members exposed as properties must be done in
+the DeviceState::init method.
+     
+     void my_type_bar(MyType *obj, int foo);
+    @@ -54,9 +55,20 @@ void my_type_bar(MyType *obj, int foo)
+     
+     static int my_type_realize(DeviceState *dev)
+     {
+    +    MyType *mt = MY_TYPE(dev);
+    +
+    +    if (mt->max_vectors > 100) {
+    +        return -EINVAL;
+    +    }
+    +
+         return 0;
+     }
+
+For this example, we simply validate the property contains a sane value and fail
+realize.
+     
+    +static Property my_type_properties[] = {
+    +    DEFINE_PROP_INT("max-vectors", MyType, max_vectors, 0),
+    +    DEFINE_PROP_END_OF_LIST(),
+    +};
+    +
+     static void my_type_class_init(ObjectClass *klass, void *data)
+     {
+         MyTypeClass *mc = MY_TYPE_CLASS(klass);
+    @@ -64,6 +76,7 @@ static void my_type_class_init(ObjectClass *klass,
+     
+         mc->bar = my_type_default_bar;
+         dc->init = my_type_realize;
+    +    dc->props = my_type_properties;
+     }
+     
+     static TypeInfo my_type_info = {
+
+Static properties are registered using a static class variable in the
+TYPE_DEVICE class.  Base classes can add static properties using this approach
+and subclasses will automatically inherit them.
+
+Child and Link Properties
+-------------------------
+
+The other common types of properties in QOM are child and link properties.  Like
+static properties, there are special helpers to add these properties to an
+object.
+
+    @@ -25,6 +25,9 @@ struct MyType
+     {
+         DeviceState parent_obj;
+     
+    +    Pin *in;
+    +    Pin out;
+    +
+         /*< private >*/
+         int foo;
+         int max_vectors;
+
+To begin with, we have to add struct members in that object that will hold the
+properties.  A link is a pointer to another object and is represented as a
+pointer in C.  A child property is an embedded object and is represented by
+embedding a struct member in the object structure.
+
+A child property has its lifecycle tied to the parent object.  IOW, when an
+object of MyType is destroyed, the 'out' object embedded within it will be
+automatically destroyed.
+
+A link property will hold a reference to the object it points to, but does not
+control the life cycle of the object it points to.  That is, when an object of
+MyType is destroyed, the object pointed to by 'in' does not necessarily get
+destroyed although its reference count will be decreased.
+
+    @@ -39,6 +42,11 @@ static void my_type_initfn(Object *obj)
+         MyType *my = MY_TYPE(obj);
+     
+         my->foo = 1;
+    +
+    +    object_initialize(&my->out, TYPE_PIN);
+    +    object_property_add_child(obj, "out", OBJECT(&my->out), NULL);
+    +
+    +    object_property_add_link(obj, "in", TYPE_PIN,
+    +                             (Object **)&my->in, NULL);
+     }
+     
+     static void my_type_default_bar(MyType *obj, int foo)
+
+To add the properties to our object, we need to first initialize our child
+object and then add the properties.  This should always be done in the
+constructor whenever possible.