From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([2001:4830:134:3::10]:34916)
	by lists.gnu.org with esmtp (Exim 4.71) (envelope-from <pl@kamp.de>)
	id 1W9A8w-0008Ew-KW
	for qemu-devel@nongnu.org; Fri, 31 Jan 2014 04:12:16 -0500
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <pl@kamp.de>) id 1W9A8r-00069o-32
	for qemu-devel@nongnu.org; Fri, 31 Jan 2014 04:12:10 -0500
Received: from mx.ipv6.kamp.de ([2a02:248:0:51::16]:41611 helo=mx01.kamp.de)
	by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from <pl@kamp.de>)
	id 1W9A8q-00069g-Of
	for qemu-devel@nongnu.org; Fri, 31 Jan 2014 04:12:05 -0500
Message-ID: <52EB68D5.3050207@kamp.de>
Date: Fri, 31 Jan 2014 10:11:49 +0100
From: Peter Lieven <pl@kamp.de>
MIME-Version: 1.0
References: <1390985425-13165-1-git-send-email-pl@kamp.de>	<1390985425-13165-2-git-send-email-pl@kamp.de>	<20140129161959.GG3079@irqsave.net>	<20140130142224.GA15494@stefanha-thinkpad.redhat.com>	<9B01B588-64FC-45B9-9212-909781C0EA31@kamp.de>
	<CAJSP0QViOxEHTrK3wtcyLRir4w+ZpK-YeZD1idwdzfAc_h6k=Q@mail.gmail.com>
In-Reply-To: <CAJSP0QViOxEHTrK3wtcyLRir4w+ZpK-YeZD1idwdzfAc_h6k=Q@mail.gmail.com>
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 8bit
Subject: Re: [Qemu-devel] [PATCHv7 1/5] block: add native support for NFS
List-Id: <qemu-devel.nongnu.org>
List-Unsubscribe: <https://lists.nongnu.org/mailman/options/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <https://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Stefan Hajnoczi <stefanha@gmail.com>
Cc: =?ISO-8859-1?Q?Beno=EEt_Canet?= <benoit.canet@irqsave.net>, Kevin Wolf <kwolf@redhat.com>, Fam Zheng <famz@redhat.com>, ronnie sahlberg <ronniesahlberg@gmail.com>, Jeff Cody <jcody@redhat.com>, qemu-devel <qemu-devel@nongnu.org>, Max Reitz <mreitz@redhat.com>, Orit Wasserman <owasserm@redhat.com>, Federico Simoncelli <fsimonce@redhat.com>, Stefan Hajnoczi <stefanha@redhat.com>, Wenchao Xia <xiawenc@linux.vnet.ibm.com>

On 31.01.2014 09:57, Stefan Hajnoczi wrote:
> On Thu, Jan 30, 2014 at 10:35 PM, Peter Lieven <pl@kamp.de> wrote:
>> Am 30.01.2014 um 15:22 schrieb Stefan Hajnoczi <stefanha@gmail.com>:
>>
>>> On Wed, Jan 29, 2014 at 05:19:59PM +0100, Benoît Canet wrote:
>>>> Le Wednesday 29 Jan 2014 à 09:50:21 (+0100), Peter Lieven a écrit :
>>>>> +static int nfs_file_open(BlockDriverState *bs, QDict *options, int flags,
>>>>> +                         Error **errp) {
>>>>> +    NFSClient *client = bs->opaque;
>>>>> +    int64_t ret;
>>>>> +    QemuOpts *opts;
>>>>> +    Error *local_err = NULL;
>>>>> +
>>>>> +    opts = qemu_opts_create(&runtime_opts, NULL, 0, &error_abort);
>>>>> +    qemu_opts_absorb_qdict(opts, options, &local_err);
>>>>> +    if (error_is_set(&local_err)) {
>>>>> +        qerror_report_err(local_err);
>>>> I have seen more usage of error_propagate(errp, local_err); in QEMU code.
>>>> Maybe I am missing the point.
>>> Yes, I think you are right.  The Error should be propagated to the
>>> caller.  It's not clear to me whether we can ever get an error from
>>> qemu_opts_absorb_qdict() in this call site though.
>> Is there any action I should take here? If yes, can you advise what
>> to do please.
> The issue is that nfs_file_open() takes an Error **errp argument.
> This means the function should report detailed errors using the Error
> object.
>
> The patch prints and then discards the local_error instead of
> propagating it to the caller's errp.
>
> We should just propagate the error instead of printing it:
> if (error_is_set(&local_err)) {
>      error_propagate(errp, local_err);
>      goto ...;

Ok, you are just referring to this part in nfs_file_open:

     if (error_is_set(&local_err)) {
         qerror_report_err(local_err);
         error_free(local_err);
         return -EINVAL;
     }

which I would change to:

     if (error_is_set(&local_err)) {
         error_propagate(errp, local_err);
         return -EINVAL;
     }

The use of error_setg in nfs_client_open is ok?

Peter