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

[Jag Raman]

> 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

Thank you!
--
Jag