[Qemu-devel] [RFC] QDev explicit constructors & destructors

[Qemu-devel] [RFC] QDev explicit constructors & destructors

[Liviu Ionescu]

The .instance_init construction method available in the current QOM provides an equivalent of the C++ default constructor, that works just fine as long as there is no need to pass additional data to the construction logic.

One natural solution would be to add an explicit constructor, like in C++. 

Since C++ style multiple constructors can be folded into a single constructor using a pointer to a structure, one single callback would be enough to accommodate any construction logic.

The generic use case would look like this:

   DeviceState *mcu = qdev_alloc(NULL, TYPE_STM32F103RB);
   {
       qdev_prop_set_uint32(mcu, "param1", value1);  /* Optional */
       qdev_prop_set_uint32(mcu, "param2", value2);  /* Optional */
       qdev_construct(mcu, &constructor_data); /* the second pointer may be NULL */

       /* Set the board specific oscillator frequencies. */
       qdev_prop_set_uint32(mcu, "hse-freq-hz", 8000000); /* 8.0 MHz */
       qdev_prop_set_uint32(mcu, "lse-freq-hz", 32768); /* 32 KHz */
   }
   qdev_realize(mcu); /* QDev specific step */



The implementation requires only to: 

- add a "void (*construct)(DeviceState *dev, void *data)" member to the DeviceClass structure
- add a qdev_construct() function, that will run all these callbacks in 'parent first' order

For completeness, although it'll probably be used rarely, a destructor can also be implemented, by adding:

- a "void (*destruct)(DeviceState *dev)" member to the DeviceClass structure, and 
- a qdev_destruct(dev) function, that will run these callbacks in 'child first' order


Notes:
- except for setting statically defined properties, the object returned by qdev_alloc() but not yet constructed, **should not** be used for any other purposes
- inside the "construct" callback any number of children objects can be created, 
- an object is not considered properly constructed before all children object are constructed; in other words, the construct callback should not return as long as it includes objects allocated but not yet constructed
- passing constructor data can be done via standard properties and/or via the more generic 'data' structure, that will be passed up in the hierarchy (similar to the object state structure, each child in the hierarchy can extend this structure with additional members).
- after construction and before freezing the object with realize(), properties that were dynamically added during construction (like aliases to internal objects created during construction), can be set as usual.


Please note that these additions do not break any compatibility with existing code.

The two other functions mentioned are just aliases with more appropriate names for qdev_create() (create is not accurate, since the object is not yet ready to use) and qdev_init_nofail() (this name is deprecated, it is not calling init but realize, and the nofail is confusing, it not only fail, it even exits).


Regards,

Liviu

Re: [Qemu-devel] [RFC] QDev explicit constructors & destructors

[Liviu Ionescu]

> On 15 Jun 2015, at 13:48, Liviu Ionescu <ilg@livius.net> wrote:
> 
> ... add an explicit constructor, like in C++. 
> ... the DeviceClass structure ... qdev_construct() ...

For more generality, the new construct/destruct pointers can be added directly to the QOM ObjectClass, and the functions implemented as:

	object_construct(Object*, void*)
	object_destruct(Object*, void*)

with qdev_construct()/qdev_destruct() as wrappers.


Regards,

Liviu

Re: [Qemu-devel] [RFC] QDev explicit constructors & destructors

[Liviu Ionescu]

> On 15 Jun 2015, at 13:48, Liviu Ionescu <ilg@livius.net> wrote:
> 
> ... One natural solution would be to add an explicit constructor, like in C++. 

if this solution is considered too complicated, it might also be possible to arrange for the constructor data to be passed to the existing .instance_init call.

the use case would be even simpler:

  DeviceState *mcu = qdev_create_with_data(NULL, TYPE_STM32F103RB, &constructor_data);
  {
      /* Set various properties. */
      qdev_prop_set_uint32(mcu, "hse-freq-hz", 8000000); /* 8.0 MHz */
      qdev_prop_set_uint32(mcu, "lse-freq-hz", 32768); /* 32 KHz */
  }
  qdev_realize(mcu); /* QDev specific step */

the implementation requires the addition of a new member to the Object structure (for example .initialize_data), that must be set by object_initialize_with_type() before calling object_init_with_type().


the existing .instace_init callbacks need not be modified, the prototype can be kept as it is now.

new classes, that requires configuration data during construction, could simply read the pointer to the constructor data from this Object initialize_data field.

one advantage would be that the possible inconsistency between the moment the object is allocated and constructed by the explicit constructor would be avoided.

one disadvantage would be that the construction params could no longer be passed via properties, but only via the constructor structure (which shouldn't be a major problem).

another disadvantage, also present in the single pointer explicit constructor, is the fact that the constructor structure is the same for the entire hierarchy construction;

for example it would not be possible to have such hierarchies:

class Base {
public:
	Base(int param) {};
	...
}

class Derived {
public:
	Derived(int param) : Base(param+1) {...};
	...
}

or

class Derived2 {
public:
	Derived2() : Base(7) {};
	...
}

these kind of construction policies can be achieved only with custom constructor callbacks, added to each class structure, and explicit chained calls issued by the user.

I currently use exactly this kind of custom calls in my code. (this is another alternate solution for this problem, do nothing in the framework and defer everything to the user).


of course, any other better ideas would be highly appreciated.


regards,

Liviu

Re: [Qemu-devel] [RFC] QDev explicit constructors & destructors

[Liviu Ionescu]

after some more experimenting, here are the conclusions:

- I added a new .construct pointer to all my classes:

    void (*construct)(Object *obj, void *data);

the 'data' pointer is optional, for qdev calls, parameters should be passed via properties.

- constructors should manually call their parent constructor

static void stm32_mcu_construct_callback(Object *obj, void *data)
{
    /* Call parent constructor */
    CORTEXM_MCU_GET_CLASS(obj)->construct(obj, NULL);

    ...
}

this is intentional, to allow the caller to set additional properties:

static void stm32_mcus_construct_callback(Object *obj, void *data)
{
    STM32DeviceClass *st_class = STM32_DEVICE_GET_CLASS(obj);
    STM32PartInfo *part_info = st_class->part_info;
    DeviceState *dev = DEVICE(obj);
    /*
     * Set additional constructor parameters, that were passed via
     * the .class_data and copied to custom class member.
     */
    qdev_prop_set_ptr(dev, "param-cortexm-capabilities",
            (void *) &part_info->cortexm);
    qdev_prop_set_ptr(dev, "param-stm32-capabilities",
            (void *) part_info->stm32);

    STM32_MCU_GET_CLASS(obj)->construct(obj, NULL);

    ...
}

- using this mechanism is pretty simple:

    DeviceState *mcu = qdev_alloc(NULL, TYPE_STM32F103RB);
    {
        qdev_prop_set_ptr(mcu, "param-machine", machine);
        STM32_DEVICE_GET_CLASS(mcu)->construct(OBJECT(mcu), NULL);

        /* Set the board specific oscillator frequencies. */
        qdev_prop_set_uint32(mcu, "hse-freq-hz", 8000000); /* 8.0 MHz */
        qdev_prop_set_uint32(mcu, "lse-freq-hz", 32768); /* 32 KHz */
    }
    qdev_realize(mcu);


- once the explicit constructors are in place, the .realize callbacks can be freed of other duties and return to their intended function, to propagate the "realized" event to parents and children:

static void stm32_mcu_realize_callback(DeviceState *dev, Error **errp)
{
    /* Call parent realize(). */
    if (!qdev_parent_realize(dev, errp, TYPE_STM32_MCU)) {
        return;
    }

    STM32MCUState *state = STM32_MCU_STATE(dev);

    /* Propagate realize() to children. */

    /* RCC */
    qdev_realize(DEVICE(state->rcc));

    /* FLASH */
    qdev_realize(DEVICE(state->flash));

    /* GPIO[A-G] */
    for (int i = 0; i < STM32_MAX_GPIO; ++i) {
        /* Realize all initialised GPIOs */
        if (state->gpio[i]) {
            qdev_realize(DEVICE(state->gpio[i]));
        }
    }
}


- as already mentioned, in my current implementation the .construct member is added to each class, but this is not needed as redundant, and should be added once to ObjectClass or at least to DeviceClass.

this would greatly simplify setting it and calling the constructor:

	object_construct(obj, data);

        qdev_prop_set_ptr(dev, "param-machine", machine);	
	qdev_construct(dev);

(for qdev the params are passed via properties, the optional data pointer is not used)


Peter, could you consider this proposal?

should I prepare a patch for it? 

adding this extra field in the class structure does not break compatibility with existing code, but would help reduce the mess with existing .realize, which is abused to do whatever initialisations were not possible in .instance_init; I expect that this will make future code easier to understand and maintain.


regards,

Liviu

Re: [Qemu-devel] [RFC] QDev explicit constructors & destructors

[Markus Armbruster]

Copying Andreas.

Liviu Ionescu <ilg@livius.net> writes:

> after some more experimenting, here are the conclusions:
>
> - I added a new .construct pointer to all my classes:
>
>     void (*construct)(Object *obj, void *data);
>
> the 'data' pointer is optional, for qdev calls, parameters should be passed via properties.
>
> - constructors should manually call their parent constructor
>
> static void stm32_mcu_construct_callback(Object *obj, void *data)
> {
>     /* Call parent constructor */
>     CORTEXM_MCU_GET_CLASS(obj)->construct(obj, NULL);
>
>     ...
> }
>
> this is intentional, to allow the caller to set additional properties:
>
> static void stm32_mcus_construct_callback(Object *obj, void *data)
> {
>     STM32DeviceClass *st_class = STM32_DEVICE_GET_CLASS(obj);
>     STM32PartInfo *part_info = st_class->part_info;
>     DeviceState *dev = DEVICE(obj);
>     /*
>      * Set additional constructor parameters, that were passed via
>      * the .class_data and copied to custom class member.
>      */
>     qdev_prop_set_ptr(dev, "param-cortexm-capabilities",
>             (void *) &part_info->cortexm);
>     qdev_prop_set_ptr(dev, "param-stm32-capabilities",
>             (void *) part_info->stm32);
>
>     STM32_MCU_GET_CLASS(obj)->construct(obj, NULL);
>
>     ...
> }
>
> - using this mechanism is pretty simple:
>
>     DeviceState *mcu = qdev_alloc(NULL, TYPE_STM32F103RB);
>     {
>         qdev_prop_set_ptr(mcu, "param-machine", machine);
>         STM32_DEVICE_GET_CLASS(mcu)->construct(OBJECT(mcu), NULL);
>
>         /* Set the board specific oscillator frequencies. */
>         qdev_prop_set_uint32(mcu, "hse-freq-hz", 8000000); /* 8.0 MHz */
>         qdev_prop_set_uint32(mcu, "lse-freq-hz", 32768); /* 32 KHz */
>     }
>     qdev_realize(mcu);
>
>
> - once the explicit constructors are in place, the .realize callbacks can be freed of other duties and return to their intended function, to propagate the "realized" event to parents and children:
>
> static void stm32_mcu_realize_callback(DeviceState *dev, Error **errp)
> {
>     /* Call parent realize(). */
>     if (!qdev_parent_realize(dev, errp, TYPE_STM32_MCU)) {
>         return;
>     }
>
>     STM32MCUState *state = STM32_MCU_STATE(dev);
>
>     /* Propagate realize() to children. */
>
>     /* RCC */
>     qdev_realize(DEVICE(state->rcc));
>
>     /* FLASH */
>     qdev_realize(DEVICE(state->flash));
>
>     /* GPIO[A-G] */
>     for (int i = 0; i < STM32_MAX_GPIO; ++i) {
>         /* Realize all initialised GPIOs */
>         if (state->gpio[i]) {
>             qdev_realize(DEVICE(state->gpio[i]));
>         }
>     }
> }
>
>
> - as already mentioned, in my current implementation the .construct member is added to each class, but this is not needed as redundant, and should be added once to ObjectClass or at least to DeviceClass.
>
> this would greatly simplify setting it and calling the constructor:
>
> 	object_construct(obj, data);
>
>         qdev_prop_set_ptr(dev, "param-machine", machine);	
> 	qdev_construct(dev);
>
> (for qdev the params are passed via properties, the optional data pointer is not used)
>
>
> Peter, could you consider this proposal?
>
> should I prepare a patch for it? 
>
> adding this extra field in the class structure does not break compatibility with existing code, but would help reduce the mess with existing .realize, which is abused to do whatever initialisations were not possible in .instance_init; I expect that this will make future code easier to understand and maintain.
>
>
> regards,
>
> Liviu

Re: [Qemu-devel] [RFC] QDev explicit constructors & destructors

[Liviu Ionescu]

> On 23 Jun 2015, at 10:47, Markus Armbruster <armbru@redhat.com> wrote:
> 
> Copying Andreas.

Hallo Andreas!


Liviu

Re: [Qemu-devel] [RFC] QDev explicit constructors & destructors

[Andreas Färber]

Hi Liviu,

Am 15.06.2015 um 15:35 schrieb Liviu Ionescu:
>> On 15 Jun 2015, at 13:48, Liviu Ionescu <ilg@livius.net> wrote:
>>
>> ... One natural solution would be to add an explicit constructor, like in C++. 
> 
> if this solution is considered too complicated, it might also be possible to arrange for the constructor data to be passed to the existing .instance_init call.
> 
> the use case would be even simpler:
> 
>   DeviceState *mcu = qdev_create_with_data(NULL, TYPE_STM32F103RB, &constructor_data);
>   {
>       /* Set various properties. */
>       qdev_prop_set_uint32(mcu, "hse-freq-hz", 8000000); /* 8.0 MHz */
>       qdev_prop_set_uint32(mcu, "lse-freq-hz", 32768); /* 32 KHz */
>   }

These {...} blocks have no justification at all, so please avoid them
for the sake of readability. Just use empty lines for grouping, that
spares indentation.

>   qdev_realize(mcu); /* QDev specific step */
[snip]

Please don't add new infrastructure with "qdev" in the name, qdev was
the old device infrastructure that has been replaced with QOM; use
"device" naming for any new APIs.

But really, you should read up on the two discussions, a) when Anthony
introduced QOM (several looong threads with Paolo, Avi and others) and
b) when I ran into the issue of virtual methods (start with the
resulting documentation in object.h).

You seem to be opening a can of worms without understanding where we're
coming from, how it's being used and where this is headed: QOM is not
just a random C++-in-C framework. We've specifically banned pointers
except for a handful by converting to link<> properties, and I see
adding random opaque pointers to the generic constructor model - be it
for devices or all objects - as a clear no-no. Look at how QOM objects
are being instantiated from QMP commands or config files as a hint to
what you're missing here (apart from CC'ing the right maintainers ;)).
(Note that quite similarly Linux has been replacing platform data
structs with Device Tree nodes.)

If you do in some local case want to pass local C data to the object,
you can already do so via .class_data if all instances are the same, as
in your case of a single MCU on a board probably.

If I'm not seeing the problem, you'll need to explain better why
class_init + instance_init + properties + realize doesn't fit your use case.

That said, your efforts of simplifying STM32 instantiation are
appreciated. I've been wanting to add an F429 Discovery machine for my
Linux port.

Cheers,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Dilip Upmanyu, Graham Norton; HRB
21284 (AG Nürnberg)

Re: [Qemu-devel] [RFC] QDev explicit constructors & destructors

[Liviu Ionescu]

> On 23 Jun 2015, at 13:39, Andreas Färber <afaerber@suse.de> wrote:
> 
> ... that spares indentation.

indentation? I'm not doing it manually, I just press a key combination in my Eclipse and indentation is done auto-magically.

> Please don't add new infrastructure with "qdev" in the name, qdev was
> the old device infrastructure that has been replaced with QOM;

I have difficulties to follow you. 

you mean qdev_* functions should no longer be used and all objects must be manipulated **only** with object_* functions?

is there a document mentioning this? the wiki page mentions them both, but does not visibly deprecate qdev.

> use
> "device" naming for any new APIs.

please provide a link to more details, I did not find any "device" naming API.

> But really, you should read up on the two discussions, a) when Anthony
> introduced QOM (several looong threads with Paolo, Avi and others)

could you provide more details to identify this thread? 

> and
> b) when I ran into the issue of virtual methods (start with the
> resulting documentation in object.h).

I read the documentation in object.h and I could not find an answer to my problem.

I need a clear method to instantiate multiple objects of the same class by providing multiple different parameters to the constructor.

your only explicit method to initialise instances is .instance_init and .instance_post_init, which do not take parameters.

the "realized" special property is another story, it is very confusing and abused to do the actual instance construction, which, in my opinion, is wrong and should be avoided.

the comments in qdev-core.h state "... devices must not create children during @realize; ..." and other restrictions:

* As an interim step, the #DeviceState:realized property is set by deprecated
* functions qdev_init() and qdev_init_nofail().
* In the future, devices will propagate this state change to their children
* and along busses they expose.
* The point in time will be deferred to machine creation, so that values
* set in @realize will not be introspectable beforehand. Therefore devices
* must not create children during @realize; they should initialize them via
* object_initialize() in their own #TypeInfo.instance_init and forward the
* realization events appropriately.

> 
> You seem to be opening a can of worms without understanding where we're
> coming from, how it's being used and where this is headed

any links to more info will be appreciated

> : QOM is not
> just a random C++-in-C framework.

no, it obviously is not C++ (what a pity), documentation is scarce, and, until I find out how to create multiple parametrised instances otherwise then in the realize() callback, I would say slightly dysfunctional framework.

> We've specifically banned pointers
> except for a handful by converting to link<> properties, and I see
> adding random opaque pointers to the generic constructor model - be it
> for devices or all objects - as a clear no-no.

in my code I pass all constructor parameters via properties, the 'void *data' was added to the prototype for completeness, but can be removed without problems.

> Look at how QOM objects
> are being instantiated from QMP commands or config files as a hint to
> what you're missing here

I did a search for 'object_' but could not find any relevant use cases. :-(

> If you do in some local case want to pass local C data to the object,
> you can already do so via .class_data if all instances are the same,

no, they are not.

> If I'm not seeing the problem, you'll need to explain better why
> class_init + instance_init + properties + realize doesn't fit your use case.

- class_init is out of question for multiple instances;
- instance_init is too early, since no parameters are available
- realize is too late, since it locks the object and no further properties can be set.

the general use case is

- alloc or use a statically allocated area
- set properties that represent constructor parameters
- call the constructor; based on the given parameters, dynamically create children objects, dynamically create properties, dynamically create property aliases from children objects to the main object
- set more properties to the newly added dynamic properties
- freeze the object configuration (via the notoriously confusing finalize())

as far as I understood the qom/qdev framework, the only way this can be done is by using a custom function (I called it .construct) to act as instance constructor. it does not need to take parameters, properties are fine.

my new proposal would be to add 

	void (*construct)(Object *obj)

to the Object class, that would greatly simplify usage compared to defining it in each derived class.

if adding this member to the Object class is not possible, I'll probably define MyObject, with this member in, and derive all classes from it. but it would be a pity...

> That said, your efforts of simplifying STM32 instantiation are
> appreciated.

thank you! :-)

I would still need some help to start it properly. 

I have tens of MCUs on my list, and tens of boards; maintaining them would be very difficult if not started properly.

> I've been wanting to add an F429 Discovery machine for my
> Linux port.

I already have a 'STM32F429I-Discovery' board on my list, my initial plan is to grow it to the point it blinks a LED, and then we'll extend it to suit your needs.


regards,

Liviu

Re: [Qemu-devel] [RFC] QDev explicit constructors & destructors

[Andreas Färber]

Am 23.06.2015 um 14:58 schrieb Liviu Ionescu:
>> On 23 Jun 2015, at 13:39, Andreas Färber <afaerber@suse.de> wrote:
>>
>> ... that spares indentation.
> 
> indentation? I'm not doing it manually, I just press a key combination in my Eclipse and indentation is done auto-magically.

Dude, we don't care *how* you indent, we only care about the results.
Just don't needlessly indent random pieces of code. See CODING_STYLE,
HACKING and related documents, also recent discussions.

>> Please don't add new infrastructure with "qdev" in the name, qdev was
>> the old device infrastructure that has been replaced with QOM;
> 
> I have difficulties to follow you. 
> 
> you mean qdev_* functions should no longer be used and all objects must be manipulated **only** with object_* functions?

No, I mean literally no qdev_* naming for new functions. I.e., no
qdev_realize(), qdev_alloc() or whatever you have proposed here and in
your other RFC. Maybe you just mixed up the names?

I am especially allergic to qdev_realize() because a) realize is a
QOM-era concept and b), as you later quote yourself, callers are not
supposed to call this themselves once we transition to a recursive
model. So introducing a new helper that is known to go away seems like
unnecessary churn.

> is there a document mentioning this? the wiki page mentions them both, but does not visibly deprecate qdev.

See my KVM Forum 2013 presentation explaining the history, on
linux-kvm.org. In short, qdev no longer exists, only some remnants where
we chose not to update the names.

You will also find further reviews of mine repeating the same naming
comments, like a broken record.

Generally, you will not find a lot of up-to-date external documents
about QEMU, as many things change over time. There's the code, the
in-tree documentation and previous review comments - sometimes a
presentation or two and maybe a not-yet-outdated Wiki page.

There's Anthony's old http://wiki.qemu-project.org/Features/QOM but not
updated since ~2012.

>> use
>> "device" naming for any new APIs.
> 
> please provide a link to more details, I did not find any "device" naming API.

No, I will not provide a link. Read carefully: for APIs, not an API.
All new code in hw/core/qdev.c has device_*, e.g., device_reset() and
device_{get,set}_{realized,hotplugged}().

>> But really, you should read up on the two discussions, a) when Anthony
>> introduced QOM (several looong threads with Paolo, Avi and others)
> 
> could you provide more details to identify this thread? 

No, I'm not going to do all your work! You're papering the list with
RFCs at a bad time, instead you should better take small steps and post
patches that can actually be built upon rather than pushing work off to
me. I have many other things to take care of, in particular during a
Soft Freeze (and after a vacation).

http://wiki.qemu.org/Planning/2.4

>> and
>> b) when I ran into the issue of virtual methods (start with the
>> resulting documentation in object.h).
> 
> I read the documentation in object.h and I could not find an answer to my problem.
> 
> I need a clear method to instantiate multiple objects of the same class by providing multiple different parameters to the constructor.

No, you don't need that, it's what you're proposing as a solution.

> your only explicit method to initialise instances is .instance_init and .instance_post_init, which do not take parameters.
> 
> the "realized" special property is another story, it is very confusing and abused to do the actual instance construction, which, in my opinion, is wrong and should be avoided.

You are free to have your opinion, but don't expect me to accept patches
just because some newbie doesn't like our established concept of
two-phase constructors.

> the comments in qdev-core.h state "... devices must not create children during @realize; ..." and other restrictions:
> 
> * As an interim step, the #DeviceState:realized property is set by deprecated
> * functions qdev_init() and qdev_init_nofail().
> * In the future, devices will propagate this state change to their children
> * and along busses they expose.
> * The point in time will be deferred to machine creation, so that values
> * set in @realize will not be introspectable beforehand. Therefore devices
> * must not create children during @realize; they should initialize them via
> * object_initialize() in their own #TypeInfo.instance_init and forward the
> * realization events appropriately.

Actually that code has just been updated yesterday, you need to rebase.

So what? I honestly don't understand what you're arguing here... Your
code examples are not telling.

Are you maybe missing that there are dynamic QOM properties in addition
to the qdev properties you use in your examples?

And that you can design your class hierarchies so that you don't need
the parametrized instantiation in the first place?

>> You seem to be opening a can of worms without understanding where we're
>> coming from, how it's being used and where this is headed
> 
> any links to more info will be appreciated
> 
>> : QOM is not
>> just a random C++-in-C framework.
> 
> no, it obviously is not C++ (what a pity), documentation is scarce, and, until I find out how to create multiple parametrised instances otherwise then in the realize() callback, I would say slightly dysfunctional framework.

We're turning in circles here, and that's a sure way to make me mad...

>> We've specifically banned pointers
>> except for a handful by converting to link<> properties, and I see
>> adding random opaque pointers to the generic constructor model - be it
>> for devices or all objects - as a clear no-no.
> 
> in my code I pass all constructor parameters via properties,

Then what's the problem with that? Just the looks of it?

Take a look at yesterday's pull, Daniel added a new shorthand function
to set properties via varargs, maybe that's what you're looking for.

> the 'void *data' was added to the prototype for completeness, but can be removed without problems.
> 
>> Look at how QOM objects
>> are being instantiated from QMP commands or config files as a hint to
>> what you're missing here
> 
> I did a search for 'object_' but could not find any relevant use cases. :-(

-device and -object handling in vl.c and qdev-monitor.c are examples.
The point is we have infrastructure (visitors) for marshaling
parameters, whereas your raw pointers don't.

>> If you do in some local case want to pass local C data to the object,
>> you can already do so via .class_data if all instances are the same,
> 
> no, they are not.
> 
>> If I'm not seeing the problem, you'll need to explain better why
>> class_init + instance_init + properties + realize doesn't fit your use case.
> 
> - class_init is out of question for multiple instances;
> - instance_init is too early, since no parameters are available
> - realize is too late, since it locks the object and no further properties can be set.
> 
> the general use case is
> 
> - alloc or use a statically allocated area
> - set properties that represent constructor parameters
> - call the constructor; based on the given parameters, dynamically create children objects, dynamically create properties, dynamically create property aliases from children objects to the main object
> - set more properties to the newly added dynamic properties

> - freeze the object configuration (via the notoriously confusing finalize())

Nope, realize freezes the properties, instance_finalize frees things as
counterpart to instance_init.

> 
> as far as I understood the qom/qdev framework, the only way this can be done is by using a custom function (I called it .construct) to act as instance constructor. it does not need to take parameters, properties are fine.
> 
> my new proposal would be to add 
> 
> 	void (*construct)(Object *obj)
> 
> to the Object class, that would greatly simplify usage compared to defining it in each derived class.
> 
> if adding this member to the Object class is not possible, I'll probably define MyObject, with this member in, and derive all classes from it. but it would be a pity...

You will see me answering briefly and rudely here, simply because you
are challenging a concept that has long been used in QEMU, even before
QOM in qdev, and I am not willing to spend time giving you personal
explanations of design decisions that i did not make myself and that you
could look up on your own, and yes it takes time researching it.

If other contributors see a need for changes - and I have seen no
contributor seconding any of your suggestions yet - then put it on a KVM
call agenda to reserve a timeslot instead of arguing about our coding
style, naming conventions and more by email.

Thanks,
Andreas

P.S. KVM Forum in August would be an opportunity for asking about
fundamental design principles like these.

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Dilip Upmanyu, Graham Norton; HRB
21284 (AG Nürnberg)

Re: [Qemu-devel] [RFC] QDev explicit constructors & destructors

[Liviu Ionescu]

> On 23 Jun 2015, at 17:25, Andreas Färber <afaerber@suse.de> wrote:
> 
> ... I honestly don't understand what you're arguing here... Your
> code examples are not telling.

:-(

> Are you maybe missing that there are dynamic QOM properties in addition
> to the qdev properties you use in your examples?

        /* Alias RCC properties to MCU, to hide internal details. */
        object_property_add_alias(obj, "hse-freq-hz", OBJECT(dev),
                "hse-freq-hz", NULL);
        object_property_add_alias(obj, "lse-freq-hz", OBJECT(dev),
                "lse-freq-hz", NULL);

I think that exactly the existence of dynamic QOM properties (aliases in my use case) is the reason why objects cannot be constructed by realize(), which is too late, since the newly created aliases will point to properties that will be frozen before having a chance to be used outside the call.

> And that you can design your class hierarchies so that you don't need
> the parametrized instantiation in the first place?

I agree that properly designed class hierarchies with virtual calls are preferred to parametrised instantiation, but in non-OO languages the effort to define the classes makes this impractical.

for example, for the STM32 MCUs, there are F0, F1, F2, F3, F4, F7, L0, L1 main families, each with several sub-families (F1 has low, medium, high, extra, connectivity, etc), it is not realistic to define QOM types for each one, especially when the differences are small, like some changed bits in a register. so my current decision was to implement all similar peripherals in a single class for all families, and, based on some flags in a capabilities structure, to adjust the behaviour for each device.

but I might be wrong.

> our established concept of two-phase constructors.

I fully agree with the two-phase constructors approach, but I'm not convinced the second step should be realize(), because I just ran into a case (dynamic QOM aliases) when I could not make it work.

> If other contributors see a need for changes - and I have seen no
> contributor seconding any of your suggestions yet

see below:

> On 14 Jun 2015, at 04:49, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:
> 
> Infact, I have an application of constructor arugments myself. This is
> for the ARM A9 MPCore container where the number of CPUs is variable
> per-instance. Same problem as you are facing, realize is too late,
> init is too early.

I'm convinced there are other cases too, especially for complex objects.

but if you do not see any issues with the existing design, ok, no problem, I'll find a workaround.

we'll stop this thread here, thank you for your time, 


Liviu

Re: [Qemu-devel] [RFC] QDev explicit constructors & destructors

[Peter Crosthwaite]

On Tue, Jun 23, 2015 at 7:25 AM, Andreas Färber <afaerber@suse.de> wrote:
> Am 23.06.2015 um 14:58 schrieb Liviu Ionescu:
>>> On 23 Jun 2015, at 13:39, Andreas Färber <afaerber@suse.de> wrote:
>>>
>>> ... that spares indentation.
>>
>> indentation? I'm not doing it manually, I just press a key combination in my Eclipse and indentation is done auto-magically.
>
> Dude, we don't care *how* you indent, we only care about the results.
> Just don't needlessly indent random pieces of code. See CODING_STYLE,
> HACKING and related documents, also recent discussions.
>
>>> Please don't add new infrastructure with "qdev" in the name, qdev was
>>> the old device infrastructure that has been replaced with QOM;
>>
>> I have difficulties to follow you.
>>
>> you mean qdev_* functions should no longer be used and all objects must be manipulated **only** with object_* functions?
>
> No, I mean literally no qdev_* naming for new functions. I.e., no
> qdev_realize(), qdev_alloc() or whatever you have proposed here and in
> your other RFC. Maybe you just mixed up the names?
>
> I am especially allergic to qdev_realize() because a) realize is a
> QOM-era concept and b), as you later quote yourself, callers are not
> supposed to call this themselves once we transition to a recursive
> model. So introducing a new helper that is known to go away seems like
> unnecessary churn.
>
>> is there a document mentioning this? the wiki page mentions them both, but does not visibly deprecate qdev.
>
> See my KVM Forum 2013 presentation explaining the history, on
> linux-kvm.org. In short, qdev no longer exists, only some remnants where
> we chose not to update the names.
>
> You will also find further reviews of mine repeating the same naming
> comments, like a broken record.
>
> Generally, you will not find a lot of up-to-date external documents
> about QEMU, as many things change over time. There's the code, the
> in-tree documentation and previous review comments - sometimes a
> presentation or two and maybe a not-yet-outdated Wiki page.
>
> There's Anthony's old http://wiki.qemu-project.org/Features/QOM but not
> updated since ~2012.
>
>>> use
>>> "device" naming for any new APIs.
>>
>> please provide a link to more details, I did not find any "device" naming API.
>
> No, I will not provide a link. Read carefully: for APIs, not an API.
> All new code in hw/core/qdev.c has device_*, e.g., device_reset() and
> device_{get,set}_{realized,hotplugged}().
>
>>> But really, you should read up on the two discussions, a) when Anthony
>>> introduced QOM (several looong threads with Paolo, Avi and others)
>>
>> could you provide more details to identify this thread?
>
> No, I'm not going to do all your work! You're papering the list with
> RFCs at a bad time, instead you should better take small steps and post
> patches that can actually be built upon rather than pushing work off to
> me. I have many other things to take care of, in particular during a
> Soft Freeze (and after a vacation).
>
> http://wiki.qemu.org/Planning/2.4
>

List discussions should be fine, regardless of release cycle phase.

This thread is getting wordy. I am struggling myself in that I still
don't see the need for constructor arguments for cross module linkages
and the static parameterisations. That said, usually you see these
kind of needs once you see the code.

We are all used to reviewing this kinda of stuff as patch series.
Given that you have something partially working, can you create a
threaded patch series that contains both the QOM change (at the front
of the series) and the application of it (all the STM stuff) at the
back. I will review it on list and see if I can think of alternate
construction sequence.

Regards.
Peter

>>> and
>>> b) when I ran into the issue of virtual methods (start with the
>>> resulting documentation in object.h).
>>
>> I read the documentation in object.h and I could not find an answer to my problem.
>>
>> I need a clear method to instantiate multiple objects of the same class by providing multiple different parameters to the constructor.
>
> No, you don't need that, it's what you're proposing as a solution.
>
>> your only explicit method to initialise instances is .instance_init and .instance_post_init, which do not take parameters.
>>
>> the "realized" special property is another story, it is very confusing and abused to do the actual instance construction, which, in my opinion, is wrong and should be avoided.
>
> You are free to have your opinion, but don't expect me to accept patches
> just because some newbie doesn't like our established concept of
> two-phase constructors.
>
>> the comments in qdev-core.h state "... devices must not create children during @realize; ..." and other restrictions:
>>
>> * As an interim step, the #DeviceState:realized property is set by deprecated
>> * functions qdev_init() and qdev_init_nofail().
>> * In the future, devices will propagate this state change to their children
>> * and along busses they expose.
>> * The point in time will be deferred to machine creation, so that values
>> * set in @realize will not be introspectable beforehand. Therefore devices
>> * must not create children during @realize; they should initialize them via
>> * object_initialize() in their own #TypeInfo.instance_init and forward the
>> * realization events appropriately.
>
> Actually that code has just been updated yesterday, you need to rebase.
>
> So what? I honestly don't understand what you're arguing here... Your
> code examples are not telling.
>
> Are you maybe missing that there are dynamic QOM properties in addition
> to the qdev properties you use in your examples?
>
> And that you can design your class hierarchies so that you don't need
> the parametrized instantiation in the first place?
>
>>> You seem to be opening a can of worms without understanding where we're
>>> coming from, how it's being used and where this is headed
>>
>> any links to more info will be appreciated
>>
>>> : QOM is not
>>> just a random C++-in-C framework.
>>
>> no, it obviously is not C++ (what a pity), documentation is scarce, and, until I find out how to create multiple parametrised instances otherwise then in the realize() callback, I would say slightly dysfunctional framework.
>
> We're turning in circles here, and that's a sure way to make me mad...
>
>>> We've specifically banned pointers
>>> except for a handful by converting to link<> properties, and I see
>>> adding random opaque pointers to the generic constructor model - be it
>>> for devices or all objects - as a clear no-no.
>>
>> in my code I pass all constructor parameters via properties,
>
> Then what's the problem with that? Just the looks of it?
>
> Take a look at yesterday's pull, Daniel added a new shorthand function
> to set properties via varargs, maybe that's what you're looking for.
>
>> the 'void *data' was added to the prototype for completeness, but can be removed without problems.
>>
>>> Look at how QOM objects
>>> are being instantiated from QMP commands or config files as a hint to
>>> what you're missing here
>>
>> I did a search for 'object_' but could not find any relevant use cases. :-(
>
> -device and -object handling in vl.c and qdev-monitor.c are examples.
> The point is we have infrastructure (visitors) for marshaling
> parameters, whereas your raw pointers don't.
>
>>> If you do in some local case want to pass local C data to the object,
>>> you can already do so via .class_data if all instances are the same,
>>
>> no, they are not.
>>
>>> If I'm not seeing the problem, you'll need to explain better why
>>> class_init + instance_init + properties + realize doesn't fit your use case.
>>
>> - class_init is out of question for multiple instances;
>> - instance_init is too early, since no parameters are available
>> - realize is too late, since it locks the object and no further properties can be set.
>>
>> the general use case is
>>
>> - alloc or use a statically allocated area
>> - set properties that represent constructor parameters
>> - call the constructor; based on the given parameters, dynamically create children objects, dynamically create properties, dynamically create property aliases from children objects to the main object
>> - set more properties to the newly added dynamic properties
>
>> - freeze the object configuration (via the notoriously confusing finalize())
>
> Nope, realize freezes the properties, instance_finalize frees things as
> counterpart to instance_init.
>
>>
>> as far as I understood the qom/qdev framework, the only way this can be done is by using a custom function (I called it .construct) to act as instance constructor. it does not need to take parameters, properties are fine.
>>
>> my new proposal would be to add
>>
>>       void (*construct)(Object *obj)
>>
>> to the Object class, that would greatly simplify usage compared to defining it in each derived class.
>>
>> if adding this member to the Object class is not possible, I'll probably define MyObject, with this member in, and derive all classes from it. but it would be a pity...
>
> You will see me answering briefly and rudely here, simply because you
> are challenging a concept that has long been used in QEMU, even before
> QOM in qdev, and I am not willing to spend time giving you personal
> explanations of design decisions that i did not make myself and that you
> could look up on your own, and yes it takes time researching it.
>
> If other contributors see a need for changes - and I have seen no
> contributor seconding any of your suggestions yet - then put it on a KVM
> call agenda to reserve a timeslot instead of arguing about our coding
> style, naming conventions and more by email.
>
> Thanks,
> Andreas
>
> P.S. KVM Forum in August would be an opportunity for asking about
> fundamental design principles like these.
>
> --
> SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Felix Imendörffer, Jane Smithard, Dilip Upmanyu, Graham Norton; HRB
> 21284 (AG Nürnberg)
>

Re: [Qemu-devel] [RFC] QDev explicit constructors & destructors

[Liviu Ionescu]

> On 23 Jun 2015, at 21:31, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:
> 
> List discussions should be fine, regardless of release cycle phase.

I apologise to all those who considered my messages not appropriate for this list or for this moment.

> ... usually you see these kind of needs once you see the code.

right

> Given that you have something partially working,

I have something fully working for my needs, i.e. I can run real-life applications created by my plug-in wizard on the emulator without any changes. currently only STM32F103RB is implemented, on Olimex STM32-H103 board, but copy/pasting code for other MCUs/boards is easy, I just delayed this to be sure the existing code is ok.

> can you create a
> threaded patch series that contains both the QOM change (at the front
> of the series) and the application of it (all the STM stuff) at the
> back. I will review it on list and see if I can think of alternate
> construction sequence.

unfortunately the differences are too many and a simple git format-patch between my branch and master creates 1480 files, too many to be posted here. and manually creating new branches and moving changes there is too labour intensive, at least for my current git knowledge.

I would really appreciate a Skype screen sharing session, it'll be much more efficient.

I highly appreciate your help,

Liviu

Re: [Qemu-devel] [RFC] QDev explicit constructors & destructors

[Andreas Färber]

Am 23.06.2015 um 20:31 schrieb Peter Crosthwaite:
> On Tue, Jun 23, 2015 at 7:25 AM, Andreas Färber <afaerber@suse.de> wrote:
>> Am 23.06.2015 um 14:58 schrieb Liviu Ionescu:
>>> could you provide more details to identify this thread?
>>
>> No, I'm not going to do all your work! You're papering the list with
>> RFCs at a bad time, instead you should better take small steps and post
>> patches that can actually be built upon rather than pushing work off to
>> me. I have many other things to take care of, in particular during a
>> Soft Freeze (and after a vacation).
>>
>> http://wiki.qemu.org/Planning/2.4
>>
> 
> List discussions should be fine, regardless of release cycle phase.

Posting stuff yes,
pinging me twice on my third day in office for hot air no,
expecting me to remember all non-2.4 threads post-freeze questionable.

It's also and foremost a matter of tone. Therefore what I said above.

Regards,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Dilip Upmanyu, Graham Norton; HRB
21284 (AG Nürnberg)

Re: [Qemu-devel] [RFC] QDev explicit constructors & destructors

[Liviu Ionescu]

> On 23 Jun 2015, at 21:31, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:
> 
> I will review it on list and see if I can think of alternate
> construction sequence.

I suggest your review should start from use cases like this:

    DeviceState *dev = my_dev_alloc(NULL, TYPE_MYTYPE);
    my_dev_prop_set_bool(dev, "param", true);
    my_dev_construct(OBJECT(dev), NULL);
    my_dev_prop_set_uint32(dev, "xyz", 123);
    my_dev_realize(dev);

please ignore the fictional function names, and just assume that constructing the object requires a parameter that is given on the command line, so it does not exist statically at .instance_init time.

the point here is that "xyz" is an alias, created during construction time.

if we take out the construction step, the code would look like:

    DeviceState *dev = my_dev_alloc(NULL, TYPE_MYTYPE);
    my_dev_prop_set_bool(dev, "param", true);
    my_dev_prop_set_uint32(dev, "xyz", 123);
    my_dev_realize(dev);

since "xyz" must be an alias to an object that cannot be created at .instance_init, the alias itself cannot be created at .instance_init time, and setting "xyz" will fail.

another solution would look like:

    DeviceState *dev = my_dev_alloc(NULL, TYPE_MYTYPE);
    my_dev_prop_set_string(dev, "param", "something");
    my_dev_realize(dev);
    my_dev_prop_set_uint32(dev, "xyz", 123);

which obviously fails while trying to set the locked "xyz" property.

as suggested by Andreas, QOM prefers all objects to be created by default constructors (in .instance_init).

in this case the code might look like:

    DeviceState *dev;
    if (param_value)
      dev = my_dev_alloc(NULL, TYPE_MYTYPE1);
    else
      dev = my_dev_alloc(NULL, TYPE_MYTYPE2);
    my_dev_prop_set_uint32(dev, "xyz", 123);
    my_dev_realize(dev);

but, in my opinion, this is impractical for complex objects. 

for example instead of using a single "stm32-mcu" object parametrised with a capabilities structure, such a solution would require tens of separate objects for each existing MCU in a family, which is not realistic.


regards,

Liviu

Re: [Qemu-devel] [RFC] QDev explicit constructors & destructors

[Peter Crosthwaite]

On Tue, Jun 23, 2015 at 12:10 PM, Liviu Ionescu <ilg@livius.net> wrote:
>
>> On 23 Jun 2015, at 21:31, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:
>>
>> List discussions should be fine, regardless of release cycle phase.
>
> I apologise to all those who considered my messages not appropriate for this list or for this moment.
>
>> ... usually you see these kind of needs once you see the code.
>
> right
>
>> Given that you have something partially working,
>
> I have something fully working for my needs, i.e. I can run real-life applications created by my plug-in wizard on the emulator without any changes. currently only STM32F103RB is implemented, on Olimex STM32-H103 board, but copy/pasting code for other MCUs/boards is easy, I just delayed this to be sure the existing code is ok.
>
>> can you create a
>> threaded patch series that contains both the QOM change (at the front
>> of the series) and the application of it (all the STM stuff) at the
>> back. I will review it on list and see if I can think of alternate
>> construction sequence.
>
> unfortunately the differences are too many and a simple git format-patch between my branch and master creates 1480 files,

Have you actually done this many commits or is mainline activity
coming up in the change set? To handle the latter I suggest starting
from scratch.

git checkout -b new_branch
git reset origin/master
git status

This purges your git history and resets your new branch to be
mainlines history, but you will have all your changes now unstaged.

git add (use -p to get some interaction going), to create patches. If
you have usable commit messages from the original branch, use git
commit -c to recycle them.

If the future, you may find it simpler to rebase your branch
continually rather than merge mainline in periodically.

If you have large amounts of out-of-scope content, commit it all as a
patch that you don't send.

If you are confident your patches are conflict free you could use a
cherry-pick based approach, but this is safer for getting a tree state
that matches your working code.

Use git rebase -i and all its capabilities to refine the series after
you have a patch set.

> too many to be posted here. and manually creating new branches and moving changes there is too labour intensive, at least for my current git knowledge.
>

It's a necessary evil of being downstream that you have a few branches going.


> I would really appreciate a Skype screen sharing session, it'll be much more efficient.
>

Give it a go and let me know where it gets stuck.

Regards,
Peter

> I highly appreciate your help,
>
> Liviu
>
>

Re: [Qemu-devel] [RFC] QDev explicit constructors & destructors

[Liviu Ionescu]

> On 23 Jun 2015, at 23:57, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:
> 
> Give it a go and let me know where it gets stuck.

I sent you a Skype invitation, but apparently it did not pass through. could you try from your side (my id is 'ilg-ul').


regards,

Liviu

Re: [Qemu-devel] [RFC] QDev explicit constructors & destructors

[Liviu Ionescu]

> On 23 Jun 2015, at 23:10, Liviu Ionescu <ilg@livius.net> wrote:
> 
> another solution would look like:
> 
>    DeviceState *dev = my_dev_alloc(NULL, TYPE_MYTYPE);
>    my_dev_prop_set_string(dev, "param", "something");
>    my_dev_realize(dev);
>    my_dev_prop_set_uint32(dev, "xyz", 123);
> 
> which obviously fails while trying to set the locked "xyz" property.

as a last resort to avoid explicit constructors, what about doing whatever tricks are necessary to have these properties unlocked even after realize()?


Peter C, is this solution acceptable? 

if so, I'll update my code and perhaps tomorrow we can schedule the Skype call.


regards,

Liviu

Re: [Qemu-devel] [RFC] QDev explicit constructors & destructors

[Peter Crosthwaite]

On Wed, Jun 24, 2015 at 12:30 AM, Liviu Ionescu <ilg@livius.net> wrote:
>
>> On 23 Jun 2015, at 23:10, Liviu Ionescu <ilg@livius.net> wrote:
>>
>> another solution would look like:
>>
>>    DeviceState *dev = my_dev_alloc(NULL, TYPE_MYTYPE);
>>    my_dev_prop_set_string(dev, "param", "something");
>>    my_dev_realize(dev);
>>    my_dev_prop_set_uint32(dev, "xyz", 123);
>>
>> which obviously fails while trying to set the locked "xyz" property.
>
> as a last resort to avoid explicit constructors, what about doing whatever tricks are necessary to have these properties unlocked even after realize()?
>

Props can be changed after realize if you use a QOM prop rather than a qdev.

>
> Peter C, is this solution acceptable?
>

Settable props are generally acceptable, but it is unusual for a
machine init routine.

> if so, I'll update my code and perhaps tomorrow we can schedule the Skype call.
>

Got your vite on Skype. Will probably have to be an evening your time
to catch me in the morning.

Regards,
Peter

>
> regards,
>
> Liviu
>
>

Re: [Qemu-devel] [RFC] QDev explicit constructors & destructors

[Paolo Bonzini]

On 23/06/2015 22:10, Liviu Ionescu wrote:
> I suggest your review should start from use cases like this:
> 
>     DeviceState *dev = my_dev_alloc(NULL, TYPE_MYTYPE);
>     my_dev_prop_set_bool(dev, "param", true);
>     my_dev_construct(OBJECT(dev), NULL);
>     my_dev_prop_set_uint32(dev, "xyz", 123);
>     my_dev_realize(dev);
> 
> please ignore the fictional function names, and just assume that constructing the object requires a parameter that is given on the command line, so it does not exist statically at .instance_init time.
> 
> the point here is that "xyz" is an alias, created during construction time.

Can you explain this with real function names and parameters?

Paolo

Re: [Qemu-devel] [RFC] QDev explicit constructors & destructors

[Liviu Ionescu]

> On 24 Jun 2015, at 11:29, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:
> 
>> 
>> Peter C, is this solution acceptable?
>> 
> 
> Settable props are generally acceptable, but it is unusual for a
> machine init routine.

ok, I managed to restructure my code to get rid of the explicit constructor. :-)

I also avoided properties aliases, and did prop setting in two steps, in machine init I set props in MCU, and from here I manually set them in specific peripherals, after they are created. less efficient then aliases, but functional.

however, there is one aspect that I'm not compliant with your requirements: I'm still creating objects in realize(), since many of my objects have dynamic content. for example MCUs have different number of GPIOs, it is possible to create all of them in init and then map only the used ones, but to me this looks sub-optimal.

but, if this is a mandatory requirement, I'll do may best to comply.


the machine init code now looks like this:

static void stm32_h103_board_init_callback(MachineState *machine)
{
    cm_board_greeting(machine);

    {
        /* Create the MCU */
        DeviceState *mcu = cm_create(TYPE_STM32F103RB);

        qdev_prop_set_ptr(mcu, "machine", machine);

        /* Set the board specific oscillator frequencies. */
        qdev_prop_set_uint32(mcu, "hse-freq-hz", 8000000); /* 8.0 MHz */
        qdev_prop_set_uint32(mcu, "lse-freq-hz", 32768); /* 32 KHz */

        cm_realize(mcu);
    }

    {
        /* Create the board LED */
        DeviceState *led = cm_create(TYPE_GPIO_LED);

        /* STM32-H103 Green LED, GPIOC[12], active low */
        qdev_prop_set_bit(led, "active-low", true);
        qdev_prop_set_string(led, "on-message", "[Green LED On]\n");
        qdev_prop_set_string(led, "off-message", "[Green LED Off]\n");

        gpio_led_connect(led, "/machine/stm32/gpio[c]", 12);

        cm_realize(led);
    }
}

I hope it is better now.

> Got your vite on Skype. Will probably have to be an evening your time
> to catch me in the morning.

fine with me.


regards,

Liviu

Re: [Qemu-devel] [RFC] QDev explicit constructors & destructors

[Paolo Bonzini]

On 24/06/2015 11:29, Liviu Ionescu wrote:
> ok, I managed to restructure my code to get rid of the explicit
> constructor. :-)
> 
> I also avoided properties aliases, and did prop setting in two steps,
> in machine init I set props in MCU, and from here I manually set them
> in specific peripherals, after they are created. less efficient then
> aliases, but functional.
> 
> however, there is one aspect that I'm not compliant with your
> requirements: I'm still creating objects in realize(), since many of
> my objects have dynamic content. for example MCUs have different
> number of GPIOs, it is possible to create all of them in init and
> then map only the used ones, but to me this looks sub-optimal.

I think this is okay.

Paolo

Re: [Qemu-devel] [RFC] QDev explicit constructors & destructors

[Andreas Färber]

Am 24.06.2015 um 11:51 schrieb Paolo Bonzini:
> On 24/06/2015 11:29, Liviu Ionescu wrote:
>> ok, I managed to restructure my code to get rid of the explicit
>> constructor. :-)
>>
>> I also avoided properties aliases, and did prop setting in two steps,
>> in machine init I set props in MCU, and from here I manually set them
>> in specific peripherals, after they are created. less efficient then
>> aliases, but functional.
>>
>> however, there is one aspect that I'm not compliant with your
>> requirements: I'm still creating objects in realize(), since many of
>> my objects have dynamic content. for example MCUs have different
>> number of GPIOs, it is possible to create all of them in init and
>> then map only the used ones, but to me this looks sub-optimal.
> 
> I think this is okay.

It's acceptable (Error **errp available for reporting) but not ideal (no
way to inspect and set properties on the created child devices).

Therefore my pointer to dynamic properties, which we made Alexey use for
ppc's XICS for a similar problem of N child devices.

Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Dilip Upmanyu, Graham Norton; HRB
21284 (AG Nürnberg)

Re: [Qemu-devel] [RFC] QDev explicit constructors & destructors

[Andreas Färber]

Am 24.06.2015 um 11:29 schrieb Liviu Ionescu:
> the machine init code now looks like this:
> 
> static void stm32_h103_board_init_callback(MachineState *machine)
> {
>     cm_board_greeting(machine);
> 
>     {
>         /* Create the MCU */
>         DeviceState *mcu = cm_create(TYPE_STM32F103RB);
> 
>         qdev_prop_set_ptr(mcu, "machine", machine);
> 
>         /* Set the board specific oscillator frequencies. */
>         qdev_prop_set_uint32(mcu, "hse-freq-hz", 8000000); /* 8.0 MHz */
>         qdev_prop_set_uint32(mcu, "lse-freq-hz", 32768); /* 32 KHz */
> 
>         cm_realize(mcu);
>     }
> 
>     {
>         /* Create the board LED */
>         DeviceState *led = cm_create(TYPE_GPIO_LED);
> 
>         /* STM32-H103 Green LED, GPIOC[12], active low */
>         qdev_prop_set_bit(led, "active-low", true);
>         qdev_prop_set_string(led, "on-message", "[Green LED On]\n");
>         qdev_prop_set_string(led, "off-message", "[Green LED Off]\n");
> 
>         gpio_led_connect(led, "/machine/stm32/gpio[c]", 12);
> 
>         cm_realize(led);
>     }
> }
> 
> I hope it is better now.

You're still doing the {...} thing I asked you not to do.

static ...
{
    DeviceState *mcu, *led;

    mcu = ...
    ...

    led = ...
    ...
}

Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Dilip Upmanyu, Graham Norton; HRB
21284 (AG Nürnberg)

Re: [Qemu-devel] [RFC] QDev explicit constructors & destructors

[Liviu Ionescu]

> On 24 Jun 2015, at 16:50, Andreas Färber <afaerber@suse.de> wrote:
> 

> You're still doing the {...} thing I asked you not to do.

thank you for your suggestions.

right now I'm more concerned on which APIs to use, and my current understanding is that you prefer direct QOM calls over QDev, which are deprecated. is this correct?

regarding the layout of my code, I did not make a final decision yet, but it might very well be C++, with some wrappers over your QOM objects.


regards,

Liviu

Re: [Qemu-devel] [RFC] QDev explicit constructors & destructors

[Andreas Färber]

Am 24.06.2015 um 16:11 schrieb Liviu Ionescu:
> On 24 Jun 2015, at 16:50, Andreas Färber <afaerber@suse.de> wrote:
> 
>> You're still doing the {...} thing I asked you not to do.
> 
> thank you for your suggestions.

It's not a suggestion. Our Coding Style is a mandatory requirement for
code review. I already pointed out that it affects readability for those
accustomed to our Coding Style. Instead of you arguing over it I
expected you to acknowledge it and to adapt your code.

> right now I'm more concerned on which APIs to use, and my current understanding is that you prefer direct QOM calls over QDev, which are deprecated. is this correct?
> 
> regarding the layout of my code, I did not make a final decision yet, but it might very well be C++, with some wrappers over your QOM objects.

First, they're not "my" QOM objects. I did not invent it.

Second, we are certainly not going to let every contributor, especially
not new ones, make their own choices of programming language inside our
code base. QOM was not just a transitory tool, some people here (not me)
were and are against C++, which currently is optional for AArch64
disassembler.

Regards,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Dilip Upmanyu, Graham Norton; HRB
21284 (AG Nürnberg)

Re: [Qemu-devel] [RFC] QDev explicit constructors & destructors

[Liviu Ionescu]

> On 24 Jun 2015, at 17:18, Andreas Färber <afaerber@suse.de> wrote:
> 
> ... our Coding Style.

sorry, but I checked both QEMU Coding Style and QEMU Coding Guidelines and could not find any rule forbidding the use of blocks for introducing local scopes.


regards,

Liviu

Re: [Qemu-devel] [RFC] QDev explicit constructors & destructors

[Andreas Färber]

Am 24.06.2015 um 16:39 schrieb Liviu Ionescu:
> 
>> On 24 Jun 2015, at 17:18, Andreas Färber <afaerber@suse.de> wrote:
>>
>> ... our Coding Style.
> 
> sorry, but I checked both QEMU Coding Style and QEMU Coding Guidelines and could not find any rule forbidding the use of blocks for introducing local scopes.

I will not repeat myself here. That's not constructive.

Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Dilip Upmanyu, Graham Norton; HRB
21284 (AG Nürnberg)

Re: [Qemu-devel] [RFC] QDev explicit constructors & destructors

[Liviu Ionescu]

> On 24 Jun 2015, at 17:11, Liviu Ionescu <ilg@livius.net> wrote:
> 
> ... it might very well be C++, with some wrappers ...

unfortunately this is not technically feasible, for many reasons, one of them being the 'struct Object' member named 'class' :-(


regards,

Liviu