From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:45297) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aGjFR-0006OS-4Y for qemu-devel@nongnu.org; Wed, 06 Jan 2016 03:15:14 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aGjFM-0001aO-3w for qemu-devel@nongnu.org; Wed, 06 Jan 2016 03:15:13 -0500 Received: from e06smtp17.uk.ibm.com ([195.75.94.113]:35692) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aGjFL-0001YV-Qy for qemu-devel@nongnu.org; Wed, 06 Jan 2016 03:15:08 -0500 Received: from localhost by e06smtp17.uk.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Wed, 6 Jan 2016 08:15:05 -0000 Received: from b06cxnps3074.portsmouth.uk.ibm.com (d06relay09.portsmouth.uk.ibm.com [9.149.109.194]) by d06dlp01.portsmouth.uk.ibm.com (Postfix) with ESMTP id 6D42217D805D for ; Wed, 6 Jan 2016 08:15:46 +0000 (GMT) Received: from d06av10.portsmouth.uk.ibm.com (d06av10.portsmouth.uk.ibm.com [9.149.37.251]) by b06cxnps3074.portsmouth.uk.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id u068F1bu57606282 for ; Wed, 6 Jan 2016 08:15:01 GMT Received: from d06av10.portsmouth.uk.ibm.com (localhost [127.0.0.1]) by d06av10.portsmouth.uk.ibm.com (8.14.4/8.14.4/NCO v10.0 AVout) with ESMTP id u067F2M2009707 for ; Wed, 6 Jan 2016 00:15:02 -0700 References: <1452015002-28493-2-git-send-email-clg@fr.ibm.com> <568C20AC.3020805@redhat.com> From: =?UTF-8?Q?C=c3=a9dric_Le_Goater?= Message-ID: <568CCCFD.9030402@fr.ibm.com> Date: Wed, 6 Jan 2016 09:14:53 +0100 MIME-Version: 1.0 In-Reply-To: <568C20AC.3020805@redhat.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [PATCH 1/8] ipmi: fix SDR length value List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake , Corey Minyard Cc: qemu-devel@nongnu.org, "Michael S. Tsirkin" On 01/05/2016 08:59 PM, Eric Blake wrote: > On 01/05/2016 10:29 AM, Cédric Le Goater wrote: > > [meta-comment] Your messages were not marked in-reply-to: the 0/8 cover > letter, but came through as separate threads. This makes it harder to > follow, especially in mail clients that sort top-level threads by most > recent activity on the thread. Yes. My bad. I put 'thread = false' in my .gitconfig for some reason and didn't check before sending. This is fixed. >> The IPMI BMC simulator populates the SDR table with a set of initial >> SDRs. The length of each SDR is taken from the record itself (byte 4) >> which does not include the size of the header. But, the full length >> (header + data) is required by the sdr_add_entry() routine. >> >> Signed-off-by: Cédric Le Goater >> --- >> >> Maybe we could use a sdr struct/typedef to clarify the code. See >> patch 7: "ipmi: introduce an ipmi_bmc_init_sensor() API" >> >> hw/ipmi/ipmi_bmc_sim.c | 14 +++++++------- >> 1 file changed, 7 insertions(+), 7 deletions(-) >> >> diff --git a/hw/ipmi/ipmi_bmc_sim.c b/hw/ipmi/ipmi_bmc_sim.c >> index 0a59e539f549..559e1398d669 100644 >> --- a/hw/ipmi/ipmi_bmc_sim.c >> +++ b/hw/ipmi/ipmi_bmc_sim.c >> @@ -362,7 +362,7 @@ static int sdr_find_entry(IPMISdr *sdr, uint16_t recid, >> >> while (pos < sdr->next_free) { >> uint16_t trec = sdr->sdr[pos] | (sdr->sdr[pos + 1] << 8); >> - unsigned int nextpos = pos + sdr->sdr[pos + 4]; >> + unsigned int nextpos = pos + sdr->sdr[pos + 4] + 5; > > 5 feels like a magic number; should you use a #define and name it? Yes. 5 being the sdr header length. The simulator uses a lot of these byte offsets and I think the code would gain to use a struct as proposed in patch 7: "ipmi: introduce an ipmi_bmc_init_sensor() API". Corey, is there a reason for not doing so ? >> @@ -1709,20 +1709,20 @@ static void ipmi_sim_init(Object *obj) >> for (i = 0;;) { >> int len; >> if ((i + 5) > sizeof(init_sdrs)) { >> - error_report("Problem with recid 0x%4.4x: \n", i); >> + error_report("Problem with recid 0x%4.4x\n", i); > > Please drop the trailing \n as long as you are touching this. Sure. Thanks, C.