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

[Jag Raman]

> 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?

Thank you!
--
Jag

> 
> Regards,
> Daniel
> -- 
> |: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org         -o-            https://fstop138.berrange.com :|
> |: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
>