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