On Fri, Jan 15, 2021 at 01:19:01PM -0500, Jag Raman wrote: > > > > On Jan 15, 2021, at 4:20 AM, Stefan Hajnoczi <stefa...@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é <berra...@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 <stefa...@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