[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 1/8] ipmi: fix SDR length value
From: |
Cédric Le Goater |
Subject: |
Re: [Qemu-devel] [PATCH 1/8] ipmi: fix SDR length value |
Date: |
Tue, 12 Jan 2016 09:09:24 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Icedove/38.5.0 |
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 <address@hidden>
>>>> ---
>>>>
>>>> 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.
>>
>