From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:42742) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aBj9Y-0003l6-25 for qemu-devel@nongnu.org; Wed, 23 Dec 2015 08:08:29 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aBj9T-00058D-Tv for qemu-devel@nongnu.org; Wed, 23 Dec 2015 08:08:27 -0500 Received: from [59.151.112.132] (port=8189 helo=heian.cn.fujitsu.com) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aBj9T-00056m-IN for qemu-devel@nongnu.org; Wed, 23 Dec 2015 08:08:23 -0500 References: <1450866192-31401-1-git-send-email-caoj.fnst@cn.fujitsu.com> From: Cao jin Message-ID: <567A9CD3.2040501@cn.fujitsu.com> Date: Wed, 23 Dec 2015 21:08:35 +0800 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [PATCH] Xen PCI passthrough: convert to realize() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefano Stabellini Cc: qemu-devel@nongnu.org Hi Stefano, first of all, thanks for your quick response:) On 12/23/2015 08:03 PM, Stefano Stabellini wrote: > On Wed, 23 Dec 2015, Cao jin wrote: >> Signed-off-by: Cao jin >> --- >> >> Since the callchain is pretty deep & error path is very much too, so I made the >> patch based on the principal: catch/report the most necessary error msg with >> smallest modification.(So you can see I don`t change some functions to void, >> despite they coule be) > > Thanks Cao. > > For consistency with the other functions, I think it would be better to > change all functions to return void or none. > Ok, I`ll select one style may with the smallest modification;) > Also it might be nice to split the patch in a series. > Yup, and the patches should be independent from each other? > The patch as is fails to build: > > qemu/hw/xen/xen_pt_config_init.c: In function ‘xen_pt_config_init’: > qemu/hw/xen/xen_pt_config_init.c:2061:42: error: ‘rc’ may be used uninitialized in this func > really weird...last patch you remind me that it cannot compile, make me find that my computer didn`t install xen-devel package, then I installed it right away. But this time, it really can compile on my computer....anyway, I will check it out later. > >> hw/xen/xen-host-pci-device.c | 79 +++++++++++++++++++++++++++----------------- >> hw/xen/xen-host-pci-device.h | 5 +-- >> hw/xen/xen_pt.c | 67 +++++++++++++++++++------------------ >> hw/xen/xen_pt.h | 5 +-- >> hw/xen/xen_pt_config_init.c | 47 +++++++++++++------------- >> hw/xen/xen_pt_graphics.c | 6 ++-- >> 6 files changed, 118 insertions(+), 91 deletions(-) >> >> diff --git a/hw/xen/xen-host-pci-device.c b/hw/xen/xen-host-pci-device.c >> index 7d8a023..1ab6d97 100644 >> --- a/hw/xen/xen-host-pci-device.c >> +++ b/hw/xen/xen-host-pci-device.c >> @@ -43,13 +43,14 @@ static int xen_host_pci_sysfs_path(const XenHostPCIDevice *d, >> /* The output is truncated, or some other error was encountered */ >> return -ENODEV; >> } >> + >> return 0; >> } > > I would prefer to keep stylistic changes separate, especially the ones > in functions which would be otherwise left unmodified. Maybe you could > move them to a separate patch? > I can do that. > [...] >> + >> if (i != PCI_NUM_REGIONS) { >> /* Invalid format or input to short */ >> - rc = -ENODEV; >> + error_setg(errp, "Invalid format or input to short"); > > ^too short How about printing all the string in buf? like: "Invalid format or input to short: %s", buf for all the other comments below: will fix them up:) > >> } >> >> out: >> close(fd); >> - return rc; >> } >> [...] > -- Yours Sincerely, Cao Jin