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

[Wenchao Xia <xiawenc@linux.vnet.ibm.com>]

于 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
>