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 <berra...@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 :|
