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

[Peter Crosthwaite]

On Tue, Jun 23, 2015 at 7:25 AM, Andreas Färber <address@hidden> wrote:
> Am 23.06.2015 um 14:58 schrieb Liviu Ionescu:
>>> On 23 Jun 2015, at 13:39, Andreas Färber <address@hidden> 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)
>