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

["Andreas Färber" <afaerber@suse.de>]

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)