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

["Daniel P. Berrange" <berrange@redhat.com>]

On Tue, Oct 20, 2015 at 11:48:03AM -0600, Eric Blake wrote:
> 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?

Yes, makes sense.


> > + * 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.

Opps, yes, I removed io_has_feature after last round of review
feedback.

> > +
> > +/**
> > + * 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?

The expected usage of this is such that I don't really expect
callers to need to check multiple features at once.

> > +/**
> > + * 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?

Are those actually POSIX, or Linux extensions ?  In any case,
only SEEK_SET, SEEK_CUR and SEEK_END are intended for use.
I'll clarify the docs.

> 
> > + * @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?

Hmm, yes, its not defined - I can't remember now if i decided
it was not needed after all, or if i just forgot about it...
Will investigate & fix one way or the other.

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

That's an oversight.


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