Re: [PATCH v5 1/6] QIOChannel: Add io_writev_zerocopy & io_flush_zerocopy callbacks

[Leonardo Bras Soares Passos]

Hello Daniel,
Thanks for the feedback!

On Fri, Nov 12, 2021 at 7:13 AM Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> On Fri, Nov 12, 2021 at 02:10:36AM -0300, Leonardo Bras wrote:
> > -int qio_channel_writev_all(QIOChannel *ioc,
> > -                           const struct iovec *iov,
> > -                           size_t niov,
> > -                           Error **erp);
> > +int qio_channel_writev_all_flags(QIOChannel *ioc,
> > +                                 const struct iovec *iov,
> > +                                 size_t niov,
> > +                                 int flags,
> > +                                 Error **errp);
> > +#define qio_channel_writev_all(ioc, iov, niov, errp) \
> > +    qio_channel_writev_all_flags(ioc, iov, niov, 0, errp)
>
> We already have separate methods for zerocopy, instead of adding
> flags, so we shouldn't add flags to this either.
>
> Add a qio_channel_writev_zerocopy_all method instead.
>
> Internally, we can still make both qio_channel_writev_zerocopy_all
> and qio_channel_writev_all use the same helper method, just don't
> expose flags in the public API. Even internally we don't really
> need flags, just a bool

I see.
The idea of having a flag was to make it easier to expand the
interface in the future.
I got some feedback on v1 that would suggest it would be desired:
http://patchwork.ozlabs.org/project/qemu-devel/patch/20210831110238.299458-2-leobras@redhat.com/


>
[...]
> > +#define qio_channel_writev_full_all(ioc, iov, niov, fds, nfds, errp) \
> > +    qio_channel_writev_full_all_flags(ioc, iov, niov, fds, nfds, 0, errp)
>
> There's no need for this at all. Since fd passing is not supported
> with zerocopy, there's no reason to ever use this method.
>
> > +/**
> > + * qio_channel_writev_zerocopy:
> > + * @ioc: the channel object
> > + * @iov: the array of memory regions to write data from
> > + * @niov: the length of the @iov array
> > + * @errp: pointer to a NULL-initialized error object
> > + *
> > + * Behaves like qio_channel_writev_full_all_flags, but may write
>
> qio_channel_writev
>
> > + * data asynchronously while avoiding unnecessary data copy.
> > + * This function may return before any data is actually written,
> > + * but should queue every buffer for writing.
>
> Callers mustn't rely on "should" docs - they must rely on the
> return value indicating how many bytes were accepted.
>
> Also mention that this requires locked memory and can/will fail if
> insufficient locked memory is available.
>

Sure, I will update that.

> > +/**
> > + * qio_channel_flush_zerocopy:
> > + * @ioc: the channel object
> > + * @errp: pointer to a NULL-initialized error object
> > + *
> > + * Will block until every packet queued with
> > + * qio_channel_writev_zerocopy() is sent, or return
> > + * in case of any error.
> > + *
> > + * Returns -1 if any error is found, 0 otherwise.
>
>   Returns -1 if any error is found, 0 if all data was sent,
>            or 1 if all data was sent but at least some was copied.
>

I don't really get the return 1 part, I mean, per description it will
'block until every queued packet was sent, so "at least some was
copied" doesn't seem to fit here.
Could you elaborate?

Best regards,
Leo