Hello Daniel, thanks for reviewing! On Tue, Feb 1, 2022 at 6:35 AM Daniel P. Berrangé <berra...@redhat.com> wrote: > > On Tue, Feb 01, 2022 at 03:28:59AM -0300, Leonardo Bras wrote: > > Add flags to io_writev and introduce io_flush as optional callback to > > QIOChannelClass, allowing the implementation of zero copy writes by > > subclasses. > > > > How to use them: > > - Write data using > > qio_channel_writev*(...,QIO_CHANNEL_WRITE_FLAG_ZERO_COPY), > > - Wait write completion with qio_channel_flush(). > > > > Notes: > > As some zero copy write implementations work asynchronously, it's > > recommended to keep the write buffer untouched until the return of > > qio_channel_flush(), to avoid the risk of sending an updated buffer > > instead of the buffer state during write. > > > > As io_flush callback is optional, if a subclass does not implement it, then: > > - io_flush will return 0 without changing anything. > > > > Also, some functions like qio_channel_writev_full_all() were adapted to > > receive a flag parameter. That allows shared code between zero copy and > > non-zero copy writev, and also an easier implementation on new flags. > > > > Signed-off-by: Leonardo Bras <leob...@redhat.com> > > --- > > include/io/channel.h | 38 ++++++++++++++++++++- > > chardev/char-io.c | 2 +- > > hw/remote/mpqemu-link.c | 2 +- > > io/channel-buffer.c | 1 + > > io/channel-command.c | 1 + > > io/channel-file.c | 1 + > > io/channel-socket.c | 2 ++ > > io/channel-tls.c | 1 + > > io/channel-websock.c | 1 + > > io/channel.c | 53 +++++++++++++++++++++++------ > > migration/rdma.c | 1 + > > scsi/pr-manager-helper.c | 2 +- > > tests/unit/test-io-channel-socket.c | 1 + > > 13 files changed, 92 insertions(+), 14 deletions(-) > > > > diff --git a/io/channel.c b/io/channel.c > > index e8b019dc36..b8b99fdc4c 100644 > > --- a/io/channel.c > > +++ b/io/channel.c > > @@ -72,18 +72,32 @@ ssize_t qio_channel_writev_full(QIOChannel *ioc, > > size_t niov, > > int *fds, > > size_t nfds, > > + int flags, > > Error **errp) > > { > > QIOChannelClass *klass = QIO_CHANNEL_GET_CLASS(ioc); > > > > - if ((fds || nfds) && > > - !qio_channel_has_feature(ioc, QIO_CHANNEL_FEATURE_FD_PASS)) { > > + if (fds || nfds) { > > + if (!qio_channel_has_feature(ioc, QIO_CHANNEL_FEATURE_FD_PASS)) { > > + error_setg_errno(errp, EINVAL, > > + "Channel does not support file descriptor > > passing"); > > + return -1; > > + } > > + if (flags & QIO_CHANNEL_WRITE_FLAG_ZERO_COPY) { > > + error_setg_errno(errp, EINVAL, > > + "Zero Copy does not support file descriptor > > passing"); > > + return -1; > > + } > > Here you gracefully reject FD passing when zero copy is requested > which is good. > > > + } > > + > > > @@ -235,10 +249,16 @@ int qio_channel_writev_full_all(QIOChannel *ioc, > > iov, niov, > > 0, iov_size(iov, niov)); > > > > + if (flags & QIO_CHANNEL_WRITE_FLAG_ZERO_COPY) { > > + assert(fds == NULL && nfds == 0); > > + } > > But here you abort QEMU if FD passing is requested when zero copy > is set. > > AFAICT, if you just delete this assert, the code to gracefully > report errors will do the right thing.
Yeah, thatś right. This test is unnecessary since qio_channel_writev_full() will be called and will return error if fds + zerocopy happens. Good catch! > > Without the assert: > > Reviewed-by: Daniel P. Berrangé <berra...@redhat.com> > Thanks! I will wait for more feedback on other patches before sending the v9, but it should not take too long this time. Best regards, Leo