From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([2001:4830:134:3::10]:40180)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <caoj.fnst@cn.fujitsu.com>) id 1aGKG4-00050f-GD
	for qemu-devel@nongnu.org; Tue, 05 Jan 2016 00:34:13 -0500
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <caoj.fnst@cn.fujitsu.com>) id 1aGKG1-0002mF-BV
	for qemu-devel@nongnu.org; Tue, 05 Jan 2016 00:34:12 -0500
Received: from [59.151.112.132] (port=50265 helo=heian.cn.fujitsu.com)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <caoj.fnst@cn.fujitsu.com>) id 1aGKG0-0002kD-TP
	for qemu-devel@nongnu.org; Tue, 05 Jan 2016 00:34:09 -0500
References: <1451223640-2569-1-git-send-email-caoj.fnst@cn.fujitsu.com>
	<1451223640-2569-2-git-send-email-caoj.fnst@cn.fujitsu.com>
	<alpine.DEB.2.02.1601041510130.19710@kaball.uk.xensource.com>
From: Cao jin <caoj.fnst@cn.fujitsu.com>
Message-ID: <568B5537.5060807@cn.fujitsu.com>
Date: Tue, 5 Jan 2016 13:31:35 +0800
MIME-Version: 1.0
In-Reply-To: <alpine.DEB.2.02.1601041510130.19710@kaball.uk.xensource.com>
Content-Type: text/plain; charset="windows-1252"; format=flowed
Content-Transfer-Encoding: 7bit
Subject: Re: [Qemu-devel] [PATCH v2 1/4] Add Error **errp for
	xen_host_pci_device_get()
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: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: qemu-devel@nongnu.org



On 01/04/2016 11:15 PM, Stefano Stabellini wrote:
> On Sun, 27 Dec 2015, Cao jin wrote:
>> To catch the error msg. Also modify the caller
>>
>> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
>
> This looks much better, thanks.
>
>
[...]
>>
>> -int xen_host_pci_device_get(XenHostPCIDevice *d, uint16_t domain,
>> -                            uint8_t bus, uint8_t dev, uint8_t func)
>> +void xen_host_pci_device_get(XenHostPCIDevice *d, uint16_t domain,
>> +                            uint8_t bus, uint8_t dev, uint8_t func,
>> +                            Error **errp)
>>   {
>>       unsigned int v;
>> -    int rc = 0;
>>
>>       d->config_fd = -1;
>>       d->domain = domain;
>> @@ -353,43 +360,48 @@ int xen_host_pci_device_get(XenHostPCIDevice *d, uint16_t domain,
>>       d->dev = dev;
>>       d->func = func;
>>
>> -    rc = xen_host_pci_config_open(d);
>> -    if (rc) {
>> +    xen_host_pci_config_open(d, errp);
>> +    if (*errp) {
>
> I think that errp could be NULL, therefore the right way to do this is:
>
>      Error *err = NULL;
>      foo(arg, &err);
>      if (err) {
>          handle the error...
>          error_propagate(errp, err);
>      }
>
> see the comment at the beginning of include/qapi/error.h.
>

Thanks for reminding, I didn`t see the comment of error.h before, now I 
am aware why lots of people like the style you mentioned. Will fix it in 
next version, also the comments in other patch.

[...]

-- 
Yours Sincerely,

Cao jin