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

[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
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?

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?

> 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