| Message ID | 1452015002-28493-2-git-send-email-clg@fr.ibm.com |
|---|---|
| State | New |
| Headers | show |
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. > 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 <clg@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? > @@ -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.
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 <clg@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 ? >> @@ -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.
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 <clg@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. -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. > >
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 <clg@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. >> >
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; if (trec == recid) { if (nextrec) { @@ -1198,7 +1198,7 @@ static void get_sdr(IPMIBmcSim *ibs, rsp[2] = IPMI_CC_REQ_ENTRY_NOT_PRESENT; goto out; } - if (cmd[6] > (ibs->sdr.sdr[pos + 4])) { + if (cmd[6] > (ibs->sdr.sdr[pos + 4] + 5)) { rsp[2] = IPMI_CC_PARM_OUT_OF_RANGE; goto out; } @@ -1207,7 +1207,7 @@ static void get_sdr(IPMIBmcSim *ibs, IPMI_ADD_RSP_DATA((nextrec >> 8) & 0xff); if (cmd[7] == 0xff) { - cmd[7] = ibs->sdr.sdr[pos + 4] - cmd[6]; + cmd[7] = ibs->sdr.sdr[pos + 4] + 5 - cmd[6]; } if ((cmd[7] + *rsp_len) > max_rsp_len) { @@ -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); return; } - len = init_sdrs[i + 4]; + len = init_sdrs[i + 4] + 5; recid = init_sdrs[i] | (init_sdrs[i + 1] << 8); if (recid == 0xffff) { break; } - if ((i + len + 5) > sizeof(init_sdrs)) { + if ((i + len) > sizeof(init_sdrs)) { error_report("Problem with recid 0x%4.4x\n", i); return; } sdr_add_entry(ibs, init_sdrs + i, len, NULL); - i += len + 5; + i += len; } ipmi_init_sensors_from_sdrs(ibs);
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 <clg@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(-)