From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:36314) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aHdWh-0002vQ-Fx for qemu-devel@nongnu.org; Fri, 08 Jan 2016 15:20:48 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aHdWg-0001Td-Gk for qemu-devel@nongnu.org; Fri, 08 Jan 2016 15:20:47 -0500 Received: from mail-ob0-x234.google.com ([2607:f8b0:4003:c01::234]:36227) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aHdWg-0001TX-Bu for qemu-devel@nongnu.org; Fri, 08 Jan 2016 15:20:46 -0500 Received: by mail-ob0-x234.google.com with SMTP id ba1so363634397obb.3 for ; Fri, 08 Jan 2016 12:20:46 -0800 (PST) References: <1452015002-28493-2-git-send-email-clg@fr.ibm.com> <568C20AC.3020805@redhat.com> <568CCCFD.9030402@fr.ibm.com> From: Corey Minyard Message-ID: <56901A1C.3050705@mvista.com> Date: Fri, 8 Jan 2016 14:20:44 -0600 MIME-Version: 1.0 In-Reply-To: <568CCCFD.9030402@fr.ibm.com> Content-Type: text/plain; charset=utf-8; format=flowed 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: =?UTF-8?Q?C=c3=a9dric_Le_Goater?= , Eric Blake Cc: qemu-devel@nongnu.org, "Michael S. Tsirkin" On 01/06/2016 02:14 AM, Cédric Le Goater wrote: > 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 ? I was just adding one and it didn't matter much at that point? Or I was lazy? I've commented a little earlier on patch 7, the struct is a better way to go. -corey > >>> @@ -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. > >