Re: [Qemu-devel] [PATCH 0/5 v2] cpu: make a child of DeviceState

[Igor Mammedov <imammedo@redhat.com>]

On Mon, 20 Aug 2012 06:52:51 +0200
Stefan Weil <sw@weilnetz.de> wrote:

> Am 20.08.2012 01:39, schrieb Igor Mammedov:
> > this is th 3rd approach to make CPU a child of DeviceState
> > for both kinds of targets *-user and *-softmmu. It seems
> > with current state of qemu it doesn't take too much effort
> > to make it compile. Please check if it doesn't break
> > something on other targets/archs/hosts than i386.
> >
> > what's tested:
> >    - compile tested building all targets on FC17x64 host.
> >    - briefly tested i386 user and softmmu targets
> >
> > Anthony Liguori (1):
> >    qdev: split up header so it can be used in cpu.h
> >
> > Igor Mammedov (4):
> >    move qemu_irq typedef out of cpu-common.h
> >    qapi-types.h doesn't really need to include qemu-common.h
> >    cleanup error.h, included qapi-types.h aready has stdbool.h
> >    make CPU a child of DeviceState
> >
> >   error.h               |    1 -
> >   hw/arm-misc.h         |    1 +
> >   hw/bt.h               |    2 +
> >   hw/devices.h          |    2 +
> >   hw/irq.h              |    2 +
> >   hw/mc146818rtc.c      |    1 +
> >   hw/omap.h             |    1 +
> >   hw/qdev-addr.c        |    1 +
> >   hw/qdev-core.h        |  240 ++++++++++++++++++++++++++++++++
> >   hw/qdev-monitor.h     |   16 ++
> >   hw/qdev-properties.c  |    1 +
> >   hw/qdev-properties.h  |  128 +++++++++++++++++
> >   hw/qdev.c             |    1 +
> >   hw/qdev.h             |  371 +------------------------------------------------
> >   hw/soc_dma.h          |    1 +
> >   hw/xen.h              |    1 +
> >   include/qemu/cpu.h    |    6 +-
> >   qemu-common.h         |    1 -
> >   scripts/qapi-types.py |    2 +-
> >   sysemu.h              |    1 +
> >   20 files changed, 407 insertions(+), 373 deletions(-)
> >   create mode 100644 hw/qdev-core.h
> >   create mode 100644 hw/qdev-monitor.h
> >   create mode 100644 hw/qdev-properties.h
> >
> 
> I'd prefer if you could keep the following simple pattern:
> 
> * Start includes in *.c files with config.h (optionally)
>    and qemu-common.h.
Can't agree with you on this. I'd say that every header should be be self
sufficient, include other headers if it uses types from them and NOT depend on
the position where it's included, it should provide all its own deps.

> 
> * Don't include standard include files which are already
>    included in qemu-common.h
Probably initially qemu-common.h was intended to simplify inclusion
of standard headers and glue stuff in multi-os build environment. but it seems
to be become misused. It includes now a lot of stuff that is not common to
every file it's included in.
Perhaps we should split out of it std includes and glue layer into something
like std/host-common.h and case on case basis move out other type
definitions that are not common into there appropriate places. Like with
qemu_irq.

> 
> * Don't include qemu-common.h in *.h files.
I'm all for it. But it's difficult right now because it tends to be included
in a lot of headers that don't really need it and then other headers happens
to depend on previous inclusion of it. It's difficult to untangle this in one
take, but could be possible in small steps.

[1/1] is a direction that could be used to put type custom types in their
proper places. It would be better way long term and help to avoid problems
with including one header in another. 

> 
> Regards,
> 
> Stefan Weil
> 


-- 
Regards,
  Igor