Allow Sensor Data records to use 6-bit ASCII encoding #14

Please try to reuse get_fru_area_str() that does essentially the same.

Pushed a new version with the change you requested.

This logic removes spaces (and any bytes from this 4-byte chunk that follow) from within the string as well. It would be better to just back off the spaces from the end once they are decoded.

Put this down before 'size = j;':

while (j > 1 && str[j-1] == 0x20)
str[--j] = '0円';

Implemented.

This is the structure used to receive data from BMC.
According to the IPMI 2.0 Specification Table 43-2, this field is max 16 bytes.
If OpenBMC now violates the spec (by playing on the subtle difference between 'should' and 'must'), then please explicitly state here something llike:

/* 16 per IPMI 2.0 Table 43-2, but some BMCs violate that and may send as many as 42 */

Also, I'd like you to define this number just once via some macro and then use it everywhere. A name like #define SENSOR_ID_STR_MAXLEN 42 would do with the comment similar to the above.

Also, why exactly 42 ? See the question about 32 in ipmi_fru.c below.

The reason for using 42 is due to using non-integer math:

The assumption is that the Type/Len can/will be assigned 0x1F. There is already ample code within ipmitool allowing this, for example ipmi_fru_print.

As a result, 31 is an allowed value.

(31 * 4 chars) / 3 bytes = 41 1/3 characters
Round the fraction up, to permit capturing that 1/3 character, and you get 42 bytes.

FWIW, I avoided creating the #defines you recommended, as there were already bunches of hard coded values. I was not certain creating the #defines would have been approved, so I left the code with a bunch of 16->42 entries.

I have added the string length #define, and described why that number is being used.
It is also being used for the FRU records as well.
There is bug in the OpenBMC IPMI code. The Type encoding is set to UTF8 instead of Ascii/Latin.
If I haven't already I will submit a commit that fixes that.

I have also done another pass through the code and found additional areas that hadn't been covered.

I avoided creating the #defines you recommended, as there were already bunches of hard coded values

That's the legacy code. There is a lot of hard-code in ipmitool and that makes me really sad.
Let's try to avoid those bad practices with the new code and fix them whenever possible.

Thank you.

Ok, this needs a macro too. And a comment about spec violation as well, I guess.
Also, where does 32 come from? By specification, SDR FRU device description is 16 bytes long, plus the preceding type/length byte. That's the original 17. With 6-bit encoding that's 16*4/3 + 1 = 22. With full length of type/length field that's 2^6 - 1 = 63 bytes of data or 64 bytes total. Using6-bit encoding 63*4/3 + 1 = 85. So, why 32 ? I don't get it.

This is one of those places where the Type/Len already allowed 31 bytes:

memset(desc, 0, sizeof(desc)); memcpy(desc, fru->id_string, __min(fru->id_code & 0x01f, sizeof(desc))); desc[fru->id_code & 0x01f] = 0;
Changing this to 32 allows the id_code to use the full range.
I don't think the FRU print is using the non-latin encoding scheme.

Good example. I've checked the IPMI spec again and found that there is a difference in the description of the Type/Length field between the IPMI and FRU specs. In FRU the length is 6-bit (5..0), whereas in IPMI it's just 5 bit (4..0). Also bits 7:6 = 00 define a Unicode string in IPMI whereas they define binary string in FRU spec.

I believe we now need to introduce two different macros because both approaches are applicable in different parts of code. That is probably out of scope of this change though.

My understanding now is that for some strings the type/length may be as long as 31 bytes, and for some it may be up to 63. Careful checking is needed.

I'll need to look further into this. Please do so as well.

This should be smth like:

printf("%-*s | ", SENSOR_ID_STR_MAXLEN - 1, sr->s_id);

Looks like I need to update this PR. I did not adjust for the six bits of the FRU Type/Len record.

This too. And everywhere else.

this has spaces instead of tabs

I don't think we need to mention some 'this code change' in this comment that will persist for some long time here. Please reword.

Modified to be less temporal

Nit: it's not NULL, it's NUL. That's how the 0円 character is called in ASCII.

Looks to me like these are actually 31, not 42. These are the lengths of raw data, if I get it right. Aren't they?
I guess that's why they were 16 and not 17 here.

Probably we need some #define SDR_MAX_ID_STR_RAW_LEN 31.

And probably it would worth it to rename SDR_MAX_ID_STR_LEN to SDR_MAX_ID_STR_DECODED_LEN or something like that

Created two constants per your suggestion.

I guess this is the place where the difference in Type/Length field definition may kick in.
When this is called for IPMI fields like SDR id_string, it must treat the field as per IPMI spec.
However, when called on a FRU area data, it must follow the FRU spec for the field.

Probably, we need a worker function with a spec type parameter that would do the actual job, and then some wrappers like the original get_fru_area_str and the new get_ipmi_typelen_str.

You may probably then drop the string_len parameter from the get_fru_area_str wrapper and restore it's original API

I prefer returning the already calculated string length.
The function has already determined the size of the string. This makes using strlen() unnecessary. Why recalculate the string length using strlen() when the answer is already available? Especially since it is done in a way that doesn't rely on NUL termination?

You've lost a space after the colon here.
Please restore it here and below as well.

Restored the formatting.

this should be all spaces and no tabs (since the first line is not indented at all)

Updated.

In other cases where returning NULL, *string_len is set to 0. It probably should be here as well. Or it could be set at the top once to 0 and then only set to a real value once below.

Updated per your second suggestion.

The wrapped function calls in this section need to have the tab/space fixup.

Updated

nit: mandate* typo

32 bytes? Or 42 characters? I do not see how we get to 42 bytes.