Re: [Qemu-devel] [PATCH v2 00/16] Introduce I/O channels framework

[Daniel P. Berrange]

Ping, does anyone feel like doing a review of this series, if not I'll
just do a pull request as-is...

Daniel

On Mon, Oct 12, 2015 at 12:14:53PM +0100, Daniel P. Berrange wrote:
> This series of patches is a followup submission for review of another
> chunk of my large series supporting TLS across chardevs, VNC and
> migration, previously shown here:
> 
>  RFC: https://lists.gnu.org/archive/html/qemu-devel/2015-09/msg00829.html
>   v1: https://lists.gnu.org/archive/html/qemu-devel/2015-09/msg04853.html
> 
> In this series, I have provided only the general I/O channels
> framework, which will ultimately be used by VNC, chardev and
> migration code, whicih currently work as follows:
> 
>  - VNC: uses raw sockets APIs directly, layering in TLS, SASL
>    and websockets where needed. This has resulted in what should
>    be fairly generic code for TLS and websockets being entangled
>    with the rest of the VNC server code.
> 
>  - Chardevs: uses GLib's GIOChannel framework. This only provides
>    implementations for sockets and files. Its API lacks support for
>    using struct iovec for read/writes, file descriptor passing,
>    misc sockets ioctls/fcntls. While you can work around these
>    problems by accessing the underling file descriptor directly,
>    this breaks the encapsulation, requiring callers to know about
>    specific implementations. It also does not have integration
>    with QEMU Error reporting framework. So while the GIOChannel
>    is extensible, extending it would result in throwing away
>    pretty much the entire of the existing implementations
> 
>  - Migration: uses QEMUFile framework. The provides an abstract
>    interface for I/O channels used during migration. It has
>    impls for sockets, files, memory buffers & commands. While
>    it has struct iovec support for writes, it does not have
>    the same for reads. It also doesn't support file descriptor
>    passing or control of the sockets ioctls/fcntls. It also
>    does not have any explicit event loop integration, expecting
>    the callers to directly access the underling file desctriptor
>    and setup events on that. This limits its utility in cases
>    where you need channels which are not backed by file
>    descriptors. It has no integration with QEMU Error object
>    for error reporting, has fixed semantics wrt writes
>    ie must always do a full write, no partial writes, and
>    most strangely forces either input or output mode, but
>    refuses to allow both, so no bi-directional channels!
> 
> Out of all of these, the migration code is probably closest
> to what is needed, but is still a good way off from being a
> generic framework that be can reused outside of the migration
> code.
> 
> There is also the GLib GIO library which provides a generic
> framework, but we can't depend on that due to the minimum
> GLib requirement. It also has various missing pieces such as
> file descriptor passing, and no support for struct iovec
> either.
> 
> Hence, this series was born, which tries to take the best of
> the designs for the GIOChannel, QIO and QEMUFile code to
> provide QIOChannel. Right from the start this new framework
> is explicitly isolated from any other QEMU subsystem, so its
> implementation will not get mixed up with specific use cases.
> 
> The QIOChannel abstract base class defines the overall API
> contract
> 
>  - qio_channel_{write,writev,write_full} for writing data. The
>    underling impl always uses struct iovec, but non-iov
>    APIs are provided as simple wrappers for convenience
> 
>  - qio_channel_{read,readv,read_full} for reading data. The
>    underling impl always uses struct iovec, but non-iov
>    APIs are provided as simple wrappers for convenience
> 
>  - qio_channel_has_feature to determine which optional
>    features the channel supports - eg file descriptor
>    passing, nagle, etc
> 
>  - qio_channel_set_{blocking,delay,cork} for various fcntl
>    and ioctl controls
> 
>  - qio_channel_{close,shutdown} for closing the I/O stream
>    or individual channels
> 
>  - qio_channel_seek for random access channels
> 
>  - qio_channel_{add,create}_watch for integration into the
>    main loop for event notifications
> 
>  - qio_channel_wait for blocking of execution pending an
>    event notification
> 
>  - qio_channel_yield for switching coroutine until an
>    event notification
> 
> All the APIs support Error ** object arguments where needed.
> The object is built using QOM, in order to get reference
> counting and sub-classing with type checking. They are *not*
> user creatable/visible objects though - these are internal
> infrastructure, so we will be free to adapt APIs/objects at
> will over time.
> 
> In this series there are a variety of implementations. Some
> of them provide an actual base layer data transport, while
> others provide a data translation layer:
> 
> In this series there are a variety of implementations. Some
> of them provide an actual base layer data transport, while
> others provide a data translation layer:
> 
>  - QIOChannelSocket - for socket based I/O, IPv4, IPV6 & UNIX,
>                       both stream and datagram.
>  - QIOChannelFile - any non-socket file, plain file, char dev,
>                     fifo, pipe, etc
>  - QIOChannelTLS - data translation layer to apply TLS protocol
>  - QIOChannelWebsock - data translation layer to apply the
>                        websockets server protocol
>  - QIOChannelCommand - spawn & communicate with a command via
>                        pipes
>  - QIOChannelBuffer - a simple in-memory buffer
> 
> These 6 implementations are sufficient to support the current
> needs of the VNC server, chardev network backends, and all
> the various migration protocols, except RDMA (which is part
> of the larger RFC series I've posted previously).
> 
> The sockets implementation in particular solves a long standing
> limitation of the qemu-sockets.c API by providing a truely
> asynchronous API for establishing listener sockets / connections.
> The current code is only async for the socket establishment,
> but still blocks the caller for DNS resolution.
> 
> I have not attempted to make the socket implementation support
> multiple listener sockets in a single object. I looked at this
> but it would significantly complicate the QIOChannelSocket
> implementation. I intend to suggest an alternative approach
> for that problem at a later date, whereby we define a
> QIONetworkService object, which provides the infrastructure
> for managing multiple QIOChannelSocket listeners, and clients,
> which was resuable for any QEMU network service.
> 
> There are unit tests for the socket, file, tls, and command
> channel implementations. I didn't create unit test for the
> websock impl, as it needs a websock client to usefully test
> it which doesn't exist in QEMU yet. The memory buffer impl
> also lacks unit tests for now, simply due to not getting
> around to it yet.
> 
> This series merely introduces all the new framework. To keep
> it an acceptable length for review, it doesn't include any of
> the patches which actually use the new APIs. If reviewers want
> to see real world usage of these APIs, they can refer to my
> previous RFC series.
> 
>   https://lists.gnu.org/archive/html/qemu-devel/2015-09/msg00829.html
> 
> In particular
> 
>  - VNC conversion to QIOChannelSocket
>      https://lists.gnu.org/archive/html/qemu-devel/2015-09/msg00851.html
> 
>  - VNC conversion to QIOChannelTLS
>      https://lists.gnu.org/archive/html/qemu-devel/2015-09/msg00845.html
> 
>  - VNC conversion to QIOChannelWebsock
>      https://lists.gnu.org/archive/html/qemu-devel/2015-09/msg00842.html
> 
>  - Chardev conversion to QIOChannelSocket
>      https://lists.gnu.org/archive/html/qemu-devel/2015-09/msg00849.html
> 
>  - Chardev addition of TLS support
>      https://lists.gnu.org/archive/html/qemu-devel/2015-09/msg00848.html
> 
>  - All the migration ones, but most interesting addition of TLS
>      https://lists.gnu.org/archive/html/qemu-devel/2015-09/msg00871.html
> 
> Finally I've listed myself as maintainer for the new io/ and
> include/io/ directory prefixes.
> 
> 
> Changed in v2:
> 
>  - Put the Buffer APIs extracted from VNC code into util/ instead
>    of io/ directories (Paolo)
> 
>  - Added Kevin & Stefan to MAINTAINERS for coroutine code (Paolo)
> 
>  - Remove feature checks for TCP_NODELAY and TCP_CORK and
>    treat them as hints which cannot fail (Paolo)
> 
>  - Push feature checking for FD passing up into QIOChannel
>    base class (Paolo)
> 
>  - Use more constants in websockets code instead of magic
>    values (David Gilbert)
> 
> Daniel P. Berrange (16):
>   sockets: add helpers for creating SocketAddress from a socket
>   sockets: move qapi_copy_SocketAddress into qemu-sockets.c
>   sockets: allow port to be NULL when listening on IP address
>   ui: convert VNC startup code to use SocketAddress
>   osdep: add qemu_fork() wrapper for safely handling signals
>   coroutine: move into libqemuutil.a library
>   util: pull Buffer code out of VNC module
>   io: add abstract QIOChannel classes
>   io: add helper module for creating watches on FDs
>   io: add QIOTask class for async operations
>   io: add QIOChannelSocket class
>   io: add QIOChannelFile class
>   io: add QIOChannelTLS class
>   io: add QIOChannelWebsock class
>   io: add QIOChannelCommand class
>   io: add QIOChannelBuffer class
> 
>  MAINTAINERS                                        |  20 +
>  Makefile                                           |   2 +
>  Makefile.objs                                      |   9 +-
>  Makefile.target                                    |   2 +
>  block.c                                            |   2 +-
>  block/qcow2.h                                      |   2 +-
>  block/vdi.c                                        |   2 +-
>  block/write-threshold.c                            |   2 +-
>  blockjob.c                                         |   2 +-
>  configure                                          |  11 +
>  hw/9pfs/codir.c                                    |   2 +-
>  hw/9pfs/cofile.c                                   |   2 +-
>  hw/9pfs/cofs.c                                     |   2 +-
>  hw/9pfs/coxattr.c                                  |   2 +-
>  hw/9pfs/virtio-9p-coth.c                           |   2 +-
>  hw/9pfs/virtio-9p-coth.h                           |   2 +-
>  hw/9pfs/virtio-9p.h                                |   2 +-
>  include/block/block.h                              |   2 +-
>  include/block/block_int.h                          |   2 +-
>  include/io/channel-buffer.h                        |  59 ++
>  include/io/channel-command.h                       |  91 ++
>  include/io/channel-file.h                          |  93 ++
>  include/io/channel-socket.h                        | 244 ++++++
>  include/io/channel-tls.h                           | 142 +++
>  include/io/channel-watch.h                         |  72 ++
>  include/io/channel-websock.h                       | 108 +++
>  include/io/channel.h                               | 506 +++++++++++
>  include/io/task.h                                  | 256 ++++++
>  include/qemu/buffer.h                              | 118 +++
>  include/{block => qemu}/coroutine.h                |   0
>  include/{block => qemu}/coroutine_int.h            |   2 +-
>  include/qemu/osdep.h                               |  16 +
>  include/qemu/sockets.h                             |  34 +
>  io/Makefile.objs                                   |   9 +
>  io/channel-buffer.c                                | 255 ++++++
>  io/channel-command.c                               | 369 ++++++++
>  io/channel-file.c                                  | 237 +++++
>  io/channel-socket.c                                | 737 ++++++++++++++++
>  io/channel-tls.c                                   | 405 +++++++++
>  io/channel-watch.c                                 | 200 +++++
>  io/channel-websock.c                               | 975 
> +++++++++++++++++++++
>  io/channel.c                                       | 283 ++++++
>  io/task.c                                          | 159 ++++
>  migration/qemu-file-buf.c                          |   2 +-
>  migration/qemu-file-stdio.c                        |   2 +-
>  migration/qemu-file-unix.c                         |   2 +-
>  migration/qemu-file.c                              |   2 +-
>  migration/rdma.c                                   |   2 +-
>  nbd.c                                              |   2 +-
>  qemu-char.c                                        |  25 -
>  scripts/create_config                              |   9 +
>  tests/.gitignore                                   |   7 +
>  tests/Makefile                                     |  16 +
>  tests/io-channel-helpers.c                         | 247 ++++++
>  tests/io-channel-helpers.h                         |  33 +
>  tests/test-coroutine.c                             |   4 +-
>  tests/test-io-channel-command.c                    | 122 +++
>  tests/test-io-channel-file.c                       |  91 ++
>  tests/test-io-channel-socket.c                     | 367 ++++++++
>  tests/test-io-channel-tls.c                        | 335 +++++++
>  tests/test-io-task.c                               | 276 ++++++
>  tests/test-vmstate.c                               |   2 +-
>  thread-pool.c                                      |   2 +-
>  trace-events                                       |  56 ++
>  ui/vnc.c                                           | 204 ++---
>  ui/vnc.h                                           |  16 +-
>  util/Makefile.objs                                 |   4 +
>  util/buffer.c                                      |  65 ++
>  coroutine-gthread.c => util/coroutine-gthread.c    |   2 +-
>  .../coroutine-sigaltstack.c                        |   2 +-
>  coroutine-ucontext.c => util/coroutine-ucontext.c  |   2 +-
>  coroutine-win32.c => util/coroutine-win32.c        |   2 +-
>  util/oslib-posix.c                                 |  71 ++
>  util/oslib-win32.c                                 |   9 +
>  qemu-coroutine-io.c => util/qemu-coroutine-io.c    |   2 +-
>  .../qemu-coroutine-lock.c                          |   4 +-
>  .../qemu-coroutine-sleep.c                         |   2 +-
>  qemu-coroutine.c => util/qemu-coroutine.c          |   4 +-
>  util/qemu-sockets.c                                | 158 +++-
>  79 files changed, 7396 insertions(+), 197 deletions(-)
>  create mode 100644 include/io/channel-buffer.h
>  create mode 100644 include/io/channel-command.h
>  create mode 100644 include/io/channel-file.h
>  create mode 100644 include/io/channel-socket.h
>  create mode 100644 include/io/channel-tls.h
>  create mode 100644 include/io/channel-watch.h
>  create mode 100644 include/io/channel-websock.h
>  create mode 100644 include/io/channel.h
>  create mode 100644 include/io/task.h
>  create mode 100644 include/qemu/buffer.h
>  rename include/{block => qemu}/coroutine.h (100%)
>  rename include/{block => qemu}/coroutine_int.h (98%)
>  create mode 100644 io/Makefile.objs
>  create mode 100644 io/channel-buffer.c
>  create mode 100644 io/channel-command.c
>  create mode 100644 io/channel-file.c
>  create mode 100644 io/channel-socket.c
>  create mode 100644 io/channel-tls.c
>  create mode 100644 io/channel-watch.c
>  create mode 100644 io/channel-websock.c
>  create mode 100644 io/channel.c
>  create mode 100644 io/task.c
>  create mode 100644 tests/io-channel-helpers.c
>  create mode 100644 tests/io-channel-helpers.h
>  create mode 100644 tests/test-io-channel-command.c
>  create mode 100644 tests/test-io-channel-file.c
>  create mode 100644 tests/test-io-channel-socket.c
>  create mode 100644 tests/test-io-channel-tls.c
>  create mode 100644 tests/test-io-task.c
>  create mode 100644 util/buffer.c
>  rename coroutine-gthread.c => util/coroutine-gthread.c (99%)
>  rename coroutine-sigaltstack.c => util/coroutine-sigaltstack.c (99%)
>  rename coroutine-ucontext.c => util/coroutine-ucontext.c (99%)
>  rename coroutine-win32.c => util/coroutine-win32.c (98%)
>  rename qemu-coroutine-io.c => util/qemu-coroutine-io.c (99%)
>  rename qemu-coroutine-lock.c => util/qemu-coroutine-lock.c (98%)
>  rename qemu-coroutine-sleep.c => util/qemu-coroutine-sleep.c (96%)
>  rename qemu-coroutine.c => util/qemu-coroutine.c (98%)
> 
> -- 
> 2.4.3
> 

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|