[Qemu-devel] [PATCH 1/8] ipmi: fix SDR length value

[Qemu-devel] [PATCH 1/8] ipmi: fix SDR length value

[Cédric Le Goater]

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;
 
         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);
-- 
2.1.4

Re: [Qemu-devel] [PATCH 1/8] ipmi: fix SDR length value

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1915 bytes --]

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.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 1/8] ipmi: fix SDR length value

[Cédric Le Goater]

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.
 

Re: [Qemu-devel] [PATCH 1/8] ipmi: fix SDR length value

[Corey Minyard]

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.
>   
>

Re: [Qemu-devel] [PATCH 1/8] ipmi: fix SDR length value

[Cédric Le Goater]

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.
>>  
>