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

[Stefan Hajnoczi]

On Fri, Jan 15, 2021 at 01:19:01PM -0500, Jag Raman wrote:
> 
> 
> > On Jan 15, 2021, at 4:20 AM, Stefan Hajnoczi <stefanha@redhat.com> wrote:
> > 
> > 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
> 
> We are working on sending the next revision out. We could handle the
> fds-only case by altering the while loop condition to be:
> ((nlocal_iov > 0) || local_fds)
> 
> For reference, we would need to handle the following cases:
> len < 0; !partial, !*nfds       => ret = -1;
> len = 0; !partial, !*nfds       => ret = 0;
> len < 0; partial, !*nfds        => ret = -1; errmsg;
> len = 0; partial, !*nfds        => ret = -1; errmsg;
> len < 0; partial, *nfds         => ret = -1; errmsg, clearfds
> len < 0; !partial, *nfds        => ret = -1; errmsg, clearfds
> len = 0; partial, *nfds         => ret = -1; errmsg, clearfds
> len = 0; !partial, *nfds        => ret = -1; errmsg, clearfds
> len = 0; !niov; (nfds && *nfds) => ret = 1 /* fds-only */
> len > 0                         => ret 1

Yes, I think that looks right.

Stefan

signature.asc
Description: PGP signature