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

From: Jag Raman <jag.raman@oracle.com>
To: Stefan Hajnoczi <stefanha@redhat.com>
Cc: "Elena Ufimtseva" <elena.ufimtseva@oracle.com>,
	"Fam Zheng" <fam@euphon.net>,
	"Swapnil Ingle" <swapnil.ingle@nutanix.com>,
	"John G Johnson" <john.g.johnson@oracle.com>,
	qemu-level <qemu-devel@nongnu.org>,
	kraxel@redhat.com, quintela@redhat.com, mst@redhat.com,
	armbru@redhat.com, kanth.ghatraju@oracle.com, felipe@nutanix.com,
	thuth@redhat.com, ehabkost@redhat.com, konrad.wilk@oracle.com,
	dgilbert@redhat.com, alex.williamson@redhat.com,
	thanos.makatos@nutanix.com, kwolf@redhat.com,
	"\"Daniel P. Berrangé\"" <berrange@redhat.com>,
	mreitz@redhat.com, ross.lagerwall@citrix.com,
	marcandre.lureau@gmail.com, pbonzini@redhat.com
Subject: Re: [PATCH v19 08/20] io: add qio_channel_readv_full_all_eof & qio_channel_readv_full_all helpers
Date: Fri, 15 Jan 2021 08:46:07 -0500	[thread overview]
Message-ID: <62563EA4-2C73-450E-A96B-2D8014E4ED03@oracle.com> (raw)
In-Reply-To: <20210115092017.GA334489@stefanha-x1.localdomain>

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

Thanks for confirming, Stefan!

Since we are adding fds support in this patch, I suppose we could add
the fds-only case since you feel that’s a better approach.

I believe it’s reasonable to assume that the caller wants to read file
descriptors if it sets the fds double-pointer. I think it would be reasonable
to wait for data if the caller expects to read either data or fds, as follows:
while ((nlocal_iov > 0) || local_fds)

Thank you! 

> 
> Stefan