On 01/08/2016 09:20 PM, Corey Minyard wrote: > 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 <c...@fr.ibm.com> >>>> --- >>>> >>>> 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.
next version will start with this change and the commands you acked. And probably also the FRU commands. I want to take sometime to look at a SDR loader, a simple one, that would use the existing code and a file backend. Thanks for the review. C. > -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. >> >