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

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

于 2013/9/16 22:27, 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
>>
>
>

Hi Paolo,
   This series tries to find a mechanism, which can avoid link with unused
symbols. Assume QObject was replaced with other design, such as QAPI, it
will still call monitor funcions to emit the event, so it will still require
resolving that function in linking of qemu-img.
   Do you have some suggestion for it?