Re: [PATCH v4 1/3] QIOChannel: Add io_writev_zerocopy & io_flush_zerocopy callbacks

[Eric Blake <eblake@redhat.com>]

On Mon, Oct 11, 2021 at 04:38:23PM -0300, Leonardo Bras Soares Passos wrote:
> > >  /**
> > > - * qio_channel_writev_full:
> > > + * qio_channel_writev_full_flags:
> > >   * @ioc: the channel object
> > >   * @iov: the array of memory regions to write data from
> > >   * @niov: the length of the @iov array
> > >   * @fds: an array of file handles to send
> > >   * @nfds: number of file handles in @fds
> > > + * @flags: write flags (QIO_CHANNEL_WRITE_FLAG_*)
> > >   * @errp: pointer to a NULL-initialized error object
> > >   *
> > >   * Write data to the IO channel, reading it from the
> > > @@ -242,6 +252,10 @@ ssize_t qio_channel_readv_full(QIOChannel *ioc,
> > >   * guaranteed. If the channel is non-blocking and no
> > >   * data can be sent, it will return QIO_CHANNEL_ERR_BLOCK
> > >   *
> > > + * If flag QIO_CHANNEL_WRITE_FLAG_ZEROCOPY is passed,
> > > + * function will return once each buffer was queued for
> > > + * sending.
> >
> > This would be a good place to document the requirement to keep the
> > buffer unchanged until the zerocopy sequence completes.
> 
> That makes sense, even though that may be true for just some implementations,
> it makes sense to document it here.
> 

> 
> Ok,
> Is it enough to document it in a single one of the places suggested, or
> would you recommend documenting it in all suggested places?

Ah, the curse of maintaining copy-and-paste.  If you can find a way to
say "see this other type for limitations" that sounds fine, it avoids
the risk of later edits touching one but not all identical copies.
But our current process for generating sphynx documentation from the
qapi generator does not have cross-referencing abilities that I'm
aware of.  Markus or John, any thoughts?

> >
> > > +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;
> >
> > Matches your documentation, but an ideal app should not be trying to
> > flush if the write failed in the first place.  So wouldn't it be
> > better to return -1 or even abort() on a coding error?
> 
> The point here is that any valid user of zrocopy_flush would have
> already used zerocopy_writev
> at some point, and failed if not supported / enabled.
> 
> Having this not returning error can help the user keep a simpler
> approach when using
> a setup in which the writev can happen in both zerocopy or default behavior.
> 
> I mean, the user will not need to check if zerocopy was or was not
> enabled, and just flush anyway.
> 
> But if it's not good behavior, or you guys think it's a better
> approach to fail here, I can also do that.

Either flush is supposed to be a no-op when zerocopy fails (so
returning 0 is okay), or should not be attempted unless zerocopy
succeeded (in which case abort()ing seems like the best way to point
out the programmer's error).  But I wasn't clear from your
documentation which of the two behaviors you had in mind.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org