From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:50028) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aH5de-0001u4-Ez for qemu-devel@nongnu.org; Thu, 07 Jan 2016 03:09:43 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aH5da-000289-8m for qemu-devel@nongnu.org; Thu, 07 Jan 2016 03:09:42 -0500 Received: from [59.151.112.132] (port=45771 helo=heian.cn.fujitsu.com) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aH5dZ-00027w-SN for qemu-devel@nongnu.org; Thu, 07 Jan 2016 03:09:38 -0500 References: <1452047951-17429-1-git-send-email-caoj.fnst@cn.fujitsu.com> <1452047951-17429-4-git-send-email-caoj.fnst@cn.fujitsu.com> <568D3AB2.8020100@redhat.com> From: Cao jin Message-ID: <568E1DD9.7020502@cn.fujitsu.com> Date: Thu, 7 Jan 2016 16:12:09 +0800 MIME-Version: 1.0 In-Reply-To: <568D3AB2.8020100@redhat.com> Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v3 3/4] Add Error **errp for xen_pt_config_init() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake , qemu-devel@nongnu.org Cc: stefano.stabellini@eu.citrix.com On 01/07/2016 12:02 AM, Eric Blake wrote: > On 01/05/2016 07:39 PM, Cao jin wrote: [...] >> +static void xen_pt_config_reg_init(XenPCIPassthroughState *s, >> + XenPTRegGroup *reg_grp, XenPTRegInfo *reg, >> + Error **errp) > > Indentation is now off. > Sharp eyes;) >> @@ -1967,10 +1970,10 @@ static int xen_pt_config_reg_init(XenPCIPassthroughState *s, >> val = data; >> >> if (val & ~size_mask) { >> - XEN_PT_ERR(&s->dev,"Offset 0x%04x:0x%04x expands past register size(%d)!\n", >> - offset, val, reg->size); >> + error_setg(errp, "Offset 0x%04x:0x%04x expands past" >> + " register size(%d)!", offset, val, reg->size); > > Drop the trailing !. Also, while touching this, it's better to have a > space before ( in English. > Ok > >> +void xen_pt_config_init(XenPCIPassthroughState *s, Error **errp) >> { >> int i, rc; >> + Error *local_err = NULL; > > Same comments as earlier in the series about using the shorter 'err' > instead of 'local_err'. > >> >> QLIST_INIT(&s->reg_grps); >> >> @@ -2039,11 +2041,12 @@ int xen_pt_config_init(XenPCIPassthroughState *s) >> reg_grp_offset, >> ®_grp_entry->size); >> if (rc < 0) { >> - XEN_PT_LOG(&s->dev, "Failed to initialize %d/%ld, type=0x%x, rc:%d\n", >> - i, ARRAY_SIZE(xen_pt_emu_reg_grps), >> + error_setg(&local_err, "Failed to initialize %d/%ld, type=0x%x," >> + " rc:%d", i, ARRAY_SIZE(xen_pt_emu_reg_grps), > > This maps ARRAY_SIZE() (which is size_t) to %ld, which can fail to > compile on 32-bit platforms (where size_t is not necessarily long). Fix > it to %zd while touching it. > a question: 1. Is %zu more suitable for size_t? since size_t is unsigned integer. and a personal question after digging into size_t: 2. Does the size of size_t always equal to the word length[*] of computer [*] https://en.wikipedia.org/wiki/Word_%28computer_architecture%29 -- Yours Sincerely, Cao jin