On Tue, Jan 18, 2022 at 05:45:09PM -0300, Leonardo Bras Soares Passos wrote:
> Hello Peter,
>
> On Thu, Jan 13, 2022 at 3:28 AM Peter Xu <pet...@redhat.com> wrote:
> >
> > On Thu, Jan 06, 2022 at 07:13:38PM -0300, Leonardo Bras wrote:
> > > diff --git a/io/channel.c b/io/channel.c
> > > index e8b019dc36..904855e16e 100644
> > > --- a/io/channel.c
> > > +++ b/io/channel.c
> > > @@ -67,12 +67,13 @@ ssize_t qio_channel_readv_full(QIOChannel *ioc,
> > > }
> > >
> > >
> > > -ssize_t qio_channel_writev_full(QIOChannel *ioc,
> > > - const struct iovec *iov,
> > > - size_t niov,
> > > - int *fds,
> > > - size_t nfds,
> > > - Error **errp)
> > > +ssize_t qio_channel_writev_full_flags(QIOChannel *ioc,
> > > + const struct iovec *iov,
> > > + size_t niov,
> > > + int *fds,
> > > + size_t nfds,
> > > + int flags,
> > > + Error **errp)
> > > {
> > > QIOChannelClass *klass = QIO_CHANNEL_GET_CLASS(ioc);
> > >
> > > @@ -83,7 +84,7 @@ ssize_t qio_channel_writev_full(QIOChannel *ioc,
> > > return -1;
> > > }
> >
> > Should we better also check QIO_CHANNEL_FEATURE_WRITE_ZERO_COPY here when
> > QIO_CHANNEL_WRITE_FLAG_ZERO_COPY is set? Just like what we do with:
>
> Yes, that's correct.
> I will also test for fds + zerocopy_flag , which should also fail here.
>
> >
> > if ((fds || nfds) &&
> > !qio_channel_has_feature(ioc, QIO_CHANNEL_FEATURE_FD_PASS)) {
> > error_setg_errno(errp, EINVAL,
> > "Channel does not support file descriptor
> > passing");
> > return -1;
> > }
> >
> > I still think it's better to have the caller be crystal clear when to use
> > zero_copy feature because it has implication on buffer lifetime.
>
> I don't disagree with that suggestion.
>
> But the buffer lifetime limitation is something on the socket
> implementation, right?
> There could be some synchronous zerocopy implementation that does not
> require flush, and thus
> don't require the buffer to be treated any special. Or am I missing something?
Currently the flush() is required for zerocopy and not required for all the
existing non-zerocopy use cases, that's already an API difference so the caller
needs to identify it anyway. Then I think it's simpler we expose all of it to
the user.
Not to mention IIUC if we don't fail here, it will just fail later when the
code will unconditionally convert the flags=ZEROCOPY into MSG_ZEROCOPY in your
next patch:
if (flags & QIO_CHANNEL_WRITE_FLAG_ZERO_COPY) {
sflags = MSG_ZEROCOPY;
}
So AFAIU it'll fail anyway, either here with the cap check I mentioned, or
later in sendmsg().
IOW, I think it fails cleaner here, rather than reaching sendmsg().
>
> >
> > I might have commented similar things before, but I have missed a few
> > versions
> > so I could also have missed some previous discussions..
> >
>
> That's all great suggestions Peter! Thanks for that!
>
> Some of the previous suggestions may have been missed because a lot of
> code moved.
> Sorry about that.
Not a problem at all, I just want to make sure my question still makes
sense. :)
--
Peter Xu
