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

[Jag Raman]

> On Jan 14, 2021, at 8:49 AM, Stefan Hajnoczi <stefanha@redhat.com> wrote:
> 
> On Wed, Jan 13, 2021 at 03:53:27PM -0500, Jagannathan Raman wrote:
>>     while (nlocal_iov > 0) {
>>         ssize_t len;
>> -        len = qio_channel_readv(ioc, local_iov, nlocal_iov, errp);
>> +        len = qio_channel_readv_full(ioc, local_iov, nlocal_iov, local_fds,
>> +                                     local_nfds, errp);
>>         if (len == QIO_CHANNEL_ERR_BLOCK) {
>>             if (qemu_in_coroutine()) {
>>                 qio_channel_yield(ioc, G_IO_IN);
>> @@ -112,20 +140,41 @@ int qio_channel_readv_all_eof(QIOChannel *ioc,
>>                 qio_channel_wait(ioc, G_IO_IN);
>>             }
>>             continue;
>> -        } else if (len < 0) {
>> -            goto cleanup;
>> -        } else if (len == 0) {
>> -            if (partial) {
>> +        }
>> +
>> +        if (len <= 0) {
>> +            if ((len == 0) && partial) {
>> +                size_t fd_idx = 0;
>> +
>>                 error_setg(errp,
>>                            "Unexpected end-of-file before all bytes were 
>> read");
>> -            } else {
>> +
>> +                if (nfds) {
>> +                    fd_idx = *nfds;
>> +                    *nfds = 0;
>> +                }
>> +
>> +                while (fds && fd_idx) {
>> +                    close((*fds)[fd_idx - 1]);
>> +                    fd_idx--;
>> +                }
>> +
>> +                if (fds) {
>> +                    g_free(*fds);
>> +                    *fds = NULL;
>> +                }
>> +            } else if (len == 0) {
>>                 ret = 0;
>>             }
> 
> The len < 0 case is missing. This function will return -1 and errp has
> been set by qio_channel_readv_full(). However, we may have received fds
> in a previous loop iteration (partial == true), so it is necessary to
> close, free, and reset fds/nfds before returning.

Thanks for the feedback, Stefan! We have addressed this and sent the next 
version out for review.