Hello Peter, sorry for the delay.
On Wed, Oct 13, 2021 at 3:33 AM Peter Xu <
peterx@redhat.com> wrote:
>
> On Wed, Oct 13, 2021 at 02:07:13PM +0800, Peter Xu wrote:
> > On Sat, Oct 09, 2021 at 04:56:11AM -0300, Leonardo Bras wrote:
> > > -int qio_channel_writev_full_all(QIOChannel *ioc,
> > > - const struct iovec *iov,
> > > - size_t niov,
> > > - int *fds, size_t nfds,
> > > - Error **errp)
> > > +int qio_channel_writev_full_all_flags(QIOChannel *ioc,
> > > + const struct iovec *iov,
> > > + size_t niov,
> > > + int *fds, size_t nfds,
> > > + int flags, Error **errp)
> > > {
> > > int ret = -1;
> > > struct iovec *local_iov = g_new(struct iovec, niov);
> > > @@ -237,8 +250,8 @@ int qio_channel_writev_full_all(QIOChannel *ioc,
> > >
> > > while (nlocal_iov > 0) {
> > > ssize_t len;
> > > - len = qio_channel_writev_full(ioc, local_iov, nlocal_iov, fds, nfds,
> > > - errp);
> > > + len = qio_channel_writev_full_flags(ioc, local_iov, nlocal_iov, fds, nfds,
> > > + flags, errp);
> >
> > IMHO we can keep qio_channel_writev_full() untouched, as it is the wrapper for
> > io_writev() hook right now (and it allows taking fds). Then here instead of
> > adding a new flags into it, we can introduce qio_channel_writev_zerocopy_full()
> > and directly call here:
> >
> > if (flags & ZEROCOPY) {
> > assert(fds == NULL && nfds == 0);
Quick question: Why is this assert needed?
> > qio_channel_writev_zerocopy_full(...[no fds passing in]);
> > } else {
> > qio_channel_writev_full(...[with all parameters]);
> > }
> >
> > Then qio_channel_writev_zerocopy_full() should be simplely the wrapper for the
> > new io_writev_zerocopy() hook.
>
> Sorry I think the name doesn't need to have "_full" - that should be used for
> io_writev() when we need to pass in fds. zerocopy doesn't have that, so I
> think we can just call it qio_channel_writev_zerocopy() as it simply does what
> writev() does.
Ok, I will make these changes to v5.
>
> >
> > > if (len == QIO_CHANNEL_ERR_BLOCK) {
> > > if (qemu_in_coroutine()) {
> > > qio_channel_yield(ioc, G_IO_OUT);
> > > @@ -474,6 +487,31 @@ off_t qio_channel_io_seek(QIOChannel *ioc,
> > > }
> > >
> > >
> > > +ssize_t qio_channel_writev_zerocopy(QIOChannel *ioc,
> > > + const struct iovec *iov,
> > > + size_t niov,
> > > + Error **errp)
> > > +{
> > > + return qio_channel_writev_full_flags(ioc, iov, niov, NULL, 0,
> > > + QIO_CHANNEL_WRITE_FLAG_ZEROCOPY,
> > > + errp);
> > > +}
> >
> > This function is never used, right? qio_channel_writev_all_flags() is used in
> > patch 3, with proper flags passed in. Then IMHO we can drop this one.
> >
> > The rest looks good, as long as with Eric's comment addressed.
> >
> > Thanks,
IIUC, this was addressed by your reply above, is that correct?
> >
> > > +
> > > +
> > > +int qio_channel_flush_zerocopy(QIOChannel *ioc,
> > > + Error **errp)
> > > +{
> > > + QIOChannelClass *klass = QIO_CHANNEL_GET_CLASS(ioc);
> > > +
> > > + if (!klass->io_flush_zerocopy ||
> > > + !qio_channel_has_feature(ioc, QIO_CHANNEL_FEATURE_WRITE_ZEROCOPY)) {
> > > + return 0;
> > > + }
> > > +
> > > + return klass->io_flush_zerocopy(ioc, errp);
> > > +}
> > > +
> > > +
> > > static void qio_channel_restart_read(void *opaque)
> > > {
> > > QIOChannel *ioc = opaque;
> > > --
> > > 2.33.0
> > >
> >
> > --
> > Peter Xu
>
> --
> Peter Xu
>