Re: [Qemu-devel] [RFC PATCH 0/8] Remove stub mon-protocol-event for block

[Wenchao Xia]

于 2013/9/16 18:02, Paolo Bonzini 写道:
> Il 16/09/2013 06:59, Wenchao Xia ha scritto:
> > 于 2013/9/12 17:31, Paolo Bonzini 写道:
> > > Il 12/09/2013 11:15, Wenchao Xia ha scritto:
> > > > This series will remove the usage of symbols of mon-protocol-event in
> > > > qemu-img, qemu-nbd and qemu-io, in short remove the connetion for block
> > > > layer.
> > > >
> > > > Background:
> > > >     I am tring to decouple block layer code with other unnnessary
> > > > components,
> > > > and in ./stub there many symbols that qemu-img linked as fake
> > > > implemtion.
> > > > As a first step, I am decouple monitor with block layer code, this is
> > > > the
> > > > first part of it.
> > > >     There are still other stub symbols for monitor, which will be
> > > > solved later.
> > > > It seems error handlering is also link with those symbols, and will
> > > > adjust
> > > > that.
> > > >
> > > > Wenchao Xia (8):
> > > >     1 block: use type MonitorEvent directly
> > > >     2 block: do not include monitor.h in block.c
> > > >     3 qapi: move MonitorEvent define
> > > >     4 qapi: rename MonitorEvent to QEvent
> > > >     5 block: add a callback layer for common functions
> > > >     6 block: replace monitor_protocol_event() with callback
> > > >     7 block: do not include monitor.h
> > > >     7 stubs: remove mon-protocol-event.o in stub obj
> > > >
> > > >    block.c                    |   22 ++++++++++++++++++----
> > > >    block/qcow2-refcount.c     |    4 +++-
> > > >    blockjob.c                 |   10 ++++++++--
> > > >    include/block/block.h      |   12 ++++++++++++
> > > >    include/block/block_int.h  |    3 +--
> > > >    include/monitor/monitor.h  |   40
> > > > ++--------------------------------------
> > > >    include/qapi/qmp/qevent.h  |   41
> > > > +++++++++++++++++++++++++++++++++++++++++
> > > >    include/qapi/qmp/types.h   |    1 +
> > > >    monitor.c                  |   12 ++++++------
> > > >    stubs/Makefile.objs        |    1 -
> > > >    stubs/mon-protocol-event.c |    2 +-
> > > >    tests/Makefile             |    3 ++-
> > > >    ui/vnc.c                   |    2 +-
> > > >    vl.c                       |    4 ++++
> > > >    14 files changed, 100 insertions(+), 57 deletions(-)
> > > >    create mode 100644 include/qapi/qmp/qevent.h
> > > Patches 1-4 look good.  I'm not sure of the advantage of the last four,
> > > however.  The ugly part of monitor_protocol_event is not really the
> > > stub, but the dependency on QObject.
> > I think replacing QObject with QAPI types is another issue we could
> > improve. The last
> > foure patches tries decouple monitor code with block layer code from
> > build time.
> > The files in ./stubs is needed since some symbols are needed in some
> > program which don't
> > need it really, and I think, that tips the code are not organized very
> > well in .c file level.
> That may very well be the case.  However, picking a function, replacing
> it with a function pointer, and throwing it into a struct, will not
> improve the structure at the .c file level very much.
>
> It does not abstract anything.  In order to provide a useful
> abstractions, the questions to answer are:
>
> 1) If a library wanted to provide a callback for events, would the
> current design (using QObject) be the right thing to do?  (If you change
> the design, it might happen that the stub goes away naturally and that
  I think it would not goes away even QAPI is used. The ./stubs is a symbol
issue at link time, so whenever block layer use a symbol belong to 
monitor component,
the stubs would be needed.
  In my opinion, there could be two steps: remove this symbol in block 
code,
replace it with QAPI in monitor code.

> the work in these patches does nothing except obscure the history).
>
> 2) Are there other services that the block layer could desire from the
> monitor?
  I have a draft makefile which link the core block code, it
shows that only event and error printf(include mon_is_qmp) service
are used by block code now. Those symbols basically are listed in ./stubs.

> 3) Could any tool (e.g. qemu-io) desire to implement
> monitor_protocol_event?  If so, what other services could it provide
> that the QEMU monitor provides?
  It seems tools do not need event service now, they link with stubs.

> > After removing ./stubs, the code will be more clear
> If done in the right way, yes.  If done in the wrong way, the code will
> be less clear.
>
> Paolo