Re: [Qemu-devel] DO_UPCAST confusion

[Gerd Hoffmann]

  Hi,

> > state = DO_UPCAST(InternalSnapshotState, common, common);
> 
> I much prefer the name container_of() (which is a bit more obvious that
> it is finding the container or derived type that embeds the parent
> type), but if we have to keep the ugly name, could we at least clean up
> the comment to make sense, and fix the name to be DO_DOWNCAST to match
> what it is actually doing?

We don't have to keep it.  DO_UPCAST is there for historical reasons,
was added with qdev.  It used to be used alot more, and the move from
qdev to QOM already killed alot of it.  But so far nobody went out
cleaning up the remaining places.

Feel free to go ahead and zap it.  But if you touch all the places
anyway please I'd much prefer a conversion to container_of() right away
instead of renaming it to something else.

cheers,
  Gerd

Re: [Qemu-devel] DO_UPCAST confusion

[Markus Armbruster]

Peter Maydell  writes:

> On 21 October 2015 at 23:49, Eric Blake  wrote:
>> I much prefer the name container_of() (which is a bit more obvious that
>> it is finding the container or derived type that embeds the parent
>> type), but if we have to keep the ugly name, could we at least clean up
>> the comment to make sense, and fix the name to be DO_DOWNCAST to match
>> what it is actually doing?

DO_UPCAST() needs deletion, not renaming.

> You can't call this one container_of, because it's doing
> container_of plus extra checking.
>
> As Gerd says, most of these uses should probably go away, but
> ideally by conversion to the QOM cast macros rather than
> just dropping down to use of container_of.

Yes, any DO_UPCAST() of QOM objects should be converted to the proper
QOM casts.

DO_UPCAST() of something that should be a QOM object... well, ideally
the something should be converted to QOM, but that's hardly simple
cleanup.

Any DO_UPCAST() that have crept into other code could be simply replaced
by container_of().  If we want to keep the extra checking Peter
mentioned, we could have a container_of_checked() or something.  *Not* a
renamed DO_UPCAST(), because DO_UPCAST() pointlessly takes its arguments
in a different order than container_of().

Re: [Qemu-devel] DO_UPCAST confusion

[Peter Maydell]

On 21 October 2015 at 23:49, Eric Blake  wrote:
> I much prefer the name container_of() (which is a bit more obvious that
> it is finding the container or derived type that embeds the parent
> type), but if we have to keep the ugly name, could we at least clean up
> the comment to make sense, and fix the name to be DO_DOWNCAST to match
> what it is actually doing?

You can't call this one container_of, because it's doing
container_of plus extra checking.

As Gerd says, most of these uses should probably go away, but
ideally by conversion to the QOM cast macros rather than
just dropping down to use of container_of.

thanks
-- PMM

Re: [Qemu-devel] DO_UPCAST confusion

[Markus Armbruster]

Eric Blake  writes:

> On 10/22/2015 06:00 AM, Markus Armbruster wrote:
>> Peter Maydell  writes:
>> 
>>> On 21 October 2015 at 23:49, Eric Blake  wrote:
 I much prefer the name container_of() (which is a bit more obvious that
 it is finding the container or derived type that embeds the parent
 type), but if we have to keep the ugly name, could we at least clean up
 the comment to make sense, and fix the name to be DO_DOWNCAST to match
 what it is actually doing?
>> 
>> DO_UPCAST() needs deletion, not renaming.
>> 
>>> You can't call this one container_of, because it's doing
>>> container_of plus extra checking.
>>>
>
> The only extra checking is that the derived class has the parent class
> as its first member.
>
>> Any DO_UPCAST() that have crept into other code could be simply replaced
>> by container_of().  If we want to keep the extra checking Peter
>> mentioned, we could have a container_of_checked() or something.  *Not* a
>> renamed DO_UPCAST(), because DO_UPCAST() pointlessly takes its arguments
>> in a different order than container_of().
>
> Or maybe container_of_first(), to make it obvious that the parent class
> is the first member.
>
> But how often does it really matter whether the container of the parent
> class had the parent as the first member?  I guess we'll find out as we
> try to nuke DO_UPCAST.

I don't know.  I dimly remember discussing a "parent must be first"
restriction in qdev a long time ago.  Perhaps all that's left of it by
now is this check.

Re: [Qemu-devel] DO_UPCAST confusion

[Paolo Bonzini]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA256



On 22/10/2015 15:55, Eric Blake wrote:
> Or maybe container_of_first(), to make it obvious that the parent
> class is the first member.
> 
> But how often does it really matter whether the container of the
> parent class had the parent as the first member?  I guess we'll
> find out as we try to nuke DO_UPCAST.

It's just a compile-time check.  DO_UPCAST is basically a type-safe(r)
version of (Foo *)bar.

Paolo
-BEGIN PGP SIGNATURE-
Version: GnuPG v2

iQEcBAEBCAAGBQJWKOwUAAoJEL/70l94x66DS+gH/Rk7DXLNiYm0jhsgKwxfZHCn
sygpayCGxpiSJ1PeRlNwjCImt1+wazqohStogCxnMkvB8mcAY5C8tTxsShzh1eao
jEkGkwPcpDdR6bC5LSzot8uNI/GZEC1xEoXF8Zi6Y+t62LfL+hmKgydxrBNu4GHk
kzczEv/4DpLh2KsZOj/yOmuw9b3hn0z05wM5taZvr/zVWB2eqenTISUF4PttgSi/
QrZapdHM0kq2Y9iGdM2necIFWwRjSm+8Gjs/cdETp30gSCA+TqlFnGgD4+sW2lrM
C3gc5KzttgAuoNMkUNo7ibRV3P1qA/aiOZFzO4LXMrV69yox7ur8OrakXR8hOJA=
=J4SY
-END PGP SIGNATURE-

Re: [Qemu-devel] DO_UPCAST confusion

[Eric Blake]

On 10/22/2015 06:00 AM, Markus Armbruster wrote:
> Peter Maydell  writes:
> 
>> On 21 October 2015 at 23:49, Eric Blake  wrote:
>>> I much prefer the name container_of() (which is a bit more obvious that
>>> it is finding the container or derived type that embeds the parent
>>> type), but if we have to keep the ugly name, could we at least clean up
>>> the comment to make sense, and fix the name to be DO_DOWNCAST to match
>>> what it is actually doing?
> 
> DO_UPCAST() needs deletion, not renaming.
> 
>> You can't call this one container_of, because it's doing
>> container_of plus extra checking.
>>

The only extra checking is that the derived class has the parent class
as its first member.

> Any DO_UPCAST() that have crept into other code could be simply replaced
> by container_of().  If we want to keep the extra checking Peter
> mentioned, we could have a container_of_checked() or something.  *Not* a
> renamed DO_UPCAST(), because DO_UPCAST() pointlessly takes its arguments
> in a different order than container_of().

Or maybe container_of_first(), to make it obvious that the parent class
is the first member.

But how often does it really matter whether the container of the parent
class had the parent as the first member?  I guess we'll find out as we
try to nuke DO_UPCAST.

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

[Qemu-devel] DO_UPCAST confusion

[Eric Blake]

In include/qemu/compiler.h, we have this gem:

> #ifndef container_of
> #define container_of(ptr, type, member) ({ \
> const typeof(((type *) 0)->member) *__mptr = (ptr); \
> (type *) ((char *) __mptr - offsetof(type, member));})
> #endif
> 
> /* Convert from a base type to a parent type, with compile time checking. */
> #ifdef __GNUC__
> #define DO_UPCAST(type, field, dev) ( __extension__ ( { \
> char __attribute__((unused)) offset_must_be_zero[ \
> -offsetof(type, field)]; \
> container_of(dev, type, field);}))
> #else
> #define DO_UPCAST(type, field, dev) container_of(dev, type, field)
> #endif

That comment is horrible.  In object-oriented programming, there are two
common sets of terminology:

base and derived classes (derived adds on to base)
parent and child classes (child inherits from parent)

Going from "base type" to "parent type" makes NO SENSE, since those two
terms are synonyms between the two terminologies.

Furthermore, according to these references:
https://programmers.stackexchange.com/questions/148615/what-is-upcasting-downcasting
http://en.wikipedia.org/wiki/Downcasting

the act of going from a base class to a derived class (or from a parent
type to a child type) is considered downcasting.  Yet our definition of
DO_UPCAST is used to go from a base class to the derived class.

Example usage in blockdev.c is to declare a derived class:

> /* internal snapshot private data */
> typedef struct InternalSnapshotState {
> BlkTransactionState common;
...
> };

then to take a generic parent pointer and convert it into the derived type:

> static void internal_snapshot_prepare(BlkTransactionState *common,
>   Error **errp)
> {
...
> InternalSnapshotState *state;
...
> state = DO_UPCAST(InternalSnapshotState, common, common);

I much prefer the name container_of() (which is a bit more obvious that
it is finding the container or derived type that embeds the parent
type), but if we have to keep the ugly name, could we at least clean up
the comment to make sense, and fix the name to be DO_DOWNCAST to match
what it is actually doing?

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature