[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