From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([2001:4830:134:3::10]:38875)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <caoj.fnst@cn.fujitsu.com>) id 1aH4lO-0007cK-EZ
	for qemu-devel@nongnu.org; Thu, 07 Jan 2016 02:13:39 -0500
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <caoj.fnst@cn.fujitsu.com>) id 1aH4lJ-0003mr-CC
	for qemu-devel@nongnu.org; Thu, 07 Jan 2016 02:13:38 -0500
Received: from [59.151.112.132] (port=18864 helo=heian.cn.fujitsu.com)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <caoj.fnst@cn.fujitsu.com>) id 1aH4lI-0003jd-VK
	for qemu-devel@nongnu.org; Thu, 07 Jan 2016 02:13:33 -0500
References: <1452047951-17429-1-git-send-email-caoj.fnst@cn.fujitsu.com>
	<1452047951-17429-5-git-send-email-caoj.fnst@cn.fujitsu.com>
	<568D3BD6.1050707@redhat.com>
From: Cao jin <caoj.fnst@cn.fujitsu.com>
Message-ID: <568E10B6.5010900@cn.fujitsu.com>
Date: Thu, 7 Jan 2016 15:16:06 +0800
MIME-Version: 1.0
In-Reply-To: <568D3BD6.1050707@redhat.com>
Content-Type: text/plain; charset="utf-8"; format=flowed
Content-Transfer-Encoding: 7bit
Subject: Re: [Qemu-devel] [PATCH v3 4/4] Xen PCI passthru: convert to
 realize()
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: Eric Blake <eblake@redhat.com>, qemu-devel@nongnu.org
Cc: stefano.stabellini@eu.citrix.com



On 01/07/2016 12:07 AM, Eric Blake wrote:
> On 01/05/2016 07:39 PM, Cao jin wrote:
[...]
>
>> @@ -827,27 +827,26 @@ static int xen_pt_initfn(PCIDevice *d)
>>       xen_pt_config_init(s, &local_err);
>>       if (local_err) {
>>           error_append_hint(&local_err, "PCI Config space initialisation failed");
>> -        rc = -1;
>> +        error_propagate(errp, local_err);
>>           goto err_out;
>>       }
>
> Looks like this fixes a memory leak in an earlier patch; maybe you need
> to shuffle hunks around?
>

Sorry, don`t quite undertand what "shuffle hunks around" means, could 
you detail it?

[...]
>>
>>   err_out:
>> +    for (i = 0; i < PCI_ROM_SLOT; i++) {
>> +        object_unparent(OBJECT(&s->bar[i]));
>> +    }
>> +    object_unparent(OBJECT(&s->rom));
>> +
>>       xen_pt_destroy(d);
>>       assert(rc);
>> -    return rc;
>
> Is the assertion still needed?
>

Actually I think in the original code, the assertion isn`t necessary 
too, but I guess it is a kind of defensive coding? You can see, not very 
if (rc) equals TRUE will goto err_out. So I prefer not to touch it when 
I am not sure about the author`s intent.

-- 
Yours Sincerely,

Cao jin