Hello Daniel, Thanks for the feedback! On Fri, Nov 12, 2021 at 7:13 AM Daniel P. Berrangé <berra...@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-leob...@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
