Re: [PULL v4 14/27] io: add qio_channel_readv_full_all_eof & qio_channel_readv_full_all helpers

From: Jag Raman <jag.raman@oracle.com>
To: "Daniel P. Berrangé" <berrange@redhat.com>
Cc: "Fam Zheng" <fam@euphon.net>,
	"Peter Maydell" <peter.maydell@linaro.org>,
	"thuth@redhat.com" <thuth@redhat.com>,
	"Elena Ufimtseva" <elena.ufimtseva@oracle.com>,
	"Eduardo Habkost" <ehabkost@redhat.com>,
	"qemu-block@nongnu.org" <qemu-block@nongnu.org>,
	"John Johnson" <john.g.johnson@oracle.com>,
	"Alex Bennée" <alex.bennee@linaro.org>,
	"Michael S. Tsirkin" <mst@redhat.com>,
	"QEMU Developers" <qemu-devel@nongnu.org>,
	"Wainer dos Santos Moschetta" <wainersm@redhat.com>,
	"Max Reitz" <mreitz@redhat.com>,
	"Paolo Bonzini" <pbonzini@redhat.com>,
	"Stefan Hajnoczi" <stefanha@redhat.com>,
	"Igor Mammedov" <imammedo@redhat.com>,
	"Denis V. Lunev" <den@openvz.org>,
	"Philippe Mathieu-Daudé" <philmd@redhat.com>
Subject: Re: [PULL v4 14/27] io: add qio_channel_readv_full_all_eof & qio_channel_readv_full_all helpers
Date: Thu, 11 Feb 2021 15:48:02 +0000	[thread overview]
Message-ID: <E7BA3F8E-E511-4593-858E-DB8B4C246A36@oracle.com> (raw)
In-Reply-To: <20210211154647.GK1302824@redhat.com>

> On Feb 11, 2021, at 10:46 AM, Daniel P. Berrangé <berrange@redhat.com> wrote:
> 
> On Thu, Feb 11, 2021 at 04:34:40PM +0100, Max Reitz wrote:
>> On 10.02.21 10:26, Stefan Hajnoczi wrote:
>>> From: Elena Ufimtseva <elena.ufimtseva@oracle.com>
>>> 
>>> Adds qio_channel_readv_full_all_eof() and qio_channel_readv_full_all()
>>> to read both data and FDs. Refactors existing code to use these helpers.
>>> 
>>> Signed-off-by: Elena Ufimtseva <elena.ufimtseva@oracle.com>
>>> Signed-off-by: John G Johnson <john.g.johnson@oracle.com>
>>> Signed-off-by: Jagannathan Raman <jag.raman@oracle.com>
>>> Acked-by: Daniel P. Berrangé <berrange@redhat.com>
>>> Message-id: b059c4cc0fb741e794d644c144cc21372cad877d.1611938319.git.jag.raman@oracle.com
>>> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
>>> ---
>>>  include/io/channel.h |  53 +++++++++++++++++++++++
>>>  io/channel.c         | 101 ++++++++++++++++++++++++++++++++++---------
>>>  2 files changed, 134 insertions(+), 20 deletions(-)
>> 
>> [...]
>> 
>>> diff --git a/io/channel.c b/io/channel.c
>>> index 0d4b8b5160..4555021b62 100644
>>> --- a/io/channel.c
>>> +++ b/io/channel.c
>> 
>> [...]
>> 
>>> @@ -135,20 +193,23 @@ int qio_channel_readv_all_eof(QIOChannel *ioc,
>>>      return ret;
>>>  }
>>> -int qio_channel_readv_all(QIOChannel *ioc,
>>> -                          const struct iovec *iov,
>>> -                          size_t niov,
>>> -                          Error **errp)
>>> +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");
>>> -    } else if (ret == 1) {
>>> -        ret = 0;
>>> +        error_prepend(errp,
>>> +                      "Unexpected end-of-file before all data were read.");
>>> +        return -1;
>> 
>> This change breaks iotest 083 (i.e., it segfaults), because
>> qio_channel_readv_full_all_eof() doesn’t set *errp when it returns 0, so
>> there is no error to prepend.
> 
> Opps, yes, this needs to be error_setg() not error_prepend()

Hi Max & Daniel,

We will send a patch shortly to address this issue.

Thank you very much!

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