Re: [Qemu-devel] [PATCH v2 08/16] io: add abstract QIOChannel classes

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 5566 bytes --]

On 10/12/2015 05:15 AM, Daniel P. Berrange wrote:
> Start the new generic I/O channel framework by defining a
> QIOChannel abstract base class. This is designed to feel
> similar to GLib's GIOChannel, but with the addition of
> support for using iovecs, qemu error reporting, file
> descriptor passing, coroutine integration and use of
> the QOM framework for easier sub-classing.
> 
> The intention is that anywhere in QEMU that almost
> anywhere that deals with sockets will use this new I/O
> infrastructure, so that it becomes trivial to then layer
> in support for TLS encryption. This will at least include
> the VNC server, char device backend and migration code.
> 
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
>  MAINTAINERS          |   7 +
>  Makefile             |   2 +
>  Makefile.objs        |   5 +
>  Makefile.target      |   2 +
>  include/io/channel.h | 506 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  io/Makefile.objs     |   1 +
>  io/channel.c         | 283 ++++++++++++++++++++++++++++
>  7 files changed, 806 insertions(+)
>  create mode 100644 include/io/channel.h
>  create mode 100644 io/Makefile.objs
>  create mode 100644 io/channel.c
> 

> +++ b/include/io/channel.h
> @@ -0,0 +1,506 @@

> +struct QIOChannel {
> +    Object parent;
> +    int features; /* bitmask of QIOChannelFeatures */

Would an unsigned type make bit manipulations any less likely to avoid
clang sanitizer warnings under future extensions of new bits?

> +};
> +
> +/**
> + * QIOChannelClass:
> + *
> + * This class defines the contract that all subclasses
> + * must follow to provide specific channel implementations.
> + * All the callbacks are mandatory with the exception of
> + * io_has_feature, which defaults to returning false.

And yet...

> + *
> + * Consult the corresponding public API docs for a description
> + * of the semantics of each callback
> + */
> +struct QIOChannelClass {
> +    ObjectClass parent;
> +
> +    /* Mandatory callbacks */
...
> +
> +    /* Optional callbacks */
> +    int (*io_shutdown)(QIOChannel *ioc,
> +                       QIOChannelShutdown how,
> +                       Error **errp);
> +    void (*io_set_cork)(QIOChannel *ioc,
> +                        bool enabled);
> +    void (*io_set_delay)(QIOChannel *ioc,
> +                         bool enabled);
> +    off64_t (*io_seek)(QIOChannel *ioc,
> +                       off64_t offset,
> +                       int whence,
> +                       Error **errp);

...there are four optional callbacks listed, and no mention of a
callback named io_has_feature.  Sounds like docs need an update.


> +
> +/**
> + * qio_channel_has_feature:
> + * @ioc: the channel object
> + * @feature: the feature to check support of
> + *
> + * Determine whether the channel implementation supports
> + * the optional feature named in @feature.
> + *
> + * Returns: true if supported, false otherwise.
> + */
> +bool qio_channel_has_feature(QIOChannel *ioc,
> +                             QIOChannelFeature feature);

Can this be used to check multiple features at once, or are we content
that checking two features requires two calls?


> +/**
> + * qio_channel_seek:
> + * @ioc: the channel object
> + * @offset: the position to seek to, relative to @whence
> + * @whence: one of the POSIX SEEK_* constants

Including SEEK_HOLE/SEEK_DATA?

> + * @errp: pointer to an uninitialized error object
> + *
> + * Moves the current I/O position within the channel
> + * @ioc, to be @offset. The value of @offset is
> + * interpreted relative to @whence:
> + *
> + * SEEK_SET - the position is set to @offset bytes
> + * SEEK_CUR - the position is moved by @offset bytes
> + * SEEK_END - the position is set to end of the file plus @offset bytes
> + *
> + * Not all implementations will support this facility,
> + * so may report an error. To avoid errors, the
> + * caller may check for the feature flag
> + * QIO_CHANNEL_FEATURE_SEEKABLE prior to calling
> + * this method.

I don't see that constant defined; stale docs?

> +++ b/io/channel.c
> @@ -0,0 +1,283 @@

> +ssize_t qio_channel_readv_full(QIOChannel *ioc,
> +                               const struct iovec *iov,
> +                               size_t niov,
> +                               int **fds,
> +                               size_t *nfds,
> +                               Error **errp)
> +{
> +    QIOChannelClass *klass = QIO_CHANNEL_GET_CLASS(ioc);
> +
> +    if ((fds || nfds) &&
> +        !(ioc->features & (1 << QIO_CHANNEL_FEATURE_FD_PASS))) {
> +        error_setg_errno(errp, EINVAL,
> +                         "Channel does not support file descriptor passing");
> +        return -1;
> +    }

Why pre-filter fds here...

> +
> +    return klass->io_readv(ioc, iov, niov, fds, nfds, errp);
> +}
> +
> +
> +ssize_t qio_channel_writev_full(QIOChannel *ioc,
> +                                const struct iovec *iov,
> +                                size_t niov,
> +                                int *fds,
> +                                size_t nfds,
> +                                Error **errp)
> +{
> +    QIOChannelClass *klass = QIO_CHANNEL_GET_CLASS(ioc);
> +    return klass->io_writev(ioc, iov, niov, fds, nfds, errp);

...but rely on the callbacks to filter fds here?

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]