Re: [PATCH v19 08/20] io: add qio_channel_readv_full_all_eof & qio_channel_readv_full_all helpers

[Stefan Hajnoczi]

On Thu, Jan 14, 2021 at 01:24:37PM -0500, Jag Raman wrote:
> 
> 
> > On Jan 14, 2021, at 1:00 PM, Daniel P. Berrangé <berrange@redhat.com> wrote:
> > 
> > On Thu, Jan 14, 2021 at 12:55:58PM -0500, Jag Raman wrote:
> >> 
> >> 
> >>> On Jan 14, 2021, at 11:27 AM, Stefan Hajnoczi <stefanha@redhat.com> wrote:
> >>> 
> >>> On Thu, Jan 14, 2021 at 10:40:03AM -0500, Jagannathan Raman wrote:
> >>>> +int qio_channel_readv_full_all(QIOChannel *ioc,
> >>>> +                               const struct iovec *iov,
> >>>> +                               size_t niov,
> >>>> +                               int **fds, size_t *nfds,
> >>>> +                               Error **errp)
> >>>> {
> >>>> -    int ret = qio_channel_readv_all_eof(ioc, iov, niov, errp);
> >>>> +    int ret = qio_channel_readv_full_all_eof(ioc, iov, niov, fds, nfds, 
> >>>> errp);
> >>>> 
> >>>>    if (ret == 0) {
> >>>> -        ret = -1;
> >>>>        error_setg(errp,
> >>>>                   "Unexpected end-of-file before all bytes were read");
> >>> 
> >>> qio_channel_readv_full_all_eof() can read file descriptors but no data
> >>> and return 0.
> >>> 
> >>> Here that case is converted into an error and the file descriptors
> >>> aren't closed, freed, and fds/nfds isn't cleared.
> >> 
> >> That’s a valid point. I’m wondering if the fix for this case should be in
> >> qio_channel_readv_full_all_eof(), instead of here.
> >> 
> >> qio_channel_readv_full_all_eof() should probably return error (-1) if the
> >> amount of data read does not match iov_size(). If the caller is only 
> >> expecting
> >> to read fds, and not any data, it would indicate that by setting iov to 
> >> NULL
> >> and/or setting niov=0. If the caller is setting these parameters, it means 
> >> it is
> >> expecting data.Does that sound good?
> > 
> > The API spec for the existing _eof() methods says:
> > 
> > * The function will wait for all requested data
> > * to be read, yielding from the current coroutine
> > * if required.
> > *
> > * If end-of-file occurs before any data is read,
> > * no error is reported; otherwise, if it occurs
> > * before all requested data has been read, an error
> > * will be reported.
> > 
> > 
> > IOW, return '0' is *only* valid if we've not read anything. I consider
> > file descriptors to be something.
> > 
> > IOW, qio_channel_readv_full_all_eof must only return 0, if it didn't
> > read any data and also didn't receive any file descriptors. So yeah,
> > we must return -1 in the scenario Stefan describes
> 
> That makes sense to me. Reading “fds" is something, which is different
> from our previous understanding. I thought data only meant iov, and not fds.
> 
> So the return values for qio_channel_readv_full_all_eof() would be:
>   - ‘0’ only if EOF is reached without reading any fds and data.
>   - ‘1’ if all data that the caller expects are read (even if the caller reads
>     fds exclusively, without any iovs)
>   - ‘-1’ otherwise, considered as error
> 
> qio_channel_readv_full_all() would return:
>   - ‘0’ if all the data that caller expects are read
>   - ‘-1’ otherwise, considered as error
> 
> Hey Stefan,
> 
>     Does this sound good to you?

The while (nlocal_iov > 0) loop only runs if the caller has requested to
read at least some data, so the fds-only case doesn't work yet.

This suggests that no current QEMU code relies on the fds-only case.
Therefore you could change the doc comment to clarify this instead of
adding support for this case to the code.

But if you would to fully support the fds-only case that would be even
better.

Stefan

signature.asc
Description: PGP signature