[Freeipmi-devel] Re: Fujitsu's iRMC S2 - support for "get SEL entry long text" within ipmi-oem

[Albert Chu]

Hey Dan,

Responses inlined below.

On Tue, 2010-08-17 at 16:19 -0700, Dan Lukes wrote:
> On 08/17/10 23:10, Albert Chu:
> > Attached is my reworked patch for ipmi-oem.  Can you give it a shot and
> > LMK if it works?
> 
> Done. Some notes:
> 
> 1. --------------
> if user call for record id >= 65536 the number record ID is silently 
> changed to something else. I recommend not to allow out-of-range values 
> at all.

Doh!  Good catch.

> 2. --------------
> > bytes_rq[8] = UCHAR_MAX;
> 
> Requests with MaxResponseDataSize>100 will fail.
> Also response packet can't be larger than IPMI_OEM_MAX_BYTES
> 
> So you can't ask for more than min(IPMI_OEM_MAX_BYTES-17,100) bytes.

Ahhh.  That's a subtlety I didn't get.  Also, based on Holger's earlier
response (which now makes more sense to me).

---
Unfortunately due to memory constrains in the previous BMC generation
the maximum length of a single response is limited to 64bytes, while on
the current product line you should be able to get the full 100bytes
translated SEL text with a single request at least over LAN. Maximum
(non standard) KCS transfer size is also different between current (255)
and previous (64) generation, so the code should compare the data
received with the total length reported in the response. 
----

We should probably use 64 instead of 100.

> 3. --------------
> > component_length = (rs_len - 16);
> 
> I prefer more safe approach - I assume that string may end before packet 
> end (e.g. len is obtained by strlen). And I doesn't assume that string 
> is '\0' terminated in response also.
> 
> The so-called documentation from Fujitsu is so vague to be trusted so 
> much, IMHO ...
> 
> 4. ----------------
> ... in advance, you count the length including the trailing '\0' (which 
> is present in every response - not only the last part of string), so the 
> concatenation doesn't work (off-by-one bug)

Ahh, this wasn't clear in the Fujitsu docs.  I'll add a comment to make
that more clear.

> 
> 5. ----------------
> >  while (offset < data_length)
> 
> ... will be neverending loop when
> IPMI_OEM_FUJITSU_SEL_ENTRY_LONG_TEXT_MAX_STRING_LENGTH < data_length
> 
> You should replace
> offset = IPMI_OEM_FUJITSU_SEL_ENTRY_LONG_TEXT_MAX_STRING_LENGTH
> with
> offset = data_length
> to resolve the issue. Or use "break;" ...
> 
> 6. ----------------
> bytes_rq array need not to be intialized every round (only offset is 
> changing). Also, most of results needs to be extracted from bytes_rs 
> only once.
> 
> 7. ----------------
> If user asked for 'first' or 'last' record then use actual_record id for 
> subsequent (looped) requests to obtain consistent results as last SEL 
> may become non-last in meantime.
>
>   ======================
> 
> > I did (atleast try) to implement the code to loop if there was more text
> > to retrieve.  Hopefully I implemented it right.
> 
> See [4], [5] and [7]
> 
> > Also, I slightly tweaked the output to more similarly match ipmi-sel
> > output, adding in semi-colons and stuff.  I admit, I'm not entirely sure
> > of the output.  You may have specifically chosen your output
> 
> I tried to follow output format of ipmi-sel.
> 
> I attached the another iteration of patch.
> 
>                               Dan

Attached is the current diff I have in ipmi-oem compared to freeipmi
0.8.8.  Look good?

Al


-- 
Albert Chu
address@hidden
Computer Scientist
High Performance Systems Division
Lawrence Livermore National Laboratory

fujitsu_get_sel_entry_long_text2.patch
Description: Text Data