From b286c459dd0948f68db0dd4b3f1715485f3a6748 Mon Sep 17 00:00:00 2001 From: Bertrand Martin Date: Fri, 9 Oct 2026 00:43:19 +0200 Subject: [PATCH 1/7] Decode SDR, SEL, FRU and completion codes by the specification Eleven decoder bugs, all found by the review of 2026-10, fixed together: - #127: a Get SDR reply that carries only the next record ID is accepted, so the walk skips the record instead of failing. - #87: reserved rate unit, modifier unit usage and power restore policy values decode to None/Unknown instead of throwing. - #81: a completion code the library does not list no longer aborts the decoding (and hence times out): CompletionCode.Unknown with the raw code on IPMIException; only 00h and C0h-FFh are generic for IPMI commands, Read FRU Data and Close Session decode their own codes; Get Channel Cipher Suites and Close Session check the completion code. - #86: the FRU locator reads the LUN from bits [4:3] and keeps its SDR record ID; the generic locator reads the bus, span and ID string where Table 43-10 puts them. - #125: SEL OEM entries are decoded with their own layout (manufacturer ID, OEM data), C0h and E0h are in the OEM ranges, reserved types give a Reserved record instead of an exception. - #82: the threshold status bits are decoded in severity order with the right names, and the states of a threshold sensor map the comparison bits to the matching going-low/going-high events. - #110: the scanning-disabled bit is exposed; a reading the BMC flags as unavailable or not scanned is not reported, nor are its states. - #83: thresholds are gated on byte 12, linearized like the reading, e^x and cube root are handled, non-linear types return the linear value, the accuracy exponent and the tolerance are decoded right. - #85: the last multirecord is decoded, unknown multirecords are skipped, a word is 2 bytes, the manufacturing date is UTC and null when unspecified, the power supply capacity is LS byte first, the compatibility masks are read at the record offset, the English language codes are 0 and 25, non-English strings are UTF-16LE; the common header checksum is checked and truncated areas are skipped. - #129: an undefined threshold is NaN so that 0 is a threshold; a Full record without analog reading produces no reading line; an OEM sensor with one state byte gets its state. - #128: a FRU 0 or Get FRU Inventory Area Info failure costs that FRU only, FRU 0 is returned once and from whichever area describes it, and a FRU is truncated at its first unreadable chunk instead of shifting the following chunks into the gap. Verified on the Lenovo IMM and the GIGABYTE BMC: the text result is identical to main apart from reading drift, minus the three GIGABYTE sensors whose readings the BMC flags as unavailable, and the Lenovo FRU warnings dropped from one per chunk to one per FRU. Fixes #127, fixes #87, fixes #81, fixes #86, fixes #125, fixes #82, fixes #110, fixes #83, fixes #85, fixes #129, fixes #128. Co-Authored-By: Claude Fable 5.1 --- .../ipmi/client/IpmiResultConverter.java | 16 +- .../ipmi/client/runner/GetFrusRunner.java | 179 +++++++-------- .../ipmi/client/runner/GetSensorsRunner.java | 9 +- .../coding/commands/IpmiCommandCoder.java | 19 +- .../chassis/GetChassisStatusResponseData.java | 3 +- .../commands/chassis/PowerRestorePolicy.java | 4 + .../core/coding/commands/fru/BaseUnit.java | 2 +- .../core/coding/commands/fru/ReadFruData.java | 139 +++++++++--- .../fru/record/BaseCompatibilityInfo.java | 2 +- .../coding/commands/fru/record/BoardInfo.java | 39 +--- .../coding/commands/fru/record/FruRecord.java | 4 +- .../commands/fru/record/PowerSupplyInfo.java | 5 +- .../commands/fru/record/ProductInfo.java | 2 +- .../ipmi/core/coding/commands/sdr/GetSdr.java | 3 +- .../coding/commands/sdr/GetSensorReading.java | 2 + .../sdr/GetSensorReadingResponseData.java | 35 +++ .../core/coding/commands/sdr/SensorState.java | 25 ++- .../sdr/record/FruDeviceLocatorRecord.java | 10 +- .../commands/sdr/record/FullSensorRecord.java | 47 ++-- .../record/GenericDeviceLocatorRecord.java | 12 +- .../sdr/record/ModifierUnitUsage.java | 3 +- .../coding/commands/sdr/record/RateUnit.java | 3 +- .../core/coding/commands/sel/SelRecord.java | 63 +++++- .../coding/commands/sel/SelRecordType.java | 13 +- .../coding/commands/session/CloseSession.java | 20 ++ .../session/GetChannelCipherSuites.java | 2 +- .../core/coding/payload/CompletionCode.java | 12 +- .../coding/payload/lan/IPMIException.java | 28 +++ .../coding/payload/lan/IpmiLanResponse.java | 25 ++- src/site/markdown/chassis-status.md | 3 +- src/site/markdown/low-level-api.md | 10 +- src/site/markdown/sensors.md | 38 ++-- src/site/markdown/supported-commands.md | 8 +- src/site/markdown/troubleshooting.md | 8 +- src/site/markdown/upgrading.md | 32 +++ .../IpmiResultConverterReadingTest.java | 60 +++++ .../ipmi/client/IpmiResultConverterTest.java | 4 +- .../ipmi/client/runner/GetFrusRunnerTest.java | 20 ++ .../client/runner/GetSensorsRunnerTest.java | 50 ++++- .../coding/commands/IpmiCommandCoderTest.java | 104 ++++++--- .../core/coding/commands/IpmiResponses.java | 43 ++++ .../GetChassisStatusResponseDataTest.java | 23 ++ .../coding/commands/fru/ReadFruDataTest.java | 205 ++++++++++++++++++ .../core/coding/commands/sdr/GetSdrTest.java | 36 +++ .../commands/sdr/GetSensorReadingTest.java | 81 +++++++ .../sdr/record/FullSensorRecordTest.java | 106 +++++++++ .../sdr/record/LocatorRecordTest.java | 64 ++++++ .../coding/commands/sel/SelRecordTest.java | 89 ++++++++ 48 files changed, 1423 insertions(+), 287 deletions(-) create mode 100644 src/test/java/org/metricshub/ipmi/client/IpmiResultConverterReadingTest.java create mode 100644 src/test/java/org/metricshub/ipmi/client/runner/GetFrusRunnerTest.java create mode 100644 src/test/java/org/metricshub/ipmi/core/coding/commands/IpmiResponses.java create mode 100644 src/test/java/org/metricshub/ipmi/core/coding/commands/chassis/GetChassisStatusResponseDataTest.java create mode 100644 src/test/java/org/metricshub/ipmi/core/coding/commands/fru/ReadFruDataTest.java create mode 100644 src/test/java/org/metricshub/ipmi/core/coding/commands/sdr/GetSdrTest.java create mode 100644 src/test/java/org/metricshub/ipmi/core/coding/commands/sdr/GetSensorReadingTest.java create mode 100644 src/test/java/org/metricshub/ipmi/core/coding/commands/sdr/record/FullSensorRecordTest.java create mode 100644 src/test/java/org/metricshub/ipmi/core/coding/commands/sdr/record/LocatorRecordTest.java create mode 100644 src/test/java/org/metricshub/ipmi/core/coding/commands/sel/SelRecordTest.java diff --git a/src/main/java/org/metricshub/ipmi/client/IpmiResultConverter.java b/src/main/java/org/metricshub/ipmi/client/IpmiResultConverter.java index fb7ed08..a5f4ea1 100644 --- a/src/main/java/org/metricshub/ipmi/client/IpmiResultConverter.java +++ b/src/main/java/org/metricshub/ipmi/client/IpmiResultConverter.java @@ -150,8 +150,15 @@ private static String extractFullSensorReadingValue(final Sensor fullSensor) { // Get the unit SensorUnit unit = fullRecord.getSensorBaseUnit(); - // No Reading ? Skip. - if (data.getPlainSensorReading() == NO_READING || deviceType == null || unit == null || sensorName == null) { + // No reading? Skip: the BMC says so (reading unavailable, scanning disabled, no analog reading: IPMI 2.0 + // Table 35-15 and Table 43-1), or the record cannot be described + if (data.getPlainSensorReading() == NO_READING + || !data.isSensorStateValid() + || !data.isScanningEnabled() + || !fullRecord.hasAnalogReading() + || deviceType == null + || unit == null + || sensorName == null) { return null; } @@ -664,7 +671,8 @@ private static T getInfo(final List fruRecords, * @return String value */ private static String getThresholdValue(final DoubleFunction conversionFunction, double threshold) { - return threshold != 0.0 ? String.valueOf(Math.round(conversionFunction.apply(threshold))) : Utils.EMPTY; + // A threshold the SDR does not define is NaN; 0 is a legitimate threshold (0 RPM, 0 degrees) + return Double.isNaN(threshold) ? Utils.EMPTY : String.valueOf(Math.round(conversionFunction.apply(threshold))); } /** @@ -676,7 +684,7 @@ private static String getAvailableThreshold(final DoubleFunction convers return Arrays .stream(thresholds) - .filter(threshold -> threshold != 0.0) + .filter(threshold -> !Double.isNaN(threshold)) .mapToObj(threshold -> getThresholdValue(conversionFunction, threshold)) .findFirst() .orElse(Utils.EMPTY); diff --git a/src/main/java/org/metricshub/ipmi/client/runner/GetFrusRunner.java b/src/main/java/org/metricshub/ipmi/client/runner/GetFrusRunner.java index a5435ae..5a7b3d8 100644 --- a/src/main/java/org/metricshub/ipmi/client/runner/GetFrusRunner.java +++ b/src/main/java/org/metricshub/ipmi/client/runner/GetFrusRunner.java @@ -23,10 +23,13 @@ */ import java.util.ArrayList; +import java.util.HashSet; import java.util.List; +import java.util.Set; import java.util.stream.Collectors; import org.metricshub.ipmi.client.IpmiClientConfiguration; +import org.metricshub.ipmi.client.Utils; import org.metricshub.ipmi.client.model.Fru; import org.metricshub.ipmi.core.coding.commands.IpmiVersion; import org.metricshub.ipmi.core.coding.commands.fru.BaseUnit; @@ -69,7 +72,11 @@ public class GetFrusRunner extends AbstractIpmiRunner> { */ private static final int FRU_READ_PACKET_SIZE = 16; - private boolean systemBoardFruUpdated = false; + /** Name of the system board FRU when none of its areas names it. */ + private static final String SYSTEM_BOARD_NAME = "System Board"; + + /** Device IDs of the FRUs already returned: a FRU is returned once. */ + private final Set returnedFruIds = new HashSet<>(); public GetFrusRunner(IpmiClientConfiguration ipmiConfiguration) { super(ipmiConfiguration); @@ -91,12 +98,13 @@ public List call() throws Exception { int reservationId = 0; int lastReservationId = -1; - List systemBoardFruRecords = getFruRecords(DEFAULT_FRU_ID); + // FRU 0 describes the system board on most BMCs; not all of them expose it, and the SDR walk must not + // depend on it + List systemBoardFruRecords = readFruRecords(DEFAULT_FRU_ID); // We get sensor data until we encounter ID = 65535 which means that // this record is the last one. while (getNextRecId() < MAX_REPO_RECORD_ID) { - SensorRecord sensorRecord = null; try { @@ -107,18 +115,16 @@ public List call() throws Exception { processFruRecord(result, sensorRecord, systemBoardFruRecords); } catch (IPMIException e) { - // If getting sensor data failed, we check if it already failed // with this reservation ID, so that we avoid the infinite loop. if (lastReservationId == reservationId || e.getCompletionCode() != CompletionCode.ReservationCanceled) { throw e; } - lastReservationId = reservationId; // If the cause of the failure was canceling of the // reservation, we get new reservationId and retry. This can - // happen many times during getting all sensors, since BMC can't + // happen many times during getting all sensors, since the BMC cannot // manage parallel sessions and invalidates old one if new one // appears. reservationId = ((ReserveSdrRepositoryResponseData) getConnector() @@ -127,92 +133,89 @@ public List call() throws Exception { new ReserveSdrRepository(IpmiVersion.V20, getHandle().getCipherSuite(), AuthenticationType.RMCPPlus))) .getReservationId(); } - } return result; } /** - * Process the given sensor record and create the system board FRU record. The new {@link Fru} is added to th FRU list - * result - * - * @param result List of {@link Fru} instance to append - * @param sensorRecord The sensor record to process - * @param systemBoardFruRecords The system board Fru records - * @throws Exception + * Adds to the result the FRU a FRU Device Locator record points at, or FRU 0 attached to the system board when a + * System Board sensor record is met before any locator for it. A FRU is returned once. */ private void processFruRecord( final List result, final SensorRecord sensorRecord, final List systemBoardFruRecords) - throws Exception { - try { - // Process the FRU record - Fru fru = null; - if (sensorRecord instanceof FruDeviceLocatorRecord) { - FruDeviceLocatorRecord fruLocator = (FruDeviceLocatorRecord) sensorRecord; + throws InterruptedException { - if (fruLocator.isLogical()) { - List fruRecords = getFruRecords(fruLocator.getDeviceId()); + if (sensorRecord instanceof FruDeviceLocatorRecord) { + FruDeviceLocatorRecord fruLocator = (FruDeviceLocatorRecord) sensorRecord; + int deviceId = fruLocator.getDeviceId(); - if (!fruRecords.isEmpty()) { - fru = new Fru(fruLocator, fruRecords); - } + if (fruLocator.isLogical() && !returnedFruIds.contains(deviceId)) { + List fruRecords = deviceId == DEFAULT_FRU_ID ? systemBoardFruRecords : readFruRecords(deviceId); + if (!fruRecords.isEmpty()) { + result.add(new Fru(fruLocator, fruRecords)); + returnedFruIds.add(deviceId); } - } else - if (!systemBoardFruRecords.isEmpty() - && !systemBoardFruUpdated - && sensorRecord instanceof CompactSensorRecord - && ((CompactSensorRecord) sensorRecord).getEntityId().equals(EntityId.SystemBoard)) { - - // Since we can only access the SystemBoard components, - // we need to build the FruDeviceLocatorRecord for SystemBoard instance. - - // OK this can be one of the SystemBoard sensors - CompactSensorRecord compactSensorRecord = (CompactSensorRecord) sensorRecord; - - BoardInfo boardInfo = systemBoardFruRecords - .stream() - .filter(BoardInfo.class::isInstance) - .map(BoardInfo.class::cast) - .findFirst() - .orElse(null); - - if (boardInfo != null) { - - // Create the Fru locator - FruDeviceLocatorRecord locator = new FruDeviceLocatorRecord(); - locator.setFruEntityId(EntityId.SystemBoard.getCode()); - locator.setFruEntityInstance(compactSensorRecord.getEntityInstanceNumber()); - locator.setName(boardInfo.getBoardProductName() + " " + compactSensorRecord.getEntityInstanceNumber()); - - fru = new Fru(locator, systemBoardFruRecords); - - // OK, now we are good! - systemBoardFruUpdated = true; - - } - } - - // Add the Fru instance - if (fru != null) { - result.add(fru); } + } else + if (!systemBoardFruRecords.isEmpty() + && !returnedFruIds.contains(DEFAULT_FRU_ID) + && sensorRecord instanceof CompactSensorRecord + && EntityId.SystemBoard.equals(((CompactSensorRecord) sensorRecord).getEntityId())) { + // No locator pointed at FRU 0 so far: build one for the system board, named after whichever area + // describes it + CompactSensorRecord compactSensorRecord = (CompactSensorRecord) sensorRecord; + + FruDeviceLocatorRecord locator = new FruDeviceLocatorRecord(); + locator.setDeviceId(DEFAULT_FRU_ID); + locator.setLogical(true); + locator.setFruEntityId(EntityId.SystemBoard.getCode()); + locator.setFruEntityInstance(compactSensorRecord.getEntityInstanceNumber()); + locator + .setName(systemBoardName(systemBoardFruRecords) + " " + compactSensorRecord.getEntityInstanceNumber()); + + result.add(new Fru(locator, systemBoardFruRecords)); + returnedFruIds.add(DEFAULT_FRU_ID); + } + } - } catch (IPMIException e) { - LOGGER.warn("Failed to read the FRU of sensor record {}: {}", sensorRecord.getId(), e.getMessage()); + /** + * @return the name of the system board from its board, product or chassis area, whichever exists + */ + static String systemBoardName(final List fruRecords) { + for (FruRecord record : fruRecords) { + String name = null; + if (record instanceof BoardInfo) { + name = ((BoardInfo) record).getBoardProductName(); + } else if (record instanceof ProductInfo) { + name = ((ProductInfo) record).getProductName(); + } else if (record instanceof ChassisInfo) { + name = ((ChassisInfo) record).getChassisPartNumber(); + } + if (Utils.isNotBlank(name)) { + return name; + } } - + return SYSTEM_BOARD_NAME; } /** - * Get the FRU records for the given FRU identifier fruId - * - * @param fruId The unique identifier of the FRU - * @return new List of {@link FruRecord} instances - * @throws Exception + * Reads a FRU; a FRU that cannot be read (absent, not answering, undecodable) is logged and reported empty, so + * that the other FRUs are still returned. */ + private List readFruRecords(int fruId) throws InterruptedException { + try { + return getFruRecords(fruId); + } catch (InterruptedException e) { + throw e; + } catch (Exception e) { + LOGGER.warn("Failed to read FRU {}: {}", fruId, e.getMessage()); + return new ArrayList<>(); + } + } + private List getFruRecords(int fruId) throws Exception { List fruData = new ArrayList<>(); @@ -249,29 +252,29 @@ private List getFruRecords(int fruId) throws Exception { unit, i, fruReadPacketSize)); - fruData.add(data); - + } catch (InterruptedException e) { + throw e; } catch (Exception e) { - LOGGER.warn("Failed to read FRU {} at offset {}, the FRU data will be truncated: {}", fruId, i, e.getMessage()); + // Stop here: the chunks after a gap would shift into its place and decode into wrong fields + LOGGER + .warn("Failed to read FRU {} at offset {}, the FRU data is truncated there: {}", fruId, i, e.getMessage()); + break; } } - try { - // after collecting all the data, we can combine and parse it - return ReadFruData - .decodeFruData(fruData) - .stream() - .filter( - fruRecord -> fruRecord instanceof BoardInfo - || fruRecord instanceof ChassisInfo - || fruRecord instanceof ProductInfo) - .collect(Collectors.toList()); - - } catch (Exception e) { - LOGGER.warn("Failed to decode FRU {}: {}", fruId, e.getMessage()); + if (fruData.isEmpty()) { + return new ArrayList<>(); } - return new ArrayList<>(); + // after collecting all the data, we can combine and parse it + return ReadFruData + .decodeFruData(fruData) + .stream() + .filter( + fruRecord -> fruRecord instanceof BoardInfo + || fruRecord instanceof ChassisInfo + || fruRecord instanceof ProductInfo) + .collect(Collectors.toList()); } } diff --git a/src/main/java/org/metricshub/ipmi/client/runner/GetSensorsRunner.java b/src/main/java/org/metricshub/ipmi/client/runner/GetSensorsRunner.java index 58144db..b919ba2 100644 --- a/src/main/java/org/metricshub/ipmi/client/runner/GetSensorsRunner.java +++ b/src/main/java/org/metricshub/ipmi/client/runner/GetSensorsRunner.java @@ -137,7 +137,8 @@ public List call() throws Exception { * @return String value */ static String buildStates(final GetSensorReadingResponseData data, final SensorRecord sensorRecord) { - if (data == null) { + // IPMI 2.0 Table 35-15: neither the reading nor the states are valid when the sensor is unavailable or not scanned + if (data == null || !data.isSensorStateValid() || !data.isScanningEnabled()) { return Utils.EMPTY; } @@ -166,10 +167,14 @@ static String buildStates(final GetSensorReadingResponseData data, final SensorR * @return a string value of the state in a format of deviceName"=0x"+raw[3]+raw[2] */ static String buildOemState(final byte[] raw, final String deviceName) { - if (raw == null || raw.length < 4) { + if (raw == null || raw.length < 3) { throw new IllegalArgumentException( String.format("Invalid IPMI raw command date for device %s.", deviceName)); } + // The second state byte (states 8-14) is optional in Get Sensor Reading (Table 35-15) + if (raw.length == 3) { + return String.format("%s=0x%02x", deviceName, raw[2]); + } return String.format("%s=0x%02x%02x", deviceName, raw[3], raw[2]); } diff --git a/src/main/java/org/metricshub/ipmi/core/coding/commands/IpmiCommandCoder.java b/src/main/java/org/metricshub/ipmi/core/coding/commands/IpmiCommandCoder.java index c70498f..4c323c4 100644 --- a/src/main/java/org/metricshub/ipmi/core/coding/commands/IpmiCommandCoder.java +++ b/src/main/java/org/metricshub/ipmi/core/coding/commands/IpmiCommandCoder.java @@ -87,12 +87,27 @@ protected byte[] validateResponse(IpmiMessage message) throws IPMIException { throw new IllegalArgumentException("Invalid response payload"); } IpmiLanResponse response = (IpmiLanResponse) message.getPayload(); - if (response.getCompletionCode() != CompletionCode.Ok) { - throw new IPMIException(response.getCompletionCode()); + CompletionCode completionCode = response.getCompletionCode(); + if (completionCode == CompletionCode.Unknown) { + completionCode = decodeCommandSpecificCompletionCode(response.getRawCompletionCode()); + } + if (completionCode != CompletionCode.Ok) { + throw new IPMIException(completionCode, response.getRawCompletionCode()); } return response.getIpmiCommandData(); } + /** + * Gives a meaning to a command-specific or OEM completion code (01h-7Eh and 80h-BEh, IPMI 2.0 Table 5-2) of this + * command. The default knows none of them. + * + * @param rawCode the completion code byte of the response + * @return the matching {@link CompletionCode}, or {@link CompletionCode#Unknown} + */ + protected CompletionCode decodeCommandSpecificCompletionCode(int rawCode) { + return CompletionCode.Unknown; + } + /** * Retrieves command code specific for command represented by this class * diff --git a/src/main/java/org/metricshub/ipmi/core/coding/commands/chassis/GetChassisStatusResponseData.java b/src/main/java/org/metricshub/ipmi/core/coding/commands/chassis/GetChassisStatusResponseData.java index 271226d..f0dd5a9 100644 --- a/src/main/java/org/metricshub/ipmi/core/coding/commands/chassis/GetChassisStatusResponseData.java +++ b/src/main/java/org/metricshub/ipmi/core/coding/commands/chassis/GetChassisStatusResponseData.java @@ -63,7 +63,8 @@ public PowerRestorePolicy getPowerRestorePolicy() { case 2: return PowerRestorePolicy.PoweredUp; default: - throw new IllegalArgumentException("Invalid Power Restore Policy"); + // 11b is "unknown" in IPMI 2.0 Table 28-3 + return PowerRestorePolicy.Unknown; } } diff --git a/src/main/java/org/metricshub/ipmi/core/coding/commands/chassis/PowerRestorePolicy.java b/src/main/java/org/metricshub/ipmi/core/coding/commands/chassis/PowerRestorePolicy.java index 19032ef..556a38a 100644 --- a/src/main/java/org/metricshub/ipmi/core/coding/commands/chassis/PowerRestorePolicy.java +++ b/src/main/java/org/metricshub/ipmi/core/coding/commands/chassis/PowerRestorePolicy.java @@ -39,4 +39,8 @@ public enum PowerRestorePolicy { * Chassis always powers up after AC/mains returns */ PoweredUp, + /** + * The BMC reports the policy as unknown (11b). + */ + Unknown, } diff --git a/src/main/java/org/metricshub/ipmi/core/coding/commands/fru/BaseUnit.java b/src/main/java/org/metricshub/ipmi/core/coding/commands/fru/BaseUnit.java index d35eb93..2ade9c7 100644 --- a/src/main/java/org/metricshub/ipmi/core/coding/commands/fru/BaseUnit.java +++ b/src/main/java/org/metricshub/ipmi/core/coding/commands/fru/BaseUnit.java @@ -32,7 +32,7 @@ public enum BaseUnit { private static final int BYTES = 0; private static final int WORDS = 1; private static final int BYTESIZE = 1; - private static final int WORDSIZE = 16; + private static final int WORDSIZE = 2; private int code; diff --git a/src/main/java/org/metricshub/ipmi/core/coding/commands/fru/ReadFruData.java b/src/main/java/org/metricshub/ipmi/core/coding/commands/fru/ReadFruData.java index 94a6fd2..597ba80 100644 --- a/src/main/java/org/metricshub/ipmi/core/coding/commands/fru/ReadFruData.java +++ b/src/main/java/org/metricshub/ipmi/core/coding/commands/fru/ReadFruData.java @@ -40,8 +40,14 @@ import org.metricshub.ipmi.core.coding.protocol.AuthenticationType; import org.metricshub.ipmi.core.coding.protocol.IpmiMessage; import org.metricshub.ipmi.core.coding.security.CipherSuite; +import org.metricshub.ipmi.core.coding.payload.CompletionCode; import org.metricshub.ipmi.core.common.TypeConverter; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import java.util.function.BiFunction; + import java.security.InvalidKeyException; import java.security.NoSuchAlgorithmException; import java.util.ArrayList; @@ -53,6 +59,16 @@ */ public class ReadFruData extends IpmiCommandCoder { + private static final Logger LOGGER = LoggerFactory.getLogger(ReadFruData.class); + + /** Size of the common header (FRU spec section 8). */ + private static final int COMMON_HEADER_SIZE = 8; + + /** Size of a multirecord header (FRU spec section 16.1). */ + private static final int MULTIRECORD_HEADER_SIZE = 5; + + private static final int FRU_DEVICE_BUSY = 0x81; + private int offset; private int size; @@ -171,6 +187,12 @@ protected IpmiPayload preparePayload(int sequenceNumber) TypeConverter.intToByte(sequenceNumber)); } + @Override + protected CompletionCode decodeCommandSpecificCompletionCode(int rawCode) { + // IPMI 2.0 Table 34-3 + return rawCode == FRU_DEVICE_BUSY ? CompletionCode.Frudevicebusy : CompletionCode.Unknown; + } + @Override public ResponseData getResponseData(IpmiMessage message) throws IPMIException, @@ -229,40 +251,107 @@ public static List decodeFruData( offset += length; } - if (data[0] == 0x1) { - - int chassisOffset = TypeConverter.byteToInt(data[2]) * 8; - int boardOffset = TypeConverter.byteToInt(data[3]) * 8; - int productInfoOffset = TypeConverter.byteToInt(data[4]) * 8; - int multiRecordOffset = TypeConverter.byteToInt(data[5]) * 8; - - if (chassisOffset != 0) { - list.add(new ChassisInfo(data, chassisOffset)); - } - if (boardOffset != 0) { - list.add(new BoardInfo(data, boardOffset)); - } - if (productInfoOffset != 0) { - list.add(new ProductInfo(data, productInfoOffset)); - } - if (multiRecordOffset != 0) { - addMultirecords(list, data, multiRecordOffset); - } - } else { + if (data.length < COMMON_HEADER_SIZE) { + throw new IllegalArgumentException("FRU data shorter than its common header: " + data.length + " byte(s)"); + } + if (data[0] != 0x1) { // TODO: recognize SPD records returned by DIMM FRUs (#107) throw new IllegalArgumentException("Invalid format version: " + data[0]); } + if (!isChecksumValid(data, 0, COMMON_HEADER_SIZE)) { + throw new IllegalArgumentException("Invalid common header checksum"); + } + + // Offsets in multiples of 8 bytes (FRU spec section 8) + addArea(list, data, TypeConverter.byteToInt(data[2]) * 8, "chassis", ChassisInfo::new); + addArea(list, data, TypeConverter.byteToInt(data[3]) * 8, "board", BoardInfo::new); + addArea(list, data, TypeConverter.byteToInt(data[4]) * 8, "product", ProductInfo::new); + + int multiRecordOffset = TypeConverter.byteToInt(data[5]) * 8; + if (multiRecordOffset != 0) { + addMultirecords(list, data, multiRecordOffset); + } return list; } - private static void addMultirecords(ArrayList list, byte[] data, int multiRecordOffset) { - int currentMultirecordOffset = multiRecordOffset; + /** + * Decodes one of the chassis, board or product info areas, when the data read holds it: a truncated read, a + * bad checksum or a decoding failure costs that area, not the whole FRU. + */ + private static void addArea( + List list, + byte[] data, + int offset, + String area, + BiFunction decoder) { + if (offset == 0) { + return; // area not present + } + if (offset + 2 > data.length) { + LOGGER.warn("The {} info area at offset {} is beyond the {} byte(s) read: skipped", area, offset, data.length); + return; + } + int length = TypeConverter.byteToInt(data[offset + 1]) * 8; + if (offset + length > data.length) { + LOGGER + .warn( + "The {} info area at offset {} is truncated ({} of {} bytes read): skipped", + area, + offset, + data.length - offset, + length); + return; + } + if (!isChecksumValid(data, offset, length)) { + LOGGER.debug("The {} info area at offset {} has an invalid checksum: decoded anyway", area, offset); + } + try { + list.add(decoder.apply(data, offset)); + } catch (RuntimeException e) { + LOGGER.warn("Cannot decode the {} info area at offset {}: {}", area, offset, e.getMessage()); + } + } - while ((TypeConverter.byteToInt(data[currentMultirecordOffset + 1]) & 0x80) == 0) { - list.add(MultiRecordInfo.populateMultiRecord(data, currentMultirecordOffset)); - currentMultirecordOffset += TypeConverter.byteToInt(data[currentMultirecordOffset + 2]) + 5; + /** + * Decodes the multirecord area (FRU spec section 16): the records are length-delimited, so one the library does + * not model is skipped, and the record carrying the end-of-list flag is decoded too. + */ + private static void addMultirecords(List list, byte[] data, int multiRecordOffset) { + int offset = multiRecordOffset; + boolean last = false; + + while (!last && offset + MULTIRECORD_HEADER_SIZE <= data.length) { + last = (TypeConverter.byteToInt(data[offset + 1]) & 0x80) != 0; + int length = TypeConverter.byteToInt(data[offset + 2]); + + if (offset + MULTIRECORD_HEADER_SIZE + length > data.length) { + LOGGER.warn("The multirecord at offset {} is truncated: the rest of the multirecord area is skipped", offset); + return; + } + try { + list.add(MultiRecordInfo.populateMultiRecord(data, offset)); + } catch (RuntimeException e) { + LOGGER + .warn( + "Skipping the multirecord of type 0x{} at offset {}: {}", + Integer.toHexString(TypeConverter.byteToInt(data[offset])), + offset, + e.getMessage()); + } + offset += MULTIRECORD_HEADER_SIZE + length; + } + } + + /** + * @return whether the bytes of the given range add up to zero modulo 256, as every FRU header and area must + */ + private static boolean isChecksumValid(byte[] data, int offset, int length) { + int sum = 0; + for (int i = offset; i < offset + length; i++) { + sum += data[i]; } + return (sum & 0xff) == 0; } } diff --git a/src/main/java/org/metricshub/ipmi/core/coding/commands/fru/record/BaseCompatibilityInfo.java b/src/main/java/org/metricshub/ipmi/core/coding/commands/fru/record/BaseCompatibilityInfo.java index 762d0de..fa790a6 100644 --- a/src/main/java/org/metricshub/ipmi/core/coding/commands/fru/record/BaseCompatibilityInfo.java +++ b/src/main/java/org/metricshub/ipmi/core/coding/commands/fru/record/BaseCompatibilityInfo.java @@ -67,7 +67,7 @@ public BaseCompatibilityInfo(byte[] fruData, int offset, int length) { compatibilityBase = TypeConverter.byteToInt(fruData[offset + 4]); codeStart = TypeConverter.byteToInt(fruData[offset + 5]) & 0x7f; codeRangeMasks = new byte[length - 6]; - System.arraycopy(fruData, 6, codeRangeMasks, 0, length - 6); + System.arraycopy(fruData, offset + 6, codeRangeMasks, 0, length - 6); } public int getManufacturerId() { diff --git a/src/main/java/org/metricshub/ipmi/core/coding/commands/fru/record/BoardInfo.java b/src/main/java/org/metricshub/ipmi/core/coding/commands/fru/record/BoardInfo.java index 0f2756b..e5b2df6 100644 --- a/src/main/java/org/metricshub/ipmi/core/coding/commands/fru/record/BoardInfo.java +++ b/src/main/java/org/metricshub/ipmi/core/coding/commands/fru/record/BoardInfo.java @@ -24,14 +24,8 @@ import org.metricshub.ipmi.core.common.TypeConverter; -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; - -import java.text.DateFormat; -import java.text.ParseException; import java.util.ArrayList; import java.util.Date; -import java.util.Locale; /** * FRU record containing Board info.
@@ -40,6 +34,9 @@ */ public class BoardInfo extends FruRecord { + /** 1996-01-01 00:00:00 UTC. */ + private static final long MFG_DATE_EPOCH_MS = 820454400000L; + private Date mfgDate; private String boardManufacturer = ""; @@ -54,8 +51,6 @@ public class BoardInfo extends FruRecord { private String[] customBoardInfo = new String[0]; - private static Logger logger = LoggerFactory.getLogger(BoardInfo.class); - /** * Creates and populates record * @@ -76,19 +71,9 @@ public BoardInfo(final byte[] fruData, final int offset) { buffer[2] = fruData[offset + 5]; buffer[3] = 0; - DateFormat df = DateFormat - .getDateInstance( - DateFormat.SHORT, - Locale.ENGLISH); - try { - setMfgDate( - new Date( - df.parse("01/01/96").getTime() - + ((long) TypeConverter.littleEndianByteArrayToInt(buffer)) - * 60000L)); - } catch (ParseException e) { - logger.error(e.getMessage(), e); - } + // Minutes since 1996-01-01 00:00 UTC (FRU spec section 11); 0 means unspecified + long minutes = TypeConverter.littleEndianByteArrayToInt(buffer); + setMfgDate(minutes == 0 ? null : new Date(MFG_DATE_EPOCH_MS + minutes * 60000L)); int partNumber = TypeConverter.byteToInt(fruData[offset + 6]); @@ -141,16 +126,14 @@ private ArrayList readCustomInfo( decodeString( partType, partNumberData, - languageCode != 0 - && languageCode != 25)); + languageCode == 0 || languageCode == 25)); break; case 1: setBoardProductName( decodeString( partType, partNumberData, - languageCode != 0 - && languageCode != 25)); + languageCode == 0 || languageCode == 25)); break; case 2: setBoardSerialNumber( @@ -164,8 +147,7 @@ private ArrayList readCustomInfo( decodeString( partType, partNumberData, - languageCode != 0 - && languageCode != 25)); + languageCode == 0 || languageCode == 25)); break; case 4: setFruFileId(partNumberData); @@ -181,8 +163,7 @@ private ArrayList readCustomInfo( decodeString( partType, partNumberData, - languageCode != 0 - && languageCode != 25)); + languageCode == 0 || languageCode == 25)); break; } } diff --git a/src/main/java/org/metricshub/ipmi/core/coding/commands/fru/record/FruRecord.java b/src/main/java/org/metricshub/ipmi/core/coding/commands/fru/record/FruRecord.java index 6490258..b1bf61c 100644 --- a/src/main/java/org/metricshub/ipmi/core/coding/commands/fru/record/FruRecord.java +++ b/src/main/java/org/metricshub/ipmi/core/coding/commands/fru/record/FruRecord.java @@ -69,11 +69,11 @@ protected static String decodeString( case 2: return TypeConverter.decode6bitAscii(data); case 3: - System.arraycopy(data, 0, data, 0, data.length); if (isEnglishLanguageCode) { return new String(data, Charset.forName("ISO-8859-1")).trim(); } else { - return new String(data, Charset.forName("UTF-8")).trim(); + // Non-English type 3 strings are 2-byte Unicode, least significant byte first (FRU spec section 13) + return new String(data, Charset.forName("UTF-16LE")).trim(); } default: throw new IllegalArgumentException("Invalid type format"); diff --git a/src/main/java/org/metricshub/ipmi/core/coding/commands/fru/record/PowerSupplyInfo.java b/src/main/java/org/metricshub/ipmi/core/coding/commands/fru/record/PowerSupplyInfo.java index 9c97df9..a139f5d 100644 --- a/src/main/java/org/metricshub/ipmi/core/coding/commands/fru/record/PowerSupplyInfo.java +++ b/src/main/java/org/metricshub/ipmi/core/coding/commands/fru/record/PowerSupplyInfo.java @@ -98,8 +98,9 @@ public PowerSupplyInfo(byte[] fruData, int offset) { // TODO: Test when server containing such records will be available - capacity = TypeConverter.byteToInt(fruData[offset]) & 0xf; - capacity |= TypeConverter.byteToInt(fruData[offset + 1]) << 4; + // 12-bit capacity, least significant byte first (FRU spec section 18.1) + capacity = TypeConverter.byteToInt(fruData[offset]); + capacity |= (TypeConverter.byteToInt(fruData[offset + 1]) & 0x0f) << 8; peakVa = TypeConverter.byteToInt(fruData[offset + 2]); peakVa |= TypeConverter.byteToInt(fruData[offset + 3]) << 8; diff --git a/src/main/java/org/metricshub/ipmi/core/coding/commands/fru/record/ProductInfo.java b/src/main/java/org/metricshub/ipmi/core/coding/commands/fru/record/ProductInfo.java index e0148fe..30a9c76 100644 --- a/src/main/java/org/metricshub/ipmi/core/coding/commands/fru/record/ProductInfo.java +++ b/src/main/java/org/metricshub/ipmi/core/coding/commands/fru/record/ProductInfo.java @@ -180,7 +180,7 @@ private ArrayList readCustomInfo( } private boolean isEnglishLanguageCode(int languageCode) { - return languageCode != 0 && languageCode != 25; + return languageCode == 0 || languageCode == 25; } private boolean partDataLengthWithinBounds(byte[] fruData, int currentOffset, int partDataLength) { diff --git a/src/main/java/org/metricshub/ipmi/core/coding/commands/sdr/GetSdr.java b/src/main/java/org/metricshub/ipmi/core/coding/commands/sdr/GetSdr.java index 8c02955..08b832f 100644 --- a/src/main/java/org/metricshub/ipmi/core/coding/commands/sdr/GetSdr.java +++ b/src/main/java/org/metricshub/ipmi/core/coding/commands/sdr/GetSdr.java @@ -154,7 +154,8 @@ public ResponseData getResponseData(IpmiMessage message) InvalidKeyException { byte[] raw = validateResponse(message); - if (raw == null || raw.length < 3) { + // Two bytes (the next record ID) and no record data is a valid, if unhelpful, answer: the walk must go on + if (raw == null || raw.length < 2) { throw new IllegalArgumentException( "Invalid response payload length"); } diff --git a/src/main/java/org/metricshub/ipmi/core/coding/commands/sdr/GetSensorReading.java b/src/main/java/org/metricshub/ipmi/core/coding/commands/sdr/GetSensorReading.java index 247218e..df3aab8 100644 --- a/src/main/java/org/metricshub/ipmi/core/coding/commands/sdr/GetSensorReading.java +++ b/src/main/java/org/metricshub/ipmi/core/coding/commands/sdr/GetSensorReading.java @@ -106,6 +106,8 @@ public ResponseData getResponseData(IpmiMessage message) responseData .setSensorStateValid((TypeConverter.byteToInt(raw[1]) & 0x20) == 0); + responseData.setScanningEnabled((TypeConverter.byteToInt(raw[1]) & 0x40) != 0); + if (raw.length >= 3) { responseData .setSensorState( diff --git a/src/main/java/org/metricshub/ipmi/core/coding/commands/sdr/GetSensorReadingResponseData.java b/src/main/java/org/metricshub/ipmi/core/coding/commands/sdr/GetSensorReadingResponseData.java index bc60468..63d1ec5 100644 --- a/src/main/java/org/metricshub/ipmi/core/coding/commands/sdr/GetSensorReadingResponseData.java +++ b/src/main/java/org/metricshub/ipmi/core/coding/commands/sdr/GetSensorReadingResponseData.java @@ -37,6 +37,14 @@ * Wrapper for Get Sensor Reading response. */ public class GetSensorReadingResponseData implements ResponseData { + + private static final int THRESHOLD_EVENT_READING_TYPE = 1; + + /** + * Event offsets (Table 42-2, event/reading type 01h) of the threshold comparison bits of Table 35-15: LNC going + * low, LC going low, LNR going low, UNC going high, UC going high, UNR going high. + */ + private static final int[] THRESHOLD_EVENT_OFFSETS = { 0, 2, 4, 7, 9, 11 }; private byte sensorReading; /** @@ -47,6 +55,8 @@ public class GetSensorReadingResponseData implements ResponseData { */ private boolean sensorStateValid; + private boolean scanningEnabled; + /** * Contains state of the sensor if it is threshold-based. */ @@ -82,6 +92,18 @@ public void setSensorStateValid(boolean sensorStateValid) { this.sensorStateValid = sensorStateValid; } + /** + * @return false when the BMC reports sensor scanning as disabled (byte 2 bit 6): the reading and the states + * are then not to be used + */ + public boolean isScanningEnabled() { + return scanningEnabled; + } + + public void setScanningEnabled(boolean scanningEnabled) { + this.scanningEnabled = scanningEnabled; + } + /** * Contains state of the sensor if it is threshold-based. */ @@ -117,6 +139,19 @@ public List getStatesAsserted( SensorType sensorType, int sensorEventReadingType) { ArrayList list = new ArrayList(); + if (statesAsserted == null) { + return list; + } + if (sensorEventReadingType == THRESHOLD_EVENT_READING_TYPE) { + // Byte 3 of a threshold sensor holds comparison bits (Table 35-15), not event offsets: bit n means + // "at or beyond" the threshold whose going-low (lower) or going-high (upper) event offset is below + for (int i = 0; i < THRESHOLD_EVENT_OFFSETS.length && i < statesAsserted.length; ++i) { + if (statesAsserted[i]) { + list.add(ReadingType.parseInt(sensorType, sensorEventReadingType, THRESHOLD_EVENT_OFFSETS[i])); + } + } + return list; + } for (int i = 0; i < statesAsserted.length; ++i) { if (statesAsserted[i]) { list diff --git a/src/main/java/org/metricshub/ipmi/core/coding/commands/sdr/SensorState.java b/src/main/java/org/metricshub/ipmi/core/coding/commands/sdr/SensorState.java index cb0e708..fdbfc27 100644 --- a/src/main/java/org/metricshub/ipmi/core/coding/commands/sdr/SensorState.java +++ b/src/main/java/org/metricshub/ipmi/core/coding/commands/sdr/SensorState.java @@ -60,16 +60,14 @@ public int getCode() { return code; } + /** + * Decodes the threshold comparison status (IPMI 2.0 Table 35-15, byte 3 bits [5:0]) into the most severe + * threshold crossed. + * + * @param value bits [5:0]: LNC, LC, LNR, UNC, UC, UNR + * @return the most severe state, {@link #Ok} when no bit is set + */ public static SensorState parseInt(int value) { - if ((value & BELOWLOWERNONRECOVERABLE) != 0) { - return BelowLowerNonRecoverable; - } - if ((value & BELOWLOWERCRITICAL) != 0) { - return BelowLowerCritical; - } - if ((value & ABOVEUPPERNONCRITICAL) != 0) { - return BelowLowerNonCritical; - } if ((value & ABOVEUPPERNONRECOVERABLE) != 0) { return AboveUpperNonRecoverable; } @@ -79,6 +77,15 @@ public static SensorState parseInt(int value) { if ((value & ABOVEUPPERNONCRITICAL) != 0) { return AboveUpperNonCritical; } + if ((value & BELOWLOWERNONRECOVERABLE) != 0) { + return BelowLowerNonRecoverable; + } + if ((value & BELOWLOWERCRITICAL) != 0) { + return BelowLowerCritical; + } + if ((value & BELOWLOWERNONCRITICAL) != 0) { + return BelowLowerNonCritical; + } if (value == OK) { return Ok; } diff --git a/src/main/java/org/metricshub/ipmi/core/coding/commands/sdr/record/FruDeviceLocatorRecord.java b/src/main/java/org/metricshub/ipmi/core/coding/commands/sdr/record/FruDeviceLocatorRecord.java index 0cd58ac..a5da789 100644 --- a/src/main/java/org/metricshub/ipmi/core/coding/commands/sdr/record/FruDeviceLocatorRecord.java +++ b/src/main/java/org/metricshub/ipmi/core/coding/commands/sdr/record/FruDeviceLocatorRecord.java @@ -71,15 +71,9 @@ protected void populateTypeSpecficValues(byte[] recordData, SensorRecord record) } setDeviceId(deviceIdFromRecord); - int id = TypeConverter.byteToInt(recordData[6]); - if (!isLogical()) { - id >>= 1; - } - - setId(id); - - setAccessLun((TypeConverter.byteToInt(recordData[7]) & 0xc) >> 2); + // IPMI 2.0 Table 43-7 byte 8: bit 7 logical, bits [4:3] LUN, bits [2:0] private bus ID + setAccessLun((TypeConverter.byteToInt(recordData[7]) & 0x18) >> 3); setManagementChannelNumber((TypeConverter.byteToInt(recordData[8]) & 0xf0) >> 4); diff --git a/src/main/java/org/metricshub/ipmi/core/coding/commands/sdr/record/FullSensorRecord.java b/src/main/java/org/metricshub/ipmi/core/coding/commands/sdr/record/FullSensorRecord.java index 8a7beed..c716718 100644 --- a/src/main/java/org/metricshub/ipmi/core/coding/commands/sdr/record/FullSensorRecord.java +++ b/src/main/java/org/metricshub/ipmi/core/coding/commands/sdr/record/FullSensorRecord.java @@ -49,22 +49,25 @@ public class FullSensorRecord extends AbstractSensorRecord { private double sensorMinmumReading; - private double upperNonRecoverableThreshold; + // A threshold the record does not define (Table 43-1, byte 12 bits [3:2] and the readable mask of byte 19) stays NaN + private double upperNonRecoverableThreshold = Double.NaN; - private double lowerNonRecoverableThreshold; + private double lowerNonRecoverableThreshold = Double.NaN; - private double upperCriticalThreshold; + private double upperCriticalThreshold = Double.NaN; - private double lowerCriticalThreshold; + private double lowerCriticalThreshold = Double.NaN; - private double upperNonCriticalThreshold; + private double upperNonCriticalThreshold = Double.NaN; - private double lowerNonCriticalThreshold; + private double lowerNonCriticalThreshold = Double.NaN; private byte sensorUnits1; private int linearization; + private static final int NO_ANALOG_READING = 3; + @Override protected void populateTypeSpecficValues( byte[] recordData, @@ -79,12 +82,8 @@ protected void populateTypeSpecficValues( setM(TypeConverter.decode2sComplement(calcM, 9)); sensorUnits1 = recordData[20]; - - setTolerance( - calcFormula( - (TypeConverter.byteToInt(recordData[25]) & 0x3f) / 2, - 8, - sensorUnits1)); + // Needed by calcFormula(): set before the first conversion + linearization = TypeConverter.byteToInt(recordData[23]) & 0x7f; int calcB = TypeConverter.byteToInt(recordData[26]); @@ -96,7 +95,7 @@ protected void populateTypeSpecficValues( calcAcc |= (TypeConverter.byteToInt(recordData[28]) & 0xf0) << 2; - int exp = TypeConverter.byteToInt(recordData[28]) & 0xc >> 2; + int exp = (TypeConverter.byteToInt(recordData[28]) & 0x0c) >> 2; setAccuracy((double) calcAcc / 10000 * Math.pow(10, exp)); @@ -133,7 +132,8 @@ protected void populateTypeSpecficValues( TypeConverter .byteToInt(recordData[35]))); - if ((TypeConverter.byteToInt(recordData[10]) & 0x4) != 0) { + // Byte 12 bits [3:2] (Table 43-1): 00b means the sensor has no thresholds + if ((TypeConverter.byteToInt(recordData[11]) & 0x0c) != 0) { if ((TypeConverter.byteToInt(recordData[18]) & 0x20) != 0) { setUpperNonRecoverableThreshold( calcFormula( @@ -175,7 +175,8 @@ protected void populateTypeSpecficValues( populateName(recordData, 47); - linearization = TypeConverter.byteToInt(recordData[23]) & 0x7f; + // Tolerance in half raw counts (byte 26 bits [5:0]): +/- tolerance / 2 x |M| x 10^R (Table 43-1) + setTolerance((TypeConverter.byteToInt(recordData[25]) & 0x3f) / 2.0 * Math.abs(getM()) * Math.pow(10, getrExp())); } private double getM() { @@ -316,6 +317,14 @@ public void setLowerNonCriticalThreshold(double lowerNonCriticalThreshold) { * - Value to be converted. Length of 8 is assumed. * @return converted value */ + /** + * @return false when the data format of Sensor Units 1 is 11b (Table 43-1): the sensor has no analog reading and + * the reading byte must not be converted + */ + public boolean hasAnalogReading() { + return ((TypeConverter.byteToInt(sensorUnits1) & 0xc0) >> 6) != NO_ANALOG_READING; + } + public double calcFormula(int value) { return calcFormula(value, 8, sensorUnits1); } @@ -369,6 +378,8 @@ protected double calcFormula(int value, int length, byte units1) { return Math.log10(result); case 3: return Math.log(result) / Math.log(2); + case 4: + return Math.exp(result); case 5: return Math.pow(10, result); case 6: @@ -382,9 +393,11 @@ protected double calcFormula(int value, int length, byte units1) { case 10: return Math.pow(result, 0.5); case 11: - return Math.pow(result, 0.33); + return Math.cbrt(result); default: - throw new IllegalArgumentException("Unsupported linearization type"); + // 70h-7Fh are non-linear (the linearization needs Get Sensor Reading Factors), the rest is reserved: + // return the linear conversion, as ipmitool does, rather than drop the sensor + return result; } } diff --git a/src/main/java/org/metricshub/ipmi/core/coding/commands/sdr/record/GenericDeviceLocatorRecord.java b/src/main/java/org/metricshub/ipmi/core/coding/commands/sdr/record/GenericDeviceLocatorRecord.java index 90ce614..176a9b5 100644 --- a/src/main/java/org/metricshub/ipmi/core/coding/commands/sdr/record/GenericDeviceLocatorRecord.java +++ b/src/main/java/org/metricshub/ipmi/core/coding/commands/sdr/record/GenericDeviceLocatorRecord.java @@ -68,9 +68,10 @@ protected void populateTypeSpecficValues( setAccessLun((TypeConverter.byteToInt(recordData[7]) & 0x18) >> 3); - setBusId(TypeConverter.byteToInt(recordData[7]) & 0x3); + // IPMI 2.0 Table 43-10: bus ID is bits [2:0] of byte 8, address span bits [2:0] of byte 9 + setBusId(TypeConverter.byteToInt(recordData[7]) & 0x7); - setAddressSpan(TypeConverter.byteToInt(recordData[8]) & 0x3); + setAddressSpan(TypeConverter.byteToInt(recordData[8]) & 0x7); setDeviceType(DeviceType.parseInt(TypeConverter.byteToInt(recordData[10]))); setDeviceTypeModifier(TypeConverter.byteToInt(recordData[11])); @@ -78,11 +79,12 @@ protected void populateTypeSpecficValues( setEntityId(TypeConverter.byteToInt(recordData[12])); setEntityInstance(TypeConverter.byteToInt(recordData[13])); - byte[] nameData = new byte[recordData.length - 17]; + // Byte 16 is the ID string type/length, the string starts at byte 17 (Table 43-10) + byte[] nameData = new byte[recordData.length - 16]; - System.arraycopy(recordData, 17, nameData, 0, nameData.length); + System.arraycopy(recordData, 16, nameData, 0, nameData.length); - setName(decodeName(recordData[16], nameData)); + setName(decodeName(recordData[15], nameData)); } public int getDeviceAccessAddress() { diff --git a/src/main/java/org/metricshub/ipmi/core/coding/commands/sdr/record/ModifierUnitUsage.java b/src/main/java/org/metricshub/ipmi/core/coding/commands/sdr/record/ModifierUnitUsage.java index c734f03..22926f2 100644 --- a/src/main/java/org/metricshub/ipmi/core/coding/commands/sdr/record/ModifierUnitUsage.java +++ b/src/main/java/org/metricshub/ipmi/core/coding/commands/sdr/record/ModifierUnitUsage.java @@ -57,7 +57,8 @@ public static ModifierUnitUsage parseInt(int value) { case MULITPLY: return Mulitply; default: - throw new IllegalArgumentException("Invalid value: " + value); + // Reserved values (IPMI 2.0 Table 43-1): a unit the record does not define, not a record to drop + return None; } } } diff --git a/src/main/java/org/metricshub/ipmi/core/coding/commands/sdr/record/RateUnit.java b/src/main/java/org/metricshub/ipmi/core/coding/commands/sdr/record/RateUnit.java index dd43cd4..adaa54f 100644 --- a/src/main/java/org/metricshub/ipmi/core/coding/commands/sdr/record/RateUnit.java +++ b/src/main/java/org/metricshub/ipmi/core/coding/commands/sdr/record/RateUnit.java @@ -74,7 +74,8 @@ public static RateUnit parseInt(int value) { case D: return Days; default: - throw new IllegalArgumentException("Invalid value: " + value); + // Reserved values (IPMI 2.0 Table 43-1): a unit the record does not define, not a record to drop + return None; } } } diff --git a/src/main/java/org/metricshub/ipmi/core/coding/commands/sel/SelRecord.java b/src/main/java/org/metricshub/ipmi/core/coding/commands/sel/SelRecord.java index 35aade7..a3cd0c5 100644 --- a/src/main/java/org/metricshub/ipmi/core/coding/commands/sel/SelRecord.java +++ b/src/main/java/org/metricshub/ipmi/core/coding/commands/sel/SelRecord.java @@ -29,6 +29,8 @@ import org.metricshub.ipmi.core.coding.commands.sdr.record.SensorType; import org.metricshub.ipmi.core.common.TypeConverter; +import java.util.Arrays; + public class SelRecord { private int recordId; @@ -57,6 +59,10 @@ public class SelRecord { */ private byte reading; + private Integer manufacturerId; + + private byte[] oemData; + public static SelRecord populateSelRecord(byte[] data) { SelRecord record = new SelRecord(); @@ -71,6 +77,39 @@ public static SelRecord populateSelRecord(byte[] data) { record.setRecordType(SelRecordType.parseInt(TypeConverter.byteToInt(data[2]))); + switch (record.getRecordType()) { + case System: + populateSystemEvent(record, data); + break; + case OemTimestamped: + // IPMI 2.0 section 32.2: timestamp, 3-byte manufacturer ID, 6 OEM-defined bytes + System.arraycopy(data, 3, buffer, 0, 4); + record.setTimestamp(TypeConverter.decodeDate(TypeConverter.littleEndianByteArrayToInt(buffer))); + buffer[0] = data[7]; + buffer[1] = data[8]; + buffer[2] = data[9]; + buffer[3] = 0; + record.setManufacturerId(TypeConverter.littleEndianByteArrayToInt(buffer)); + record.setOemData(Arrays.copyOfRange(data, 10, 16)); + break; + case OemNonTimestamped: + // IPMI 2.0 section 32.3: 13 OEM-defined bytes + record.setOemData(Arrays.copyOfRange(data, 3, 16)); + break; + default: + // Reserved: nothing but the record ID is defined + break; + } + + return record; + } + + /** + * Decodes the body of a system event record (IPMI 2.0 section 32.1). + */ + private static void populateSystemEvent(SelRecord record, byte[] data) { + byte[] buffer = new byte[4]; + System.arraycopy(data, 3, buffer, 0, 4); record.setTimestamp(TypeConverter.decodeDate(TypeConverter.littleEndianByteArrayToInt(buffer))); @@ -88,8 +127,30 @@ public static SelRecord populateSelRecord(byte[] data) { record.setEvent(ReadingType.parseInt(record.getSensorType(), eventType, eventOffset)); record.setReading(data[14]); + } - return record; + /** + * @return the manufacturer ID (IANA enterprise number) of an OEM timestamped record, null for the other record + * types + */ + public Integer getManufacturerId() { + return manufacturerId; + } + + public void setManufacturerId(Integer manufacturerId) { + this.manufacturerId = manufacturerId; + } + + /** + * @return the OEM-defined bytes of an OEM record (6 for a timestamped one, 13 otherwise), null for the other + * record types + */ + public byte[] getOemData() { + return oemData; + } + + public void setOemData(byte[] oemData) { + this.oemData = oemData; } public void setRecordId(int recordId) { diff --git a/src/main/java/org/metricshub/ipmi/core/coding/commands/sel/SelRecordType.java b/src/main/java/org/metricshub/ipmi/core/coding/commands/sel/SelRecordType.java index 3d48b55..c0ec334 100644 --- a/src/main/java/org/metricshub/ipmi/core/coding/commands/sel/SelRecordType.java +++ b/src/main/java/org/metricshub/ipmi/core/coding/commands/sel/SelRecordType.java @@ -26,7 +26,11 @@ public enum SelRecordType { OemTimestamped(SelRecordType.OEMTIMESTAMPED), System(SelRecordType.SYSTEM), OemNonTimestamped( - SelRecordType.OEMNONTIMESTAMPED),; + SelRecordType.OEMNONTIMESTAMPED), + /** + * A reserved record type (03h-BFh, IPMI 2.0 section 32): only the record ID of such an entry is meaningful. + */ + Reserved(SelRecordType.RESERVED),; /** * Represents OEM timestamped record type (C0h-DFh) @@ -37,6 +41,7 @@ public enum SelRecordType { * Represents OEM timestamped record type (E0h-FFh) */ private static final int OEMNONTIMESTAMPED = 224; + private static final int RESERVED = -1; private int code; @@ -52,12 +57,12 @@ public static SelRecordType parseInt(int value) { if (value == SYSTEM) { return System; } - if (value > OEMNONTIMESTAMPED) { + if (value >= OEMNONTIMESTAMPED) { return OemNonTimestamped; } - if (value > OEMTIMESTAMPED) { + if (value >= OEMTIMESTAMPED) { return OemTimestamped; } - throw new IllegalArgumentException("Invalid value: " + value); + return Reserved; } } diff --git a/src/main/java/org/metricshub/ipmi/core/coding/commands/session/CloseSession.java b/src/main/java/org/metricshub/ipmi/core/coding/commands/session/CloseSession.java index 21a93d2..88b1466 100644 --- a/src/main/java/org/metricshub/ipmi/core/coding/commands/session/CloseSession.java +++ b/src/main/java/org/metricshub/ipmi/core/coding/commands/session/CloseSession.java @@ -32,6 +32,7 @@ import org.metricshub.ipmi.core.coding.protocol.AuthenticationType; import org.metricshub.ipmi.core.coding.protocol.IpmiMessage; import org.metricshub.ipmi.core.coding.security.CipherSuite; +import org.metricshub.ipmi.core.coding.payload.CompletionCode; import org.metricshub.ipmi.core.common.TypeConverter; import java.security.InvalidKeyException; @@ -42,6 +43,10 @@ */ public class CloseSession extends IpmiCommandCoder { + private static final int INVALID_SESSION_ID = 0x87; + + private static final int INVALID_SESSION_HANDLE = 0x88; + private int sessionId; /** @@ -92,11 +97,26 @@ protected IpmiPayload preparePayload(int sequenceNumber) TypeConverter.intToByte(sequenceNumber)); } + @Override + protected CompletionCode decodeCommandSpecificCompletionCode(int rawCode) { + // IPMI 2.0 Table 22-19 + switch (rawCode) { + case INVALID_SESSION_ID: + return CompletionCode.InvalidSessionId; + case INVALID_SESSION_HANDLE: + return CompletionCode.InvalidSessionHandle; + default: + return CompletionCode.Unknown; + } + } + @Override public ResponseData getResponseData(IpmiMessage message) throws IPMIException, NoSuchAlgorithmException, InvalidKeyException { + validateResponse(message); + return new CloseSessionResponseData(); } diff --git a/src/main/java/org/metricshub/ipmi/core/coding/commands/session/GetChannelCipherSuites.java b/src/main/java/org/metricshub/ipmi/core/coding/commands/session/GetChannelCipherSuites.java index 0a7efbe..23394cc 100644 --- a/src/main/java/org/metricshub/ipmi/core/coding/commands/session/GetChannelCipherSuites.java +++ b/src/main/java/org/metricshub/ipmi/core/coding/commands/session/GetChannelCipherSuites.java @@ -181,7 +181,7 @@ public ResponseData getResponseData(IpmiMessage message) GetChannelCipherSuitesResponseData data = new GetChannelCipherSuitesResponseData(); - byte[] raw = message.getPayload().getIpmiCommandData(); + byte[] raw = validateResponse(message); data.setChannelNumber(raw[0]); diff --git a/src/main/java/org/metricshub/ipmi/core/coding/payload/CompletionCode.java b/src/main/java/org/metricshub/ipmi/core/coding/payload/CompletionCode.java index 78a7606..082d4b7 100644 --- a/src/main/java/org/metricshub/ipmi/core/coding/payload/CompletionCode.java +++ b/src/main/java/org/metricshub/ipmi/core/coding/payload/CompletionCode.java @@ -217,6 +217,11 @@ public enum CompletionCode { * Invalid role. */ InvalidRole(CompletionCode.INVALIDROLE), + /** + * A code this enumeration does not list: command-specific, OEM or reserved. The raw value is available on the + * {@link org.metricshub.ipmi.core.coding.payload.lan.IPMIException}. + */ + Unknown(CompletionCode.UNKNOWN), ; private static final int OK = 0; @@ -258,6 +263,7 @@ public enum CompletionCode { private static final int COMMANDNOTSUPPORTED = 213; private static final int ILLEGALPARAMETER = 214; private static final int UNSPECIFIEDERROR = 255; + private static final int UNKNOWN = -1; private static final int INVALIDPAYLOADTYPE = 3; private static final int INVALIDAUTHENTICATIONALGORITHM = 4; private static final int INVALIDINTEGRITYALGORITHM = 5; @@ -381,7 +387,9 @@ public static CompletionCode parseInt(int value) { case INVALIDROLE: return InvalidRole; default: - throw new IllegalArgumentException("Invalid value: " + value); + // IPMI 2.0 Table 5-2 lets every command use command-specific (01h-7Eh) and OEM (80h-BEh) codes, and + // reserves the rest: a code this table does not list must not abort the decoding of the response + return Unknown; } } @@ -479,6 +487,8 @@ public String getMessage() { return "Inactive session ID."; case INVALIDROLE: return "Invalid role."; + case UNKNOWN: + return "Unknown completion code."; default: throw new IllegalArgumentException("Invalid value: " + code); } diff --git a/src/main/java/org/metricshub/ipmi/core/coding/payload/lan/IPMIException.java b/src/main/java/org/metricshub/ipmi/core/coding/payload/lan/IPMIException.java index fcbb95b..51fc36a 100644 --- a/src/main/java/org/metricshub/ipmi/core/coding/payload/lan/IPMIException.java +++ b/src/main/java/org/metricshub/ipmi/core/coding/payload/lan/IPMIException.java @@ -28,18 +28,46 @@ public class IPMIException extends Exception { private static final long serialVersionUID = 1L; + private static final int OEM_CODES_START = 0x80; + + private static final int GENERIC_CODES_START = 0xC0; + private final CompletionCode completionCode; + private final int rawCode; + public IPMIException(CompletionCode completionCode) { + this(completionCode, completionCode.getCode()); + } + + /** + * @param completionCode the decoded completion code, {@link CompletionCode#Unknown} for a code the library + * does not list + * @param rawCode the completion code byte as the BMC sent it + */ + public IPMIException(CompletionCode completionCode, int rawCode) { this.completionCode = completionCode; + this.rawCode = rawCode; } public CompletionCode getCompletionCode() { return completionCode; } + /** + * @return the completion code byte as the BMC sent it, meaningful when {@link #getCompletionCode()} is + * {@link CompletionCode#Unknown} + */ + public int getRawCode() { + return rawCode; + } + @Override public String getMessage() { + if (completionCode == CompletionCode.Unknown && rawCode >= 0) { + String kind = rawCode < OEM_CODES_START ? "Command-specific" : rawCode < GENERIC_CODES_START ? "OEM" : "Reserved"; + return String.format("%s completion code 0x%02X.", kind, rawCode); + } return completionCode.getMessage(); } } diff --git a/src/main/java/org/metricshub/ipmi/core/coding/payload/lan/IpmiLanResponse.java b/src/main/java/org/metricshub/ipmi/core/coding/payload/lan/IpmiLanResponse.java index a222e09..9bcc96f 100644 --- a/src/main/java/org/metricshub/ipmi/core/coding/payload/lan/IpmiLanResponse.java +++ b/src/main/java/org/metricshub/ipmi/core/coding/payload/lan/IpmiLanResponse.java @@ -30,19 +30,36 @@ */ public class IpmiLanResponse extends IpmiLanMessage { + private static final int GENERIC_CODES_START = 0xC0; + private CompletionCode completionCode; + private int rawCompletionCode; + + /** + * Decodes the completion code. Only 00h and the generic codes (C0h-FFh, IPMI 2.0 Table 5-2) have a meaning + * common to every command; a command-specific (01h-7Eh) or OEM (80h-BEh) code is {@link CompletionCode#Unknown} + * here, and the command coder may give it its own meaning. + * + * @param completionCode the completion code byte of the response + */ public void setCompletionCode(byte completionCode) { - this.completionCode = CompletionCode - .parseInt( - TypeConverter - .byteToInt(completionCode)); + rawCompletionCode = TypeConverter.byteToInt(completionCode); + this.completionCode = rawCompletionCode == 0 || rawCompletionCode >= GENERIC_CODES_START ? + CompletionCode.parseInt(rawCompletionCode) : CompletionCode.Unknown; } public CompletionCode getCompletionCode() { return completionCode; } + /** + * @return the completion code byte as the BMC sent it + */ + public int getRawCompletionCode() { + return rawCompletionCode; + } + /** * Builds IPMI LAN response message from byte array. * diff --git a/src/site/markdown/chassis-status.md b/src/site/markdown/chassis-status.md index 2afbad3..08a4876 100644 --- a/src/site/markdown/chassis-status.md +++ b/src/site/markdown/chassis-status.md @@ -60,8 +60,7 @@ decodes the response (IPMI 2.0, section 28.2): > [!NOTE] > `getChassisIdentifyState()` throws `IllegalAccessError` when > `isChassisIdentifyCommandSupported()` is `false`: check it first. `getPowerRestorePolicy()` -> throws `IllegalArgumentException` when the BMC reports the policy as *unknown* -> ([#87](https://github.com/metricshub/ipmi-java/issues/87)). +> returns `Unknown` when the BMC reports the policy as such. Not every BMC fills every flag: the intrusion, drive and cooling bits in particular are optional in the specification, and a BMC that does not implement them reports `false`. diff --git a/src/site/markdown/low-level-api.md b/src/site/markdown/low-level-api.md index 81277a7..69729c2 100644 --- a/src/site/markdown/low-level-api.md +++ b/src/site/markdown/low-level-api.md @@ -165,7 +165,9 @@ if (info.getEntriesCount() > 0) { // Get SEL Entry fails on an empty SEL System.out.println(record.getTimestamp() + " " + record.getSensorType() + " " + record.getEvent() + " " + record.getEventDirection()); } else { - System.out.println("OEM entry " + record.getRecordId()); // vendor-defined content + // OEM entries (IPMI 2.0 sections 32.2 and 32.3): the manufacturer ID is null for the non-timestamped ones + System.out.println(record.getRecordType() + " entry " + record.getRecordId() + " from manufacturer " + + record.getManufacturerId() + ": " + Arrays.toString(record.getOemData())); } recordId = entry.getNextRecordId(); } @@ -175,9 +177,9 @@ if (info.getEntriesCount() > 0) { // Get SEL Entry fails on an empty SEL ```text SEL entries: 643 Wed May 15 11:15:25 CEST 2024 EventLoggingDisabled LogAreaReset Assertion -OEM entry 2 -OEM entry 3 -OEM entry 4 +OemTimestamped entry 2 from manufacturer 19046: [2, 1, 5, 0, 0, 0] +OemTimestamped entry 3 from manufacturer 19046: [2, 8, 5, 0, 0, 0] +OemTimestamped entry 4 from manufacturer 19046: [2, 2, 5, 0, 0, 0] Wed May 15 11:23:27 CEST 2024 PowerUnit PowerOffOrDown Assertion Wed May 15 11:23:34 CEST 2024 PowerUnit PowerOffOrDown Deassertion ``` diff --git a/src/site/markdown/sensors.md b/src/site/markdown/sensors.md index 126c290..257a4a7 100644 --- a/src/site/markdown/sensors.md +++ b/src/site/markdown/sensors.md @@ -54,41 +54,35 @@ linear formula (`M`, `B` and the exponents of IPMI 2.0, section 36.3): ```java for (Sensor sensor : IpmiClient.getSensors(config)) { - // isSensorStateValid() is false when the BMC flags the reading as unavailable - if (sensor.isFull() && sensor.getData() != null && sensor.getData().isSensorStateValid()) { + if (sensor.isFull() && sensor.getData() != null) { FullSensorRecord record = (FullSensorRecord) sensor.getRecord(); - double value = sensor.getData().getSensorReading(record); - System.out.println(sensor.getName() + " = " + value + " " + record.getSensorBaseUnit() - + " (upper critical: " + record.getUpperCriticalThreshold() + ")"); + GetSensorReadingResponseData data = sensor.getData(); + // The BMC flags a reading it does not vouch for: unavailable, scanning disabled, or no analog reading + if (data.isSensorStateValid() && data.isScanningEnabled() && record.hasAnalogReading()) { + double value = data.getSensorReading(record); + System.out.println(sensor.getName() + " = " + value + " " + record.getSensorBaseUnit() + + " (upper critical: " + record.getUpperCriticalThreshold() + ")"); + } } } ``` ```text Ambient Temp = 17.0 DegreesC (upper critical: 39.0) -System Power = 92.0 Watts (upper critical: 0.0) -Fan 1 = 6600.0 Rpm (upper critical: 0.0) +System Power = 92.0 Watts (upper critical: NaN) +Fan 1 = 6600.0 Rpm (upper critical: NaN) System 3.3V = 3.38 Volts (upper critical: 3.56) ``` `getSensorBaseUnit()` returns a [`SensorUnit`](apidocs/org/metricshub/ipmi/core/coding/commands/sdr/record/SensorUnit.html); the six thresholds (`getLowerNonCriticalThreshold()` to `getUpperNonRecoverableThreshold()`) -are converted with the same formula, and are `0.0` when the BMC does not define them. - -> [!WARNING] -> Known limitations of the decoding, by the IPMI 2.0 specification -> (not all of them reproduced on real hardware): -> -> * a sensor whose reading is flagged *unavailable* or whose scanning is disabled is still -> reported with a reading: check `getData().isSensorStateValid()` as above -> ([#110](https://github.com/metricshub/ipmi-java/issues/110)); -> * the non-linear conversions and the readability of each threshold are not fully handled -> ([#83](https://github.com/metricshub/ipmi-java/issues/83)); -> * the threshold status bits of the reading are mis-mapped -> ([#82](https://github.com/metricshub/ipmi-java/issues/82)); -> * a Compact record that describes several shared sensors is reported as a single sensor -> ([#100](https://github.com/metricshub/ipmi-java/issues/100)). +are converted with the same formula (linearization included), and are `Double.NaN` when the +record does not define them, so that a threshold of `0` is a threshold. + +> [!NOTE] +> A Compact record that describes several shared sensors is reported as a single sensor +> ([#100](https://github.com/metricshub/ipmi-java/issues/100)). ### States diff --git a/src/site/markdown/supported-commands.md b/src/site/markdown/supported-commands.md index 71359b4..5418f5b 100644 --- a/src/site/markdown/supported-commands.md +++ b/src/site/markdown/supported-commands.md @@ -116,5 +116,9 @@ decodes the FRU information (Platform Management FRU Information Storage Definit `IpmiClient.getFrus()` returns the Chassis, Board and Product areas only. FRUs in another format, such as the SPD data of memory modules, are not decoded ([#107](https://github.com/metricshub/ipmi-java/issues/107)), and are logged and left out. -Known decoding issues of the FRU areas are listed in -[#85](https://github.com/metricshub/ipmi-java/issues/85). + +The decoder checks the common header checksum and rejects a FRU whose header is corrupt. An area +that the data read does not hold in full is skipped and logged, the others are returned. In the +MultiRecord area, a record of a type the library does not model (such as the Extended DC Output +and Extended DC Load records) or of an unknown format version is skipped, and the record that +carries the end-of-list flag is decoded like the others. diff --git a/src/site/markdown/troubleshooting.md b/src/site/markdown/troubleshooting.md index 012b806..4045cab 100644 --- a/src/site/markdown/troubleshooting.md +++ b/src/site/markdown/troubleshooting.md @@ -76,10 +76,10 @@ to make it use suite 3 or 17. | Symptom | Cause | | --- | --- | | `WARN Skipping SDR record ...` | A record that cannot be decoded is skipped; the other sensors are still returned. [SDR records](supported-commands.html#sdr-records) lists what is decoded. | -| `WARN Failed to read FRU at offset ... Requested Sensor, data, or record not present` | The FRU is declared in the SDR repository but not present, for example an empty power supply bay. Usually harmless. | -| `WARN Failed to decode FRU ` | The FRU data is not in the IPMI FRU format (for example the SPD data of a memory module, [#107](https://github.com/metricshub/ipmi-java/issues/107)). | -| A sensor known to `ipmitool` is not returned | Only Full and Compact sensor records of the BMC's own repository are read: sensors behind satellite controllers are not ([#84](https://github.com/metricshub/ipmi-java/issues/84)), and shared Compact records are not expanded ([#100](https://github.com/metricshub/ipmi-java/issues/100)). | -| A sensor reads `0.0` | The BMC flags the reading as unavailable, which is not checked yet ([#110](https://github.com/metricshub/ipmi-java/issues/110)). | +| `WARN Failed to read FRU at offset , the FRU data is truncated there: Requested Sensor, data, or record not present` | The FRU is declared in the SDR repository but not present, for example an empty power supply bay. Usually harmless: the reading stops there and the areas read so far are decoded. | +| `WARN Failed to read FRU ` | The BMC did not answer Get FRU Inventory Area Info for that FRU, or its data is not in the IPMI FRU format (for example the SPD data of a memory module, [#107](https://github.com/metricshub/ipmi-java/issues/107)). The other FRUs are still returned. | +| `WARN The info area at offset is truncated: skipped` | The read stopped before the end of that area (see above): the complete areas of the FRU are still returned. | +| A sensor known to `ipmitool` is not returned | Only Full and Compact sensor records of the BMC's own repository are read: sensors behind satellite controllers are not ([#84](https://github.com/metricshub/ipmi-java/issues/84)), and shared Compact records are not expanded ([#100](https://github.com/metricshub/ipmi-java/issues/100)). A sensor whose reading the BMC flags as unavailable or not scanned (`ipmitool` shows `na` or `disabled`) is returned without reading or states. | | Negative processor temperatures (`CPU1 DTS = -44.0`) | Not an error: Intel *Digital Thermal Sensor* readings are the margin below the maximum junction temperature. | ## Collecting is slow diff --git a/src/site/markdown/upgrading.md b/src/site/markdown/upgrading.md index f2a6919..a34de3b 100644 --- a/src/site/markdown/upgrading.md +++ b/src/site/markdown/upgrading.md @@ -38,6 +38,38 @@ The `IpmiClient` API is unchanged, and the client is more tolerant of real-world ([Troubleshooting](troubleshooting.html#the-login-fails)); only a handshake step that got no reply is sent again. +The decoders follow the IPMI 2.0 and FRU specifications more closely; the visible changes are: + +* a Full Sensor record reports `Double.NaN` for a threshold it does not define, where 1.2.02 + reported `0.0`; the text result now reports a threshold of `0` instead of leaving it empty; +* the thresholds are linearized like the reading, are read whenever byte 12 of the record says + the sensor has thresholds (1.2.02 looked at the wrong byte), and `getAccuracy()` and + `getTolerance()` are decoded as the specification says; `hasAnalogReading()` tells when the + reading byte is not a reading; +* a sensor whose reading the BMC flags as unavailable or not scanned is returned without reading + and without states (1.2.02 reported `0.0`); `GetSensorReadingResponseData.isScanningEnabled()` + exposes the flag; +* the threshold status of a reading (`getSensorState()`) and the states of a threshold sensor + (`getStatesAsserted()`) name the threshold actually crossed (1.2.02 reported an upper + threshold crossing as "below lower non-critical"); +* a completion code the library does not list no longer aborts the decoding of the response: + the command fails with an `IPMIException` whose `getCompletionCode()` is the new + `CompletionCode.Unknown` and whose `getRawCode()` holds the code, and the RMCP+ status codes are + no longer applied to IPMI commands; +* `FruDeviceLocatorRecord.getId()` returns the SDR record ID, as for every other record (1.2.02 + returned the FRU device ID, which `getDeviceId()` returns); +* SEL entries of the OEM record types are decoded with their own layout (`getManufacturerId()`, + `getOemData()`), the record types `C0h` and `E0h` are accepted, and a reserved record type gives + a `SelRecordType.Reserved` entry instead of an exception; +* `BoardInfo.getMfgDate()` is computed in UTC and is `null` when the FRU leaves the date + unspecified (1.2.02 used the JVM time zone and reported 1996-01-01); FRU strings in a + non-English language are decoded as UTF-16LE; a word-addressed FRU is read with 2-byte words; +* `IpmiClient.getFrus()` returns the FRUs it could read even when FRU 0 or Get FRU Inventory Area + Info fails, returns FRU 0 once, and stops reading a FRU at its first unreadable chunk instead of + shifting the following chunks into the gap; +* reserved values of the rate unit, modifier unit usage and power restore policy decode to + `None` or `Unknown` instead of throwing. + Code that **extends** the library's protocol classes needs the changes below. `QueueElement` lost its `isTimedOut()`, `makeTimedOut()` and `refreshTimestamp()` methods: a timed-out message now leaves the queue at once. diff --git a/src/test/java/org/metricshub/ipmi/client/IpmiResultConverterReadingTest.java b/src/test/java/org/metricshub/ipmi/client/IpmiResultConverterReadingTest.java new file mode 100644 index 0000000..3f5affd --- /dev/null +++ b/src/test/java/org/metricshub/ipmi/client/IpmiResultConverterReadingTest.java @@ -0,0 +1,60 @@ +package org.metricshub.ipmi.client; + +import static org.junit.jupiter.api.Assertions.assertEquals; + +import java.util.Collections; + +import org.junit.jupiter.api.Test; +import org.metricshub.ipmi.client.model.Sensor; +import org.metricshub.ipmi.core.coding.commands.sdr.GetSensorReadingResponseData; +import org.metricshub.ipmi.core.coding.commands.sdr.record.EntityId; +import org.metricshub.ipmi.core.coding.commands.sdr.record.FullSensorRecord; +import org.metricshub.ipmi.core.coding.commands.sdr.record.SensorUnit; + +/** + * The reading rows of {@link IpmiResultConverter#convertResult(java.util.List, java.util.List)}: when a reading is + * reported and which thresholds go with it. + */ +class IpmiResultConverterReadingTest { + + private static FullSensorRecord fan(double lowerCritical, double lowerNonCritical) { + FullSensorRecord record = new FullSensorRecord(); + record.setId(1); + record.setEntityId(EntityId.SystemBoard); + record.setEntityInstanceNumber((byte) 1); + record.setName("Fan 1"); + record.setSensorBaseUnit(SensorUnit.Rpm); + record.setLowerCriticalThreshold(lowerCritical); + record.setLowerNonCriticalThreshold(lowerNonCritical); + return record; + } + + private static GetSensorReadingResponseData reading(boolean available, boolean scanned) { + GetSensorReadingResponseData data = new GetSensorReadingResponseData(); + data.setSensorReading((byte) 0); + data.setSensorStateValid(available); + data.setScanningEnabled(scanned); + return data; + } + + private static String convert(FullSensorRecord record, GetSensorReadingResponseData data) { + return IpmiResultConverter + .convertResult(Collections.emptyList(), Collections.singletonList(new Sensor(record, data, Utils.EMPTY))); + } + + @Test + void zeroThresholdIsAThreshold() { + assertEquals("Fan;0001;Fan 1;System Board 1;0.0;0;", convert(fan(0.0, Double.NaN), reading(true, true))); + } + + @Test + void undefinedThresholdIsLeftEmpty() { + assertEquals("Fan;0001;Fan 1;System Board 1;0.0;;", convert(fan(Double.NaN, Double.NaN), reading(true, true))); + } + + @Test + void unavailableOrUnscannedReadingIsNotReported() { + assertEquals(Utils.EMPTY, convert(fan(0.0, 0.0), reading(false, true))); + assertEquals(Utils.EMPTY, convert(fan(0.0, 0.0), reading(true, false))); + } +} diff --git a/src/test/java/org/metricshub/ipmi/client/IpmiResultConverterTest.java b/src/test/java/org/metricshub/ipmi/client/IpmiResultConverterTest.java index f62dd43..60ba290 100644 --- a/src/test/java/org/metricshub/ipmi/client/IpmiResultConverterTest.java +++ b/src/test/java/org/metricshub/ipmi/client/IpmiResultConverterTest.java @@ -20,7 +20,7 @@ import org.metricshub.ipmi.core.coding.commands.sdr.record.FruDeviceLocatorRecord; import org.metricshub.ipmi.core.coding.commands.sdr.record.SensorRecord; -class IpmiResultConverterTest { +public class IpmiResultConverterTest { public static final byte[] BASE_BOARD_PRODUCT_INFO = { 1, @@ -671,6 +671,8 @@ public static List buildSensors() { GetSensorReadingResponseData data = new GetSensorReadingResponseData(); // And sorry... data.setSensorReading((byte) -102); + data.setSensorStateValid(true); + data.setScanningEnabled(true); return Arrays .asList( diff --git a/src/test/java/org/metricshub/ipmi/client/runner/GetFrusRunnerTest.java b/src/test/java/org/metricshub/ipmi/client/runner/GetFrusRunnerTest.java new file mode 100644 index 0000000..1fd11f5 --- /dev/null +++ b/src/test/java/org/metricshub/ipmi/client/runner/GetFrusRunnerTest.java @@ -0,0 +1,20 @@ +package org.metricshub.ipmi.client.runner; + +import static org.junit.jupiter.api.Assertions.assertEquals; + +import java.util.Collections; + +import org.junit.jupiter.api.Test; +import org.metricshub.ipmi.client.IpmiResultConverterTest; +import org.metricshub.ipmi.core.coding.commands.fru.record.ProductInfo; + +class GetFrusRunnerTest { + + @Test + void systemBoardIsNamedAfterWhateverAreaDescribesIt() { + ProductInfo product = new ProductInfo(IpmiResultConverterTest.BASE_BOARD_PRODUCT_INFO, 360); + + assertEquals("System x3650 M2", GetFrusRunner.systemBoardName(Collections.singletonList(product))); + assertEquals("System Board", GetFrusRunner.systemBoardName(Collections.emptyList())); + } +} diff --git a/src/test/java/org/metricshub/ipmi/client/runner/GetSensorsRunnerTest.java b/src/test/java/org/metricshub/ipmi/client/runner/GetSensorsRunnerTest.java index e62cf5b..c360586 100644 --- a/src/test/java/org/metricshub/ipmi/client/runner/GetSensorsRunnerTest.java +++ b/src/test/java/org/metricshub/ipmi/client/runner/GetSensorsRunnerTest.java @@ -4,7 +4,6 @@ import static org.junit.jupiter.api.Assertions.assertThrows; import org.junit.jupiter.api.Test; - import org.metricshub.ipmi.client.Utils; import org.metricshub.ipmi.core.coding.commands.sdr.GetSensorReadingResponseData; import org.metricshub.ipmi.core.coding.commands.sdr.record.CompactSensorRecord; @@ -16,17 +15,25 @@ class GetSensorsRunnerTest { private static final String DEVICE_NAME = "name"; private static final int OEM_EVENT_READING_TYPE = 127; + /** A reading the BMC vouches for: available and scanned. */ + private static GetSensorReadingResponseData validReading() { + final GetSensorReadingResponseData data = new GetSensorReadingResponseData(); + data.setSensorStateValid(true); + data.setScanningEnabled(true); + return data; + } + @Test void testBuildStates() { // check arguments null assertEquals(Utils.EMPTY, GetSensorsRunner.buildStates(null, new CompactSensorRecord())); - assertEquals(Utils.EMPTY, GetSensorsRunner.buildStates(new GetSensorReadingResponseData(), null)); + assertEquals(Utils.EMPTY, GetSensorsRunner.buildStates(validReading(), null)); // check CompactSensorRecord oem type { final byte[] raw = { 1, 2, 3, 127 }; - final GetSensorReadingResponseData data = new GetSensorReadingResponseData(); + final GetSensorReadingResponseData data = validReading(); data.setRaw(raw); final CompactSensorRecord record = new CompactSensorRecord(); record.setName(DEVICE_NAME); @@ -38,7 +45,7 @@ void testBuildStates() { // check CompactSensorRecord { final boolean[] statesAsserted = { true, false }; - final GetSensorReadingResponseData data = new GetSensorReadingResponseData(); + final GetSensorReadingResponseData data = validReading(); data.setStatesAsserted(statesAsserted); final CompactSensorRecord record = new CompactSensorRecord(); @@ -52,7 +59,7 @@ void testBuildStates() { // check FullSensorRecord oem { final byte[] raw = { 1, 2, 3, 127 }; - final GetSensorReadingResponseData data = new GetSensorReadingResponseData(); + final GetSensorReadingResponseData data = validReading(); data.setRaw(raw); final FullSensorRecord record = new FullSensorRecord(); record.setName(DEVICE_NAME); @@ -64,7 +71,7 @@ void testBuildStates() { // check FullSensorRecord { final boolean[] statesAsserted = { true, true }; - final GetSensorReadingResponseData data = new GetSensorReadingResponseData(); + final GetSensorReadingResponseData data = validReading(); data.setStatesAsserted(statesAsserted); final FullSensorRecord record = new FullSensorRecord(); @@ -76,6 +83,25 @@ void testBuildStates() { } } + @Test + void statesOfAnUnavailableOrUnscannedSensorAreNotReported() { + final boolean[] statesAsserted = { true, false }; + final CompactSensorRecord record = new CompactSensorRecord(); + record.setName(DEVICE_NAME); + record.setSensorType(SensorType.PowerUnit); + record.setEventReadingType(7535); + + final GetSensorReadingResponseData unavailable = validReading(); + unavailable.setSensorStateValid(false); + unavailable.setStatesAsserted(statesAsserted); + assertEquals(Utils.EMPTY, GetSensorsRunner.buildStates(unavailable, record)); + + final GetSensorReadingResponseData notScanned = validReading(); + notScanned.setScanningEnabled(false); + notScanned.setStatesAsserted(statesAsserted); + assertEquals(Utils.EMPTY, GetSensorsRunner.buildStates(notScanned, record)); + } + @Test void testBuildOemState() { @@ -99,9 +125,9 @@ void testBuildOemState() { assertEquals("Invalid IPMI raw command date for device name.", exception.getMessage()); } - // check raw length < 4 + // check raw without any state byte { - final byte[] raw = { 1, 2, 3 }; + final byte[] raw = { 1, 2 }; final Exception exception = assertThrows( IllegalArgumentException.class, @@ -110,7 +136,13 @@ void testBuildOemState() { assertEquals("Invalid IPMI raw command date for device name.", exception.getMessage()); } - // check ok + // check one state byte: the second one is optional (IPMI 2.0 Table 35-15) + { + final byte[] raw = { 1, 2, 3 }; + assertEquals("name=0x03", GetSensorsRunner.buildOemState(raw, DEVICE_NAME)); + } + + // check two state bytes { final byte[] raw = { 1, 2, 3, 127 }; assertEquals("name=0x7f03", GetSensorsRunner.buildOemState(raw, DEVICE_NAME)); diff --git a/src/test/java/org/metricshub/ipmi/core/coding/commands/IpmiCommandCoderTest.java b/src/test/java/org/metricshub/ipmi/core/coding/commands/IpmiCommandCoderTest.java index 27da00b..3a75558 100644 --- a/src/test/java/org/metricshub/ipmi/core/coding/commands/IpmiCommandCoderTest.java +++ b/src/test/java/org/metricshub/ipmi/core/coding/commands/IpmiCommandCoderTest.java @@ -2,56 +2,33 @@ import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.metricshub.ipmi.core.coding.commands.IpmiResponses.response; import org.junit.jupiter.api.Test; +import org.metricshub.ipmi.core.coding.commands.fru.BaseUnit; +import org.metricshub.ipmi.core.coding.commands.fru.ReadFruData; import org.metricshub.ipmi.core.coding.commands.sdr.ReserveSdrRepository; import org.metricshub.ipmi.core.coding.commands.sdr.ReserveSdrRepositoryResponseData; +import org.metricshub.ipmi.core.coding.commands.session.CloseSession; +import org.metricshub.ipmi.core.coding.commands.session.GetChannelCipherSuites; import org.metricshub.ipmi.core.coding.payload.CompletionCode; import org.metricshub.ipmi.core.coding.payload.lan.IPMIException; -import org.metricshub.ipmi.core.coding.payload.lan.IpmiLanResponse; import org.metricshub.ipmi.core.coding.protocol.AuthenticationType; -import org.metricshub.ipmi.core.coding.protocol.IpmiMessage; -import org.metricshub.ipmi.core.coding.protocol.Ipmiv15Message; import org.metricshub.ipmi.core.coding.security.CipherSuite; class IpmiCommandCoderTest { + private static final byte CLOSE_SESSION = 0x3c; + private static final ReserveSdrRepository COMMAND = new ReserveSdrRepository( IpmiVersion.V20, CipherSuite.getEmpty(), AuthenticationType.RMCPPlus); - /** - * Build a message wrapping an IPMI LAN response (IPMI 2.0 table 13-5) from the BMC to remote console software. - */ - private static IpmiMessage response(byte command, int completionCode, int... data) { - byte[] raw = new byte[8 + data.length]; - raw[0] = (byte) 0x81; // requester address - raw[1] = (byte) 0x2c; // storage response network function, LUN 0 - raw[2] = (byte) -(0x81 + 0x2c); // checksum 1 - raw[3] = 0x20; // responder address - raw[4] = 0x04; // sequence number 1, LUN 0 - raw[5] = command; - raw[6] = (byte) completionCode; - for (int i = 0; i < data.length; i++) { - raw[7 + i] = (byte) data[i]; - } - int checksum = 0; - for (int i = 3; i < raw.length - 1; i++) { - checksum += raw[i]; - } - raw[raw.length - 1] = (byte) -checksum; // checksum 2 - - IpmiMessage message = new Ipmiv15Message(); - message.setPayload(new IpmiLanResponse(raw)); - return message; - } - @Test void successfulResponseIsDecoded() throws Exception { ReserveSdrRepositoryResponseData data = (ReserveSdrRepositoryResponseData) COMMAND .getResponseData(response(CommandCodes.RESERVE_SDR_REPOSITORY, 0x00, 0x34, 0x12)); - assertEquals(0x1234, data.getReservationId()); } @@ -60,8 +37,8 @@ void errorCompletionCodeIsThrown() { IPMIException e = assertThrows( IPMIException.class, () -> COMMAND.getResponseData(response(CommandCodes.RESERVE_SDR_REPOSITORY, 0xc5))); - assertEquals(CompletionCode.ReservationCanceled, e.getCompletionCode()); + assertEquals(0xc5, e.getRawCode()); } @Test @@ -69,7 +46,70 @@ void responseToAnotherCommandIsRejected() { IllegalArgumentException e = assertThrows( IllegalArgumentException.class, () -> COMMAND.getResponseData(response(CommandCodes.GET_SDR, 0x00, 0x34, 0x12))); - assertEquals("This is not a response for ReserveSdrRepository command", e.getMessage()); } + + @Test + void oemAndCommandSpecificCodesAreReportedWithTheirRawValue() { + IPMIException oem = assertThrows( + IPMIException.class, + () -> COMMAND.getResponseData(response(CommandCodes.RESERVE_SDR_REPOSITORY, 0x8a))); + assertEquals(CompletionCode.Unknown, oem.getCompletionCode()); + assertEquals(0x8a, oem.getRawCode()); + assertEquals("OEM completion code 0x8A.", oem.getMessage()); + + // 0Dh is "Unauthorized name" for RAKP only: for an IPMI command it is a command-specific code + IPMIException specific = assertThrows( + IPMIException.class, + () -> COMMAND.getResponseData(response(CommandCodes.RESERVE_SDR_REPOSITORY, 0x0d))); + assertEquals(CompletionCode.Unknown, specific.getCompletionCode()); + assertEquals("Command-specific completion code 0x0D.", specific.getMessage()); + + IPMIException reserved = assertThrows( + IPMIException.class, + () -> COMMAND.getResponseData(response(CommandCodes.RESERVE_SDR_REPOSITORY, 0xd9))); + assertEquals("Reserved completion code 0xD9.", reserved.getMessage()); + } + + @Test + void readFruDataKnowsItsBusyCode() { + ReadFruData readFruData = new ReadFruData( + IpmiVersion.V20, + CipherSuite.getEmpty(), + AuthenticationType.RMCPPlus, + 0, + BaseUnit.Bytes, + 0, + 16); + IPMIException e = assertThrows( + IPMIException.class, + () -> readFruData.getResponseData(response(CommandCodes.READ_FRU_DATA, 0x81))); + assertEquals(CompletionCode.Frudevicebusy, e.getCompletionCode()); + } + + @Test + void closeSessionKnowsItsSessionCodes() { + CloseSession closeSession = new CloseSession( + IpmiVersion.V20, + CipherSuite.getEmpty(), + AuthenticationType.RMCPPlus, + 0x1234); + IPMIException invalidId = assertThrows( + IPMIException.class, + () -> closeSession.getResponseData(response(CLOSE_SESSION, 0x87))); + assertEquals(CompletionCode.InvalidSessionId, invalidId.getCompletionCode()); + IPMIException invalidHandle = assertThrows( + IPMIException.class, + () -> closeSession.getResponseData(response(CLOSE_SESSION, 0x88))); + assertEquals(CompletionCode.InvalidSessionHandle, invalidHandle.getCompletionCode()); + } + + @Test + void getChannelCipherSuitesChecksTheCompletionCode() { + GetChannelCipherSuites command = new GetChannelCipherSuites((byte) 0x0e, (byte) 0); + IPMIException e = assertThrows( + IPMIException.class, + () -> command.getResponseData(response(CommandCodes.GET_CHANNEL_CIPHER_SUITES, 0xcc))); + assertEquals(CompletionCode.InvalidData, e.getCompletionCode()); + } } diff --git a/src/test/java/org/metricshub/ipmi/core/coding/commands/IpmiResponses.java b/src/test/java/org/metricshub/ipmi/core/coding/commands/IpmiResponses.java new file mode 100644 index 0000000..e317790 --- /dev/null +++ b/src/test/java/org/metricshub/ipmi/core/coding/commands/IpmiResponses.java @@ -0,0 +1,43 @@ +package org.metricshub.ipmi.core.coding.commands; + +import org.metricshub.ipmi.core.coding.payload.lan.IpmiLanResponse; +import org.metricshub.ipmi.core.coding.protocol.IpmiMessage; +import org.metricshub.ipmi.core.coding.protocol.Ipmiv15Message; + +/** + * Builds IPMI LAN responses as a BMC would send them, for the command coder tests. + */ +public final class IpmiResponses { + + private IpmiResponses() {} + + /** + * Build a message wrapping an IPMI LAN response (IPMI 2.0 table 13-5) from the BMC to remote console software. + * + * @param command the command code the response answers + * @param completionCode the completion code byte + * @param data the response data bytes, after the completion code + * @return the message, with valid checksums + */ + public static IpmiMessage response(byte command, int completionCode, int... data) { + byte[] raw = new byte[8 + data.length]; + raw[0] = (byte) 0x81; // requester address + raw[1] = (byte) 0x2c; // storage response network function, LUN 0 + raw[2] = (byte) -(0x81 + 0x2c); // checksum 1 + raw[3] = 0x20; // responder address + raw[4] = 0x04; // sequence number 1, LUN 0 + raw[5] = command; + raw[6] = (byte) completionCode; + for (int i = 0; i < data.length; i++) { + raw[7 + i] = (byte) data[i]; + } + int checksum = 0; + for (int i = 3; i < raw.length - 1; i++) { + checksum += raw[i]; + } + raw[raw.length - 1] = (byte) -checksum; // checksum 2 + IpmiMessage message = new Ipmiv15Message(); + message.setPayload(new IpmiLanResponse(raw)); + return message; + } +} diff --git a/src/test/java/org/metricshub/ipmi/core/coding/commands/chassis/GetChassisStatusResponseDataTest.java b/src/test/java/org/metricshub/ipmi/core/coding/commands/chassis/GetChassisStatusResponseDataTest.java new file mode 100644 index 0000000..935e4f1 --- /dev/null +++ b/src/test/java/org/metricshub/ipmi/core/coding/commands/chassis/GetChassisStatusResponseDataTest.java @@ -0,0 +1,23 @@ +package org.metricshub.ipmi.core.coding.commands.chassis; + +import static org.junit.jupiter.api.Assertions.assertEquals; + +import org.junit.jupiter.api.Test; + +class GetChassisStatusResponseDataTest { + + private static PowerRestorePolicy policy(int currentPowerState) { + GetChassisStatusResponseData data = new GetChassisStatusResponseData(); + data.setCurrentPowerState((byte) currentPowerState); + return data.getPowerRestorePolicy(); + } + + @Test + void everyPowerRestorePolicyValueIsDecoded() { + // IPMI 2.0 Table 28-3, bits [6:5] of the current power state + assertEquals(PowerRestorePolicy.PoweredOff, policy(0x00)); + assertEquals(PowerRestorePolicy.PowerRestored, policy(0x20)); + assertEquals(PowerRestorePolicy.PoweredUp, policy(0x40)); + assertEquals(PowerRestorePolicy.Unknown, policy(0x60)); + } +} diff --git a/src/test/java/org/metricshub/ipmi/core/coding/commands/fru/ReadFruDataTest.java b/src/test/java/org/metricshub/ipmi/core/coding/commands/fru/ReadFruDataTest.java new file mode 100644 index 0000000..4534057 --- /dev/null +++ b/src/test/java/org/metricshub/ipmi/core/coding/commands/fru/ReadFruDataTest.java @@ -0,0 +1,205 @@ +package org.metricshub.ipmi.core.coding.commands.fru; + +import static org.junit.jupiter.api.Assertions.assertArrayEquals; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertInstanceOf; +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertThrows; + +import java.nio.charset.StandardCharsets; +import java.util.Arrays; +import java.util.Collections; +import java.util.Date; +import java.util.List; + +import org.junit.jupiter.api.Test; +import org.metricshub.ipmi.core.coding.commands.fru.record.BaseCompatibilityInfo; +import org.metricshub.ipmi.core.coding.commands.fru.record.BoardInfo; +import org.metricshub.ipmi.core.coding.commands.fru.record.FruRecord; +import org.metricshub.ipmi.core.coding.commands.fru.record.PowerSupplyInfo; +import org.metricshub.ipmi.core.coding.commands.fru.record.ProductInfo; + +class ReadFruDataTest { + + /** 1996-01-01 00:00 UTC, the origin of the manufacturing date. */ + private static final long MFG_DATE_EPOCH_MS = 820454400000L; + + /** 2023-12-18 20:28 UTC, as ipmiutil prints the test board, in minutes since the origin. */ + private static final int MFG_DATE_MINUTES = 14707948; + + private static final int POWER_SUPPLY_RECORD = 0x00; + + private static final int EXTENDED_DC_OUTPUT_RECORD = 0x06; + + private static byte[] bytes(int... values) { + byte[] result = new byte[values.length]; + for (int i = 0; i < values.length; i++) { + result[i] = (byte) values[i]; + } + return result; + } + + private static byte[] concat(byte[]... parts) { + int length = 0; + for (byte[] part : parts) { + length += part.length; + } + byte[] result = new byte[length]; + int offset = 0; + for (byte[] part : parts) { + System.arraycopy(part, 0, result, offset, part.length); + offset += part.length; + } + return result; + } + + private static byte checksum(byte[] data, int offset, int length) { + int sum = 0; + for (int i = offset; i < offset + length; i++) { + sum += data[i]; + } + return (byte) -sum; + } + + /** A type 3 (8-bit ASCII) field: type/length byte, then the characters. */ + private static byte[] text(String value) { + byte[] chars = value.getBytes(StandardCharsets.ISO_8859_1); + return concat(bytes(0xc0 | chars.length), chars); + } + + /** An info area: format version 1, length in 8-byte units, the body, zero padding, checksum. */ + private static byte[] area(byte[] body) { + int length = (2 + body.length + 1 + 7) / 8 * 8; + byte[] area = new byte[length]; + area[0] = 1; + area[1] = (byte) (length / 8); + System.arraycopy(body, 0, area, 2, body.length); + area[length - 1] = checksum(area, 0, length - 1); + return area; + } + + /** A multirecord: 5-byte header (type, end-of-list flag and format version 2, length, checksums), then the data. */ + private static byte[] multirecord(int type, boolean last, byte[] data) { + byte[] header = bytes(type, (last ? 0x80 : 0) | 0x02, data.length, checksum(data, 0, data.length), 0); + header[4] = checksum(header, 0, 4); + return concat(header, data); + } + + private static byte[] boardArea() { + return area( + concat( + bytes(0, MFG_DATE_MINUTES & 0xff, (MFG_DATE_MINUTES >> 8) & 0xff, (MFG_DATE_MINUTES >> 16) & 0xff), + text("IBM"), + text("X123"), + text("S1"), + text("P1"), + bytes(0xc0, 0xc1))); + } + + private static byte[] productArea() { + return area( + concat( + bytes(25), + text("IBM"), + text("Model"), + text("PN"), + text("V1"), + text("SN"), + text("AT"), + bytes(0xc0, 0xc1))); + } + + private static byte[] powerSupplyRecord(boolean last) { + byte[] data = new byte[24]; + data[0] = (byte) 0xee; // 750 W = 0x2EE, least significant byte first + data[1] = 0x02; + return multirecord(POWER_SUPPLY_RECORD, last, data); + } + + /** A FRU image with a board area, a product area and two multirecords, the last one carrying the end flag. */ + private static byte[] image() { + byte[] board = boardArea(); + byte[] product = productArea(); + byte[] multirecords = concat( + multirecord(EXTENDED_DC_OUTPUT_RECORD, false, bytes(1, 2, 3)), + powerSupplyRecord(true)); + byte[] header = bytes(1, 0, 0, 1, 1 + board.length / 8, 1 + (board.length + product.length) / 8, 0, 0); + header[7] = checksum(header, 0, 7); + return concat(header, board, product, multirecords); + } + + private static List decode(byte[] image) { + ReadFruDataResponseData chunk = new ReadFruDataResponseData(); + chunk.setFruData(image); + return ReadFruData.decodeFruData(Collections.singletonList(chunk)); + } + + @Test + void everyAreaAndTheLastMultirecordAreDecoded() { + List records = decode(image()); + + assertEquals(3, records.size(), "board, product and the power supply record; the unknown record is skipped"); + + BoardInfo board = assertInstanceOf(BoardInfo.class, records.get(0)); + assertEquals("IBM", board.getBoardManufacturer()); + assertEquals("X123", board.getBoardProductName()); + assertEquals("S1", board.getBoardSerialNumber()); + assertEquals("P1", board.getBoardPartNumber()); + assertEquals(new Date(MFG_DATE_EPOCH_MS + MFG_DATE_MINUTES * 60000L), board.getMfgDate()); + + ProductInfo product = assertInstanceOf(ProductInfo.class, records.get(1)); + assertEquals("IBM", product.getManufacturerName()); + assertEquals("Model", product.getProductName()); + + PowerSupplyInfo powerSupply = assertInstanceOf(PowerSupplyInfo.class, records.get(2)); + assertEquals(750, powerSupply.getCapacity()); + } + + @Test + void unspecifiedManufacturingDateIsNull() { + byte[] board = area(concat(bytes(0, 0, 0, 0), text("IBM"), bytes(0xc1))); + byte[] header = bytes(1, 0, 0, 1, 0, 0, 0, 0); + header[7] = checksum(header, 0, 7); + + List records = decode(concat(header, board)); + + assertNull(assertInstanceOf(BoardInfo.class, records.get(0)).getMfgDate()); + } + + @Test + void truncatedReadKeepsTheCompleteAreas() { + byte[] image = image(); + byte[] truncated = Arrays.copyOf(image, 8 + boardArea().length + 3); + + List records = decode(truncated); + + assertEquals(1, records.size(), "only the board area is complete"); + assertInstanceOf(BoardInfo.class, records.get(0)); + } + + @Test + void invalidHeaderChecksumIsRejected() { + byte[] image = image(); + image[7] ^= 0x01; + + assertThrows(IllegalArgumentException.class, () -> decode(image)); + } + + @Test + void aWordIsTwoBytes() { + assertEquals(1, BaseUnit.Bytes.getSize()); + assertEquals(2, BaseUnit.Words.getSize()); + } + + @Test + void baseCompatibilityMasksAreReadAtTheRecordOffset() { + byte[] junk = new byte[10]; + // manufacturer ID, entity ID 7 (system board), compatibility base, code start, 3 mask bytes + byte[] record = bytes(0x4c, 0x4c, 0x00, 0x07, 0x01, 0x02, 0xaa, 0xbb, 0xcc); + + BaseCompatibilityInfo info = new BaseCompatibilityInfo(concat(junk, record), junk.length, record.length); + + assertEquals(0x4c4c, info.getManufacturerId()); + assertArrayEquals(bytes(0xaa, 0xbb, 0xcc), info.getCodeRangeMasks()); + } +} diff --git a/src/test/java/org/metricshub/ipmi/core/coding/commands/sdr/GetSdrTest.java b/src/test/java/org/metricshub/ipmi/core/coding/commands/sdr/GetSdrTest.java new file mode 100644 index 0000000..3d02554 --- /dev/null +++ b/src/test/java/org/metricshub/ipmi/core/coding/commands/sdr/GetSdrTest.java @@ -0,0 +1,36 @@ +package org.metricshub.ipmi.core.coding.commands.sdr; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.metricshub.ipmi.core.coding.commands.IpmiResponses.response; + +import org.junit.jupiter.api.Test; +import org.metricshub.ipmi.core.coding.commands.CommandCodes; +import org.metricshub.ipmi.core.coding.commands.IpmiVersion; +import org.metricshub.ipmi.core.coding.protocol.AuthenticationType; +import org.metricshub.ipmi.core.coding.security.CipherSuite; + +class GetSdrTest { + + private static final GetSdr COMMAND = new GetSdr( + IpmiVersion.V20, + CipherSuite.getEmpty(), + AuthenticationType.RMCPPlus, + 0, + 1); + + @Test + void replyWithoutRecordDataStillGivesTheNextRecordId() throws Exception { + GetSdrResponseData data = (GetSdrResponseData) COMMAND + .getResponseData(response(CommandCodes.GET_SDR, 0x00, 0x34, 0x12)); + assertEquals(0x1234, data.getNextRecordId()); + assertEquals(0, data.getSensorRecordData().length); + } + + @Test + void replyShorterThanTheNextRecordIdIsRejected() { + assertThrows( + IllegalArgumentException.class, + () -> COMMAND.getResponseData(response(CommandCodes.GET_SDR, 0x00, 0x34))); + } +} diff --git a/src/test/java/org/metricshub/ipmi/core/coding/commands/sdr/GetSensorReadingTest.java b/src/test/java/org/metricshub/ipmi/core/coding/commands/sdr/GetSensorReadingTest.java new file mode 100644 index 0000000..3f62dd7 --- /dev/null +++ b/src/test/java/org/metricshub/ipmi/core/coding/commands/sdr/GetSensorReadingTest.java @@ -0,0 +1,81 @@ +package org.metricshub.ipmi.core.coding.commands.sdr; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.metricshub.ipmi.core.coding.commands.IpmiResponses.response; + +import java.util.Arrays; + +import org.junit.jupiter.api.Test; +import org.metricshub.ipmi.core.coding.commands.IpmiVersion; +import org.metricshub.ipmi.core.coding.commands.sdr.record.ReadingType; +import org.metricshub.ipmi.core.coding.commands.sdr.record.SensorType; +import org.metricshub.ipmi.core.coding.protocol.AuthenticationType; +import org.metricshub.ipmi.core.coding.security.CipherSuite; + +class GetSensorReadingTest { + + private static final GetSensorReading COMMAND = new GetSensorReading( + IpmiVersion.V20, + CipherSuite.getEmpty(), + AuthenticationType.RMCPPlus, + 5); + + private static final int THRESHOLD_TYPE = 1; + + private static final byte GET_SENSOR_READING = 0x2d; + + private static GetSensorReadingResponseData decode(int... data) throws Exception { + return (GetSensorReadingResponseData) COMMAND + .getResponseData(response(GET_SENSOR_READING, 0x00, data)); + } + + @Test + void upperThresholdCrossingIsReportedAsSuch() throws Exception { + // Table 35-15: byte 2 bit 6 scanning enabled, byte 3 bit 3 "at or above upper non-critical" + GetSensorReadingResponseData data = decode(0x64, 0x40, 0x08); + + assertEquals(100, data.getPlainSensorReading(), 0); + assertTrue(data.isSensorStateValid()); + assertTrue(data.isScanningEnabled()); + assertEquals(SensorState.AboveUpperNonCritical, data.getSensorState()); + assertEquals( + Arrays.asList(ReadingType.UpperNonCriticalGoingHigh), + data.getStatesAsserted(SensorType.Temperature, THRESHOLD_TYPE)); + } + + @Test + void everyThresholdBitMapsToItsEvent() throws Exception { + // LNC (bit 0) and UC (bit 4) at the same time + GetSensorReadingResponseData data = decode(0x64, 0x40, 0x11); + + assertEquals(SensorState.AboveUpperCritical, data.getSensorState()); + assertEquals( + Arrays.asList(ReadingType.LowerNonCriticalGoingLow, ReadingType.UpperCriticalGoingHigh), + data.getStatesAsserted(SensorType.Temperature, THRESHOLD_TYPE)); + } + + @Test + void unavailableReadingAndDisabledScanningAreExposed() throws Exception { + // bit 5: reading/state unavailable + GetSensorReadingResponseData unavailable = decode(0x00, 0x60, 0x00); + assertFalse(unavailable.isSensorStateValid()); + assertTrue(unavailable.isScanningEnabled()); + + GetSensorReadingResponseData notScanned = decode(0x00, 0x00, 0x00); + assertTrue(notScanned.isSensorStateValid()); + assertFalse(notScanned.isScanningEnabled()); + } + + @Test + void sensorStateIsTheMostSevereThresholdCrossed() { + assertEquals(SensorState.Ok, SensorState.parseInt(0x00)); + assertEquals(SensorState.BelowLowerNonCritical, SensorState.parseInt(0x01)); + assertEquals(SensorState.BelowLowerCritical, SensorState.parseInt(0x03)); + assertEquals(SensorState.BelowLowerNonRecoverable, SensorState.parseInt(0x07)); + assertEquals(SensorState.AboveUpperNonCritical, SensorState.parseInt(0x08)); + assertEquals(SensorState.AboveUpperCritical, SensorState.parseInt(0x18)); + assertEquals(SensorState.AboveUpperNonRecoverable, SensorState.parseInt(0x3f)); + } +} diff --git a/src/test/java/org/metricshub/ipmi/core/coding/commands/sdr/record/FullSensorRecordTest.java b/src/test/java/org/metricshub/ipmi/core/coding/commands/sdr/record/FullSensorRecordTest.java new file mode 100644 index 0000000..7b8d9e4 --- /dev/null +++ b/src/test/java/org/metricshub/ipmi/core/coding/commands/sdr/record/FullSensorRecordTest.java @@ -0,0 +1,106 @@ +package org.metricshub.ipmi.core.coding.commands.sdr.record; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertInstanceOf; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import org.junit.jupiter.api.Test; + +class FullSensorRecordTest { + + private static final double DELTA = 1e-6; + + /** + * A full sensor record (IPMI 2.0 Table 43-1) whose body starts at byte 10; bytes 5-9 hold the sensor key and + * entity. + */ + private static byte[] fullRecord(int... body) { + int[] head = { 0x42, 0x00, 0x51, 0x01, 0, 0x40, 0x00, 0x01, 0x07, 0x01 }; + byte[] result = new byte[head.length + body.length]; + for (int i = 0; i < head.length; i++) { + result[i] = (byte) head[i]; + } + for (int i = 0; i < body.length; i++) { + result[head.length + i] = (byte) body[i]; + } + result[4] = (byte) (result.length - 5); + return result; + } + + // @formatter:off + private static final byte[] EXPONENTIAL_TEMPERATURE = fullRecord( + 0x00, // 10: initialization: "init sensor type" (bit 2) clear, which has nothing to do with thresholds + 0x24, // 11: capabilities: thresholds readable + 0x01, 0x01, // 12-13: temperature sensor, threshold reading type + 0x00, 0x00, 0x00, 0x00, 0x38, 0x00, // 14-19: masks: UNR, UC and UNC readable, lower thresholds not + 0x00, 0x01, 0x00, // 20-22: unsigned, degrees C, no modifier unit + 0x04, // 23: linearization e^x + 0x01, 0x04, 0x00, 0x01, 0x08, 0x00, // 24-29: M=1, tolerance 4, B=0, accuracy 1 x 10^2, R exp 0, B exp 0 + 0x00, // 30: analog characteristics + 0x02, 0x00, 0x00, 0x00, 0x00, // 31-35: nominal 2, normal max/min, sensor max/min + 0x05, 0x04, 0x03, 0x00, 0x00, 0x00, // 36-41: UNR 5, UC 4, UNC 3, lower thresholds unreadable + 0x00, 0x00, 0x00, 0x00, 0x00, // 42-46: hysteresis, reserved, OEM + 0xc3, 'C', 'P', 'U'); // 47-50: 8-bit ASCII ID string + + private static final byte[] NO_THRESHOLDS_NO_READING = fullRecord( + 0x04, // 10: initialization: "init sensor type" set, which is not "thresholds present" + 0x20, // 11: capabilities: no thresholds (bits [3:2] = 00b) + 0x01, 0x01, // 12-13: temperature sensor, threshold reading type + 0x00, 0x00, 0x00, 0x00, 0x3f, 0x00, // 14-19: masks claim every threshold readable + 0xc0, 0x01, 0x00, // 20-22: no analog reading (data format 11b), degrees C + 0x00, // 23: linear + 0x01, 0x00, 0x00, 0x00, 0x00, 0x00, // 24-29: M=1 + 0x00, // 30 + 0x00, 0x00, 0x00, 0x00, 0x00, // 31-35 + 0x05, 0x04, 0x03, 0x02, 0x01, 0x00, // 36-41: threshold bytes that must be ignored + 0x00, 0x00, 0x00, 0x00, 0x00, // 42-46 + 0xc3, 'C', 'P', 'U'); // 47-50 + // @formatter:on + + @Test + void thresholdsAreGatedOnByte12AndLinearizedLikeTheReading() { + FullSensorRecord record = assertInstanceOf( + FullSensorRecord.class, + SensorRecord.populateSensorRecord(EXPONENTIAL_TEMPERATURE)); + + assertTrue(record.hasAnalogReading()); + assertEquals(Math.exp(2), record.getNominalReading(), DELTA); + assertEquals(Math.exp(5), record.getUpperNonRecoverableThreshold(), DELTA); + assertEquals(Math.exp(4), record.getUpperCriticalThreshold(), DELTA); + assertEquals(Math.exp(3), record.getUpperNonCriticalThreshold(), DELTA); + assertTrue(Double.isNaN(record.getLowerNonRecoverableThreshold()), "an unreadable threshold is NaN"); + assertTrue(Double.isNaN(record.getLowerCriticalThreshold())); + assertTrue(Double.isNaN(record.getLowerNonCriticalThreshold())); + assertEquals(Math.exp(7), record.calcFormula(7), DELTA); + } + + @Test + void accuracyExponentAndToleranceAreDecoded() { + FullSensorRecord record = (FullSensorRecord) SensorRecord.populateSensorRecord(EXPONENTIAL_TEMPERATURE); + + // accuracy 1 (in 1/100 %) x 10^2 + assertEquals(0.01, record.getAccuracy(), DELTA); + // tolerance 4 half raw counts: 4 / 2 x |M| x 10^R + assertEquals(2.0, record.getTolerance(), DELTA); + } + + @Test + void recordWithoutThresholdsOrAnalogReadingSaysSo() { + FullSensorRecord record = (FullSensorRecord) SensorRecord.populateSensorRecord(NO_THRESHOLDS_NO_READING); + + assertFalse(record.hasAnalogReading()); + assertTrue(Double.isNaN(record.getUpperNonRecoverableThreshold())); + assertTrue(Double.isNaN(record.getUpperCriticalThreshold())); + assertTrue(Double.isNaN(record.getUpperNonCriticalThreshold())); + assertTrue(Double.isNaN(record.getLowerNonRecoverableThreshold())); + assertTrue(Double.isNaN(record.getLowerCriticalThreshold())); + assertTrue(Double.isNaN(record.getLowerNonCriticalThreshold())); + } + + @Test + void reservedUnitValuesDoNotThrow() { + assertEquals(RateUnit.None, RateUnit.parseInt(7)); + assertEquals(ModifierUnitUsage.None, ModifierUnitUsage.parseInt(3)); + } +} diff --git a/src/test/java/org/metricshub/ipmi/core/coding/commands/sdr/record/LocatorRecordTest.java b/src/test/java/org/metricshub/ipmi/core/coding/commands/sdr/record/LocatorRecordTest.java new file mode 100644 index 0000000..3830040 --- /dev/null +++ b/src/test/java/org/metricshub/ipmi/core/coding/commands/sdr/record/LocatorRecordTest.java @@ -0,0 +1,64 @@ +package org.metricshub.ipmi.core.coding.commands.sdr.record; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertInstanceOf; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import org.junit.jupiter.api.Test; + +class LocatorRecordTest { + + private static final int RECORD_ID = 0x0042; + + /** An SDR record: ID, SDR version 1.5, type, length, payload. */ + private static byte[] record(int type, int... payload) { + byte[] result = new byte[5 + payload.length]; + result[0] = (byte) RECORD_ID; + result[1] = (byte) (RECORD_ID >> 8); + result[2] = 0x51; + result[3] = (byte) type; + result[4] = (byte) payload.length; + for (int i = 0; i < payload.length; i++) { + result[5 + i] = (byte) payload[i]; + } + return result; + } + + @Test + void fruDeviceLocatorKeepsItsRecordIdAndReadsTheLunFromBits4And3() { + // IPMI 2.0 Table 43-7: access address 20h, device ID 3, logical + LUN 1 + bus 2, channel 1, reserved, FRU + // inventory device, modifier, system board, instance 1, OEM, 8-bit ASCII ID string + FruDeviceLocatorRecord locator = assertInstanceOf( + FruDeviceLocatorRecord.class, + SensorRecord + .populateSensorRecord( + record(0x11, 0x40, 0x03, 0x8a, 0x10, 0x00, 0x10, 0x00, 0x07, 0x01, 0x00, 0xc3, 'F', 'R', 'U'))); + + assertEquals(RECORD_ID, locator.getId(), "the SDR record ID, not the device ID"); + assertEquals(3, locator.getDeviceId()); + assertTrue(locator.isLogical()); + assertEquals(1, locator.getAccessLun()); + assertEquals(0x20, locator.getDeviceAccessAddress()); + assertEquals(7, locator.getFruEntityId()); + assertEquals(1, locator.getFruEntityInstance()); + assertEquals("FRU", locator.getName()); + } + + @Test + void genericDeviceLocatorReadsBusSpanAndNameWhereTheSpecPutsThem() { + // IPMI 2.0 Table 43-10: access address 20h, slave address 21h, LUN 1 + bus 5, address span 7, reserved, + // device type, modifier, system board, instance 1, OEM, 8-bit ASCII ID string at byte 16 + GenericDeviceLocatorRecord locator = assertInstanceOf( + GenericDeviceLocatorRecord.class, + SensorRecord + .populateSensorRecord( + record(0x10, 0x40, 0x42, 0x0d, 0x07, 0x00, 0x10, 0x00, 0x07, 0x01, 0x00, 0xc4, 'N', 'A', 'M', 'E'))); + + assertEquals(RECORD_ID, locator.getId()); + assertEquals(0x21, locator.getDeviceSlaveAddress()); + assertEquals(1, locator.getAccessLun()); + assertEquals(5, locator.getBusId()); + assertEquals(7, locator.getAddressSpan()); + assertEquals("NAME", locator.getName()); + } +} diff --git a/src/test/java/org/metricshub/ipmi/core/coding/commands/sel/SelRecordTest.java b/src/test/java/org/metricshub/ipmi/core/coding/commands/sel/SelRecordTest.java new file mode 100644 index 0000000..35b5a39 --- /dev/null +++ b/src/test/java/org/metricshub/ipmi/core/coding/commands/sel/SelRecordTest.java @@ -0,0 +1,89 @@ +package org.metricshub.ipmi.core.coding.commands.sel; + +import static org.junit.jupiter.api.Assertions.assertArrayEquals; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNull; + +import java.util.Date; + +import org.junit.jupiter.api.Test; +import org.metricshub.ipmi.core.coding.commands.sdr.record.ReadingType; +import org.metricshub.ipmi.core.coding.commands.sdr.record.SensorType; + +class SelRecordTest { + + private static byte[] bytes(int... values) { + byte[] result = new byte[values.length]; + for (int i = 0; i < values.length; i++) { + result[i] = (byte) values[i]; + } + return result; + } + + private static final int TIMESTAMP = 0x40302010; + + @Test + void systemEventRecordIsDecoded() { + // IPMI 2.0 section 32.1: record 1, type 02h, timestamp, generator 0020h, EvM rev 04h, temperature sensor 5, + // assertion of threshold event offset 7 (upper non-critical going high), event data + SelRecord record = SelRecord + .populateSelRecord( + bytes(0x01, 0x00, 0x02, 0x10, 0x20, 0x30, 0x40, 0x20, 0x00, 0x04, 0x01, 0x05, 0x01, 0x07, 0x00, 0x00)); + + assertEquals(1, record.getRecordId()); + assertEquals(SelRecordType.System, record.getRecordType()); + assertEquals(new Date(TIMESTAMP * 1000L), record.getTimestamp()); + assertEquals(SensorType.Temperature, record.getSensorType()); + assertEquals(5, record.getSensorNumber()); + assertEquals(ReadingType.UpperNonCriticalGoingHigh, record.getEvent()); + assertNull(record.getManufacturerId()); + assertNull(record.getOemData()); + } + + @Test + void oemTimestampedRecordKeepsItsManufacturerIdAndData() { + // Section 32.2: type C0h, timestamp, manufacturer ID 0x004C4C (3 bytes, LS first), 6 OEM bytes + SelRecord record = SelRecord + .populateSelRecord(bytes(0x02, 0x00, 0xc0, 0x10, 0x20, 0x30, 0x40, 0x4c, 0x4c, 0x00, 1, 2, 3, 4, 5, 6)); + + assertEquals(SelRecordType.OemTimestamped, record.getRecordType()); + assertEquals(new Date(TIMESTAMP * 1000L), record.getTimestamp()); + assertEquals(0x4c4c, record.getManufacturerId()); + assertArrayEquals(bytes(1, 2, 3, 4, 5, 6), record.getOemData()); + assertNull(record.getSensorType(), "an OEM record has no system event fields"); + assertNull(record.getEvent()); + assertNull(record.getEventDirection()); + } + + @Test + void oemNonTimestampedRecordKeepsItsData() { + // Section 32.3: type E0h, 13 OEM bytes + SelRecord record = SelRecord + .populateSelRecord(bytes(0x03, 0x00, 0xe0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13)); + + assertEquals(SelRecordType.OemNonTimestamped, record.getRecordType()); + assertNull(record.getTimestamp()); + assertNull(record.getManufacturerId()); + assertArrayEquals(bytes(1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13), record.getOemData()); + } + + @Test + void recordTypeRangesIncludeTheirBoundaries() { + assertEquals(SelRecordType.OemTimestamped, SelRecordType.parseInt(0xc0)); + assertEquals(SelRecordType.OemTimestamped, SelRecordType.parseInt(0xdf)); + assertEquals(SelRecordType.OemNonTimestamped, SelRecordType.parseInt(0xe0)); + assertEquals(SelRecordType.OemNonTimestamped, SelRecordType.parseInt(0xff)); + assertEquals(SelRecordType.Reserved, SelRecordType.parseInt(0x10)); + } + + @Test + void reservedRecordTypeDoesNotAbortTheWalk() { + SelRecord record = SelRecord + .populateSelRecord(bytes(0x04, 0x00, 0x10, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13)); + + assertEquals(4, record.getRecordId()); + assertEquals(SelRecordType.Reserved, record.getRecordType()); + assertNull(record.getTimestamp()); + assertNull(record.getOemData()); + } +} From 2543486a176d2fb9dfd2b3940b67e1be484f5a12 Mon Sep 17 00:00:00 2001 From: Bertrand Martin Date: Fri, 9 Oct 2026 10:19:55 +0200 Subject: [PATCH 2/7] Address the Codex review of the decoder fixes - Deactivate Payload reads its command-specific codes from the raw byte and reports it on the exception. - Completion-code labels follow Table 5-2: 01h-7Eh OEM, 80h-BEh command-specific, 7Fh/BFh/D7h-FEh reserved. - Non-linear sensors (70h-7Fh) and reserved linearizations give NaN, and hasAnalogReading() is false for them: their conversion needs Get Sensor Reading Factors, which the library does not implement. - Javadoc reattached to calcFormula(int); Javadoc on the new setters. Co-Authored-By: Claude Fable 5.1 --- .../coding/commands/IpmiCommandCoder.java | 2 +- .../commands/payload/DeactivatePayload.java | 9 +++--- .../sdr/GetSensorReadingResponseData.java | 3 ++ .../commands/sdr/record/FullSensorRecord.java | 32 ++++++++++++------- .../core/coding/commands/sel/SelRecord.java | 6 ++++ .../core/coding/payload/CompletionCode.java | 4 +-- .../coding/payload/lan/IPMIException.java | 14 ++++++-- .../coding/payload/lan/IpmiLanResponse.java | 2 +- src/site/markdown/upgrading.md | 4 ++- .../coding/commands/IpmiCommandCoderTest.java | 12 ++++--- .../sdr/record/FullSensorRecordTest.java | 12 +++++++ 11 files changed, 73 insertions(+), 27 deletions(-) diff --git a/src/main/java/org/metricshub/ipmi/core/coding/commands/IpmiCommandCoder.java b/src/main/java/org/metricshub/ipmi/core/coding/commands/IpmiCommandCoder.java index 4c323c4..e378cb7 100644 --- a/src/main/java/org/metricshub/ipmi/core/coding/commands/IpmiCommandCoder.java +++ b/src/main/java/org/metricshub/ipmi/core/coding/commands/IpmiCommandCoder.java @@ -98,7 +98,7 @@ protected byte[] validateResponse(IpmiMessage message) throws IPMIException { } /** - * Gives a meaning to a command-specific or OEM completion code (01h-7Eh and 80h-BEh, IPMI 2.0 Table 5-2) of this + * Gives a meaning to an OEM or command-specific completion code (01h-7Eh and 80h-BEh, IPMI 2.0 Table 5-2) of this * command. The default knows none of them. * * @param rawCode the completion code byte of the response diff --git a/src/main/java/org/metricshub/ipmi/core/coding/commands/payload/DeactivatePayload.java b/src/main/java/org/metricshub/ipmi/core/coding/commands/payload/DeactivatePayload.java index 5146065..70d208d 100644 --- a/src/main/java/org/metricshub/ipmi/core/coding/commands/payload/DeactivatePayload.java +++ b/src/main/java/org/metricshub/ipmi/core/coding/commands/payload/DeactivatePayload.java @@ -117,16 +117,17 @@ public ResponseData getResponseData(IpmiMessage message) throw new IllegalArgumentException("Invalid response payload"); } - CompletionCode completionCode = ((IpmiLanResponse) message.getPayload()).getCompletionCode(); + IpmiLanResponse response = (IpmiLanResponse) message.getPayload(); - if (completionCode != CompletionCode.Ok) { + if (response.getCompletionCode() != CompletionCode.Ok) { + // The command-specific codes (IPMI 2.0 Table 24-8) are read from the raw byte DeactivatePayloadCompletionCode specificCompletionCode = DeactivatePayloadCompletionCode - .parseInt(completionCode.getCode()); + .parseInt(response.getRawCompletionCode()); if (specificCompletionCode == DeactivatePayloadCompletionCode.PAYLOAD_ALREADY_DEACTIVATED) { LOGGER.warn(specificCompletionCode.getMessage()); } else { - throw new IPMIException(((IpmiLanResponse) message.getPayload()).getCompletionCode()); + throw new IPMIException(response.getCompletionCode(), response.getRawCompletionCode()); } } diff --git a/src/main/java/org/metricshub/ipmi/core/coding/commands/sdr/GetSensorReadingResponseData.java b/src/main/java/org/metricshub/ipmi/core/coding/commands/sdr/GetSensorReadingResponseData.java index 63d1ec5..590c256 100644 --- a/src/main/java/org/metricshub/ipmi/core/coding/commands/sdr/GetSensorReadingResponseData.java +++ b/src/main/java/org/metricshub/ipmi/core/coding/commands/sdr/GetSensorReadingResponseData.java @@ -100,6 +100,9 @@ public boolean isScanningEnabled() { return scanningEnabled; } + /** + * @param scanningEnabled whether the BMC reports sensor scanning as enabled (byte 2 bit 6) + */ public void setScanningEnabled(boolean scanningEnabled) { this.scanningEnabled = scanningEnabled; } diff --git a/src/main/java/org/metricshub/ipmi/core/coding/commands/sdr/record/FullSensorRecord.java b/src/main/java/org/metricshub/ipmi/core/coding/commands/sdr/record/FullSensorRecord.java index c716718..ef46d37 100644 --- a/src/main/java/org/metricshub/ipmi/core/coding/commands/sdr/record/FullSensorRecord.java +++ b/src/main/java/org/metricshub/ipmi/core/coding/commands/sdr/record/FullSensorRecord.java @@ -68,6 +68,9 @@ public class FullSensorRecord extends AbstractSensorRecord { private static final int NO_ANALOG_READING = 3; + /** Last linearization code (cube root, Table 43-1 byte 24) the library can apply. */ + private static final int LAST_LINEARIZABLE = 0x0b; + @Override protected void populateTypeSpecficValues( byte[] recordData, @@ -309,22 +312,27 @@ public void setLowerNonCriticalThreshold(double lowerNonCriticalThreshold) { this.lowerNonCriticalThreshold = lowerNonCriticalThreshold; } + /** + * Tells whether the reading byte of this sensor converts to a value: false when the data format of Sensor Units 1 + * is 11b (no analog reading, Table 43-1), or when the linearization is non-linear (70h-7Fh) or reserved, as the + * conversion then needs the Get Sensor Reading Factors command, which the library does not implement. + * + * @return whether {@link #calcFormula(int)} gives a meaningful value + */ + public boolean hasAnalogReading() { + return ((TypeConverter.byteToInt(sensorUnits1) & 0xc0) >> 6) != NO_ANALOG_READING + && linearization <= LAST_LINEARIZABLE; + } + /** * Converts to units-based value using the 'y=Mx+B' formula. 1's or 2's * complement signed or unsigned per flag bits in Sensor Units 1. * * @param value * - Value to be converted. Length of 8 is assumed. - * @return converted value - */ - /** - * @return false when the data format of Sensor Units 1 is 11b (Table 43-1): the sensor has no analog reading and - * the reading byte must not be converted + * @return converted value, {@link Double#NaN} when the sensor has no analog reading + * ({@link #hasAnalogReading()}) */ - public boolean hasAnalogReading() { - return ((TypeConverter.byteToInt(sensorUnits1) & 0xc0) >> 6) != NO_ANALOG_READING; - } - public double calcFormula(int value) { return calcFormula(value, 8, sensorUnits1); } @@ -395,9 +403,9 @@ protected double calcFormula(int value, int length, byte units1) { case 11: return Math.cbrt(result); default: - // 70h-7Fh are non-linear (the linearization needs Get Sensor Reading Factors), the rest is reserved: - // return the linear conversion, as ipmitool does, rather than drop the sensor - return result; + // 70h-7Fh are non-linear: the SDR factors hold at the nominal reading only and the conversion needs + // Get Sensor Reading Factors, which the library does not implement; the rest is reserved + return Double.NaN; } } diff --git a/src/main/java/org/metricshub/ipmi/core/coding/commands/sel/SelRecord.java b/src/main/java/org/metricshub/ipmi/core/coding/commands/sel/SelRecord.java index a3cd0c5..322eb0c 100644 --- a/src/main/java/org/metricshub/ipmi/core/coding/commands/sel/SelRecord.java +++ b/src/main/java/org/metricshub/ipmi/core/coding/commands/sel/SelRecord.java @@ -137,6 +137,9 @@ public Integer getManufacturerId() { return manufacturerId; } + /** + * @param manufacturerId the manufacturer ID of an OEM timestamped record, null for the other record types + */ public void setManufacturerId(Integer manufacturerId) { this.manufacturerId = manufacturerId; } @@ -149,6 +152,9 @@ public byte[] getOemData() { return oemData; } + /** + * @param oemData the OEM-defined bytes of an OEM record, null for the other record types + */ public void setOemData(byte[] oemData) { this.oemData = oemData; } diff --git a/src/main/java/org/metricshub/ipmi/core/coding/payload/CompletionCode.java b/src/main/java/org/metricshub/ipmi/core/coding/payload/CompletionCode.java index 082d4b7..19d8866 100644 --- a/src/main/java/org/metricshub/ipmi/core/coding/payload/CompletionCode.java +++ b/src/main/java/org/metricshub/ipmi/core/coding/payload/CompletionCode.java @@ -218,7 +218,7 @@ public enum CompletionCode { */ InvalidRole(CompletionCode.INVALIDROLE), /** - * A code this enumeration does not list: command-specific, OEM or reserved. The raw value is available on the + * A code this enumeration does not list: OEM, command-specific or reserved. The raw value is available on the * {@link org.metricshub.ipmi.core.coding.payload.lan.IPMIException}. */ Unknown(CompletionCode.UNKNOWN), @@ -387,7 +387,7 @@ public static CompletionCode parseInt(int value) { case INVALIDROLE: return InvalidRole; default: - // IPMI 2.0 Table 5-2 lets every command use command-specific (01h-7Eh) and OEM (80h-BEh) codes, and + // IPMI 2.0 Table 5-2 lets every command use OEM (01h-7Eh) and command-specific (80h-BEh) codes, and // reserves the rest: a code this table does not list must not abort the decoding of the response return Unknown; } diff --git a/src/main/java/org/metricshub/ipmi/core/coding/payload/lan/IPMIException.java b/src/main/java/org/metricshub/ipmi/core/coding/payload/lan/IPMIException.java index 51fc36a..d9f738e 100644 --- a/src/main/java/org/metricshub/ipmi/core/coding/payload/lan/IPMIException.java +++ b/src/main/java/org/metricshub/ipmi/core/coding/payload/lan/IPMIException.java @@ -28,7 +28,8 @@ public class IPMIException extends Exception { private static final long serialVersionUID = 1L; - private static final int OEM_CODES_START = 0x80; + /** First command-specific code (IPMI 2.0 Table 5-2): 01h-7Eh are OEM, 80h-BEh command-specific. */ + private static final int COMMAND_SPECIFIC_CODES_START = 0x80; private static final int GENERIC_CODES_START = 0xC0; @@ -50,6 +51,13 @@ public IPMIException(CompletionCode completionCode, int rawCode) { this.rawCode = rawCode; } + /** + * @return whether the code is one Table 5-2 reserves: 7Fh, BFh, D7h-FEh + */ + private static boolean isReserved(int rawCode) { + return rawCode == 0x7f || rawCode == 0xbf || (rawCode >= 0xd7 && rawCode <= 0xfe); + } + public CompletionCode getCompletionCode() { return completionCode; } @@ -65,7 +73,9 @@ public int getRawCode() { @Override public String getMessage() { if (completionCode == CompletionCode.Unknown && rawCode >= 0) { - String kind = rawCode < OEM_CODES_START ? "Command-specific" : rawCode < GENERIC_CODES_START ? "OEM" : "Reserved"; + String kind = isReserved(rawCode) ? + "Reserved" : rawCode < COMMAND_SPECIFIC_CODES_START ? + "OEM" : rawCode < GENERIC_CODES_START ? "Command-specific" : "Generic"; return String.format("%s completion code 0x%02X.", kind, rawCode); } return completionCode.getMessage(); diff --git a/src/main/java/org/metricshub/ipmi/core/coding/payload/lan/IpmiLanResponse.java b/src/main/java/org/metricshub/ipmi/core/coding/payload/lan/IpmiLanResponse.java index 9bcc96f..98fabcf 100644 --- a/src/main/java/org/metricshub/ipmi/core/coding/payload/lan/IpmiLanResponse.java +++ b/src/main/java/org/metricshub/ipmi/core/coding/payload/lan/IpmiLanResponse.java @@ -38,7 +38,7 @@ public class IpmiLanResponse extends IpmiLanMessage { /** * Decodes the completion code. Only 00h and the generic codes (C0h-FFh, IPMI 2.0 Table 5-2) have a meaning - * common to every command; a command-specific (01h-7Eh) or OEM (80h-BEh) code is {@link CompletionCode#Unknown} + * common to every command; an OEM (01h-7Eh) or command-specific (80h-BEh) code is {@link CompletionCode#Unknown} * here, and the command coder may give it its own meaning. * * @param completionCode the completion code byte of the response diff --git a/src/site/markdown/upgrading.md b/src/site/markdown/upgrading.md index a34de3b..968c746 100644 --- a/src/site/markdown/upgrading.md +++ b/src/site/markdown/upgrading.md @@ -45,7 +45,9 @@ The decoders follow the IPMI 2.0 and FRU specifications more closely; the visibl * the thresholds are linearized like the reading, are read whenever byte 12 of the record says the sensor has thresholds (1.2.02 looked at the wrong byte), and `getAccuracy()` and `getTolerance()` are decoded as the specification says; `hasAnalogReading()` tells when the - reading byte is not a reading; + reading byte is not a reading, which includes the non-linear sensors (linearization `70h`-`7Fh`), + whose conversion needs the Get Sensor Reading Factors command the library does not implement: + their reading and thresholds are `NaN` where 1.2.02 dropped the sensor; * a sensor whose reading the BMC flags as unavailable or not scanned is returned without reading and without states (1.2.02 reported `0.0`); `GetSensorReadingResponseData.isScanningEnabled()` exposes the flag; diff --git a/src/test/java/org/metricshub/ipmi/core/coding/commands/IpmiCommandCoderTest.java b/src/test/java/org/metricshub/ipmi/core/coding/commands/IpmiCommandCoderTest.java index 3a75558..d6bd955 100644 --- a/src/test/java/org/metricshub/ipmi/core/coding/commands/IpmiCommandCoderTest.java +++ b/src/test/java/org/metricshub/ipmi/core/coding/commands/IpmiCommandCoderTest.java @@ -50,25 +50,29 @@ void responseToAnotherCommandIsRejected() { } @Test - void oemAndCommandSpecificCodesAreReportedWithTheirRawValue() { + void oemCommandSpecificAndReservedCodesAreReportedWithTheirRawValue() { IPMIException oem = assertThrows( IPMIException.class, () -> COMMAND.getResponseData(response(CommandCodes.RESERVE_SDR_REPOSITORY, 0x8a))); assertEquals(CompletionCode.Unknown, oem.getCompletionCode()); assertEquals(0x8a, oem.getRawCode()); - assertEquals("OEM completion code 0x8A.", oem.getMessage()); + assertEquals("Command-specific completion code 0x8A.", oem.getMessage()); - // 0Dh is "Unauthorized name" for RAKP only: for an IPMI command it is a command-specific code + // 0Dh is "Unauthorized name" for RAKP only: for an IPMI command it is an OEM (device-specific) code IPMIException specific = assertThrows( IPMIException.class, () -> COMMAND.getResponseData(response(CommandCodes.RESERVE_SDR_REPOSITORY, 0x0d))); assertEquals(CompletionCode.Unknown, specific.getCompletionCode()); - assertEquals("Command-specific completion code 0x0D.", specific.getMessage()); + assertEquals("OEM completion code 0x0D.", specific.getMessage()); IPMIException reserved = assertThrows( IPMIException.class, () -> COMMAND.getResponseData(response(CommandCodes.RESERVE_SDR_REPOSITORY, 0xd9))); assertEquals("Reserved completion code 0xD9.", reserved.getMessage()); + IPMIException boundary = assertThrows( + IPMIException.class, + () -> COMMAND.getResponseData(response(CommandCodes.RESERVE_SDR_REPOSITORY, 0xbf))); + assertEquals("Reserved completion code 0xBF.", boundary.getMessage()); } @Test diff --git a/src/test/java/org/metricshub/ipmi/core/coding/commands/sdr/record/FullSensorRecordTest.java b/src/test/java/org/metricshub/ipmi/core/coding/commands/sdr/record/FullSensorRecordTest.java index 7b8d9e4..1eae321 100644 --- a/src/test/java/org/metricshub/ipmi/core/coding/commands/sdr/record/FullSensorRecordTest.java +++ b/src/test/java/org/metricshub/ipmi/core/coding/commands/sdr/record/FullSensorRecordTest.java @@ -98,6 +98,18 @@ void recordWithoutThresholdsOrAnalogReadingSaysSo() { assertTrue(Double.isNaN(record.getLowerNonCriticalThreshold())); } + @Test + void nonLinearSensorHasNoComputableReading() { + byte[] nonLinear = EXPONENTIAL_TEMPERATURE.clone(); + nonLinear[23] = 0x70; // linearization: non-linear, the factors hold at the nominal reading only + + FullSensorRecord record = (FullSensorRecord) SensorRecord.populateSensorRecord(nonLinear); + + assertFalse(record.hasAnalogReading()); + assertTrue(Double.isNaN(record.calcFormula(7))); + assertTrue(Double.isNaN(record.getUpperCriticalThreshold())); + } + @Test void reservedUnitValuesDoNotThrow() { assertEquals(RateUnit.None, RateUnit.parseInt(7)); From c4acb22b26c54b1807cab30d11f46aab2e2bf2db Mon Sep 17 00:00:00 2001 From: Bertrand Martin Date: Fri, 9 Oct 2026 10:34:26 +0200 Subject: [PATCH 3/7] Send word-addressed FRU offsets in words; NaN for records without analog reading - ReadFruData takes the offset in bytes and sends it in the unit of the device (divided by two for a word-addressed FRU), the count stays in bytes, as ipmitool does; the runner advances its cursor in bytes. - calcFormula() returns NaN for the data format 11b, as hasAnalogReading() promises. Co-Authored-By: Claude Fable 5.1 --- .../core/coding/commands/fru/ReadFruData.java | 26 +++++++------------ .../commands/sdr/record/FullSensorRecord.java | 5 ++-- .../coding/commands/fru/ReadFruDataTest.java | 10 +++++++ .../sdr/record/FullSensorRecordTest.java | 1 + 4 files changed, 23 insertions(+), 19 deletions(-) diff --git a/src/main/java/org/metricshub/ipmi/core/coding/commands/fru/ReadFruData.java b/src/main/java/org/metricshub/ipmi/core/coding/commands/fru/ReadFruData.java index 597ba80..abf7893 100644 --- a/src/main/java/org/metricshub/ipmi/core/coding/commands/fru/ReadFruData.java +++ b/src/main/java/org/metricshub/ipmi/core/coding/commands/fru/ReadFruData.java @@ -89,9 +89,9 @@ public class ReadFruData extends IpmiCommandCoder { * - {@link BaseUnit} indicating if the FRU device is accessed in * {@link BaseUnit#Bytes} or {@link BaseUnit#Words} * @param offset - * - offset to read in units specified by unit + * - offset to read, in bytes (sent in words when the device is word-addressed, so it must be even then) * @param countToRead - * - size of the area to read in unit. Cannot exceed 255; + * - number of bytes to read. Cannot exceed 255; */ public ReadFruData(int fruId, BaseUnit unit, int offset, int countToRead) { super(); @@ -105,13 +105,10 @@ public ReadFruData(int fruId, BaseUnit unit, int offset, int countToRead) { throw new IllegalArgumentException("FRU ID cannot exceed 255"); } - this.offset = offset * unit.getSize(); - - size = countToRead * unit.getSize(); - + // Table 34-3: the offset goes on the wire in the unit of the device, the count in bytes (as ipmitool sends it) + this.offset = offset / unit.getSize(); + size = countToRead; this.fruId = fruId; - // TODO: Check if Count To Read field is encoded in words if the FRU is - // addressed in words (requires different server settings). } /** @@ -132,9 +129,9 @@ public ReadFruData(int fruId, BaseUnit unit, int offset, int countToRead) { * - {@link BaseUnit} indicating if the FRU device is accessed in * {@link BaseUnit#Bytes} or {@link BaseUnit#Words} * @param offset - * - offset to read in units specified by unit + * - offset to read, in bytes (sent in words when the device is word-addressed, so it must be even then) * @param countToRead - * - size of the area to read in unit. Cannot exceed 255; + * - number of bytes to read. Cannot exceed 255; */ public ReadFruData(IpmiVersion version, CipherSuite cipherSuite, AuthenticationType authenticationType, int fruId, BaseUnit unit, @@ -150,13 +147,10 @@ public ReadFruData(IpmiVersion version, CipherSuite cipherSuite, throw new IllegalArgumentException("FRU ID cannot exceed 255"); } - this.offset = offset * unit.getSize(); - - size = countToRead * unit.getSize(); - + // Table 34-3: the offset goes on the wire in the unit of the device, the count in bytes (as ipmitool sends it) + this.offset = offset / unit.getSize(); + size = countToRead; this.fruId = fruId; - // TODO: Check if Count To Read field is encoded in words if the FRU is - // addressed in words (requires different server settings). } @Override diff --git a/src/main/java/org/metricshub/ipmi/core/coding/commands/sdr/record/FullSensorRecord.java b/src/main/java/org/metricshub/ipmi/core/coding/commands/sdr/record/FullSensorRecord.java index ef46d37..e2c6aa0 100644 --- a/src/main/java/org/metricshub/ipmi/core/coding/commands/sdr/record/FullSensorRecord.java +++ b/src/main/java/org/metricshub/ipmi/core/coding/commands/sdr/record/FullSensorRecord.java @@ -367,9 +367,8 @@ protected double calcFormula(int value, int length, byte units1) { case 2: // 2's complement base = TypeConverter.decode2sComplement(value, length - 1); break; - case 3: // no analog reading - base = value; - break; + case 3: // no analog reading: the byte is not a value to convert + return Double.NaN; default: throw new IllegalArgumentException( "Invalid data format in sensorUnits1"); diff --git a/src/test/java/org/metricshub/ipmi/core/coding/commands/fru/ReadFruDataTest.java b/src/test/java/org/metricshub/ipmi/core/coding/commands/fru/ReadFruDataTest.java index 4534057..387af8c 100644 --- a/src/test/java/org/metricshub/ipmi/core/coding/commands/fru/ReadFruDataTest.java +++ b/src/test/java/org/metricshub/ipmi/core/coding/commands/fru/ReadFruDataTest.java @@ -191,6 +191,16 @@ void aWordIsTwoBytes() { assertEquals(2, BaseUnit.Words.getSize()); } + @Test + void wordAddressedDeviceGetsItsOffsetInWordsAndItsCountInBytes() throws Exception { + // Table 34-3: FRU ID, offset (LS byte first) in the unit of the device, count in bytes + ReadFruData bytes = new ReadFruData(3, BaseUnit.Bytes, 32, 16); + assertArrayEquals(bytes(3, 32, 0, 16), bytes.preparePayload(1).getData()); + + ReadFruData words = new ReadFruData(3, BaseUnit.Words, 32, 16); + assertArrayEquals(bytes(3, 16, 0, 16), words.preparePayload(1).getData()); + } + @Test void baseCompatibilityMasksAreReadAtTheRecordOffset() { byte[] junk = new byte[10]; diff --git a/src/test/java/org/metricshub/ipmi/core/coding/commands/sdr/record/FullSensorRecordTest.java b/src/test/java/org/metricshub/ipmi/core/coding/commands/sdr/record/FullSensorRecordTest.java index 1eae321..c3a8301 100644 --- a/src/test/java/org/metricshub/ipmi/core/coding/commands/sdr/record/FullSensorRecordTest.java +++ b/src/test/java/org/metricshub/ipmi/core/coding/commands/sdr/record/FullSensorRecordTest.java @@ -90,6 +90,7 @@ void recordWithoutThresholdsOrAnalogReadingSaysSo() { FullSensorRecord record = (FullSensorRecord) SensorRecord.populateSensorRecord(NO_THRESHOLDS_NO_READING); assertFalse(record.hasAnalogReading()); + assertTrue(Double.isNaN(record.calcFormula(7)), "the reading byte of such a record is not a value"); assertTrue(Double.isNaN(record.getUpperNonRecoverableThreshold())); assertTrue(Double.isNaN(record.getUpperCriticalThreshold())); assertTrue(Double.isNaN(record.getUpperNonCriticalThreshold())); From ff77c8c3d8699e1259c40e1d202a750efee93b1f Mon Sep 17 00:00:00 2001 From: Bertrand Martin Date: Fri, 9 Oct 2026 10:47:15 +0200 Subject: [PATCH 4/7] Check the multirecord checksums; document the ReadFruData units - A multirecord whose record checksum is wrong is skipped; a wrong header checksum ends the multirecord area, as the length and the end-of-list flag cannot be trusted. - upgrading.md tells low-level callers that the ReadFruData offset and count are now in bytes, with the loop to write. Co-Authored-By: Claude Fable 5.1 --- .../core/coding/commands/fru/ReadFruData.java | 24 ++++++++++++++++++- src/site/markdown/upgrading.md | 15 ++++++++++++ .../coding/commands/fru/ReadFruDataTest.java | 22 +++++++++++++++++ 3 files changed, 60 insertions(+), 1 deletion(-) diff --git a/src/main/java/org/metricshub/ipmi/core/coding/commands/fru/ReadFruData.java b/src/main/java/org/metricshub/ipmi/core/coding/commands/fru/ReadFruData.java index abf7893..17019cd 100644 --- a/src/main/java/org/metricshub/ipmi/core/coding/commands/fru/ReadFruData.java +++ b/src/main/java/org/metricshub/ipmi/core/coding/commands/fru/ReadFruData.java @@ -316,6 +316,14 @@ private static void addMultirecords(List list, byte[] data, int multi boolean last = false; while (!last && offset + MULTIRECORD_HEADER_SIZE <= data.length) { + // A corrupt header (section 16.2.5) means the length and the end-of-list flag cannot be trusted + if (!isChecksumValid(data, offset, MULTIRECORD_HEADER_SIZE)) { + LOGGER + .warn( + "The multirecord header at offset {} has an invalid checksum: the rest of the multirecord area is skipped", + offset); + return; + } last = (TypeConverter.byteToInt(data[offset + 1]) & 0x80) != 0; int length = TypeConverter.byteToInt(data[offset + 2]); @@ -323,6 +331,16 @@ private static void addMultirecords(List list, byte[] data, int multi LOGGER.warn("The multirecord at offset {} is truncated: the rest of the multirecord area is skipped", offset); return; } + // The record checksum (header byte 4) is a zero checksum of the record data (section 16.2.6) + if (((data[offset + 3] + sum(data, offset + MULTIRECORD_HEADER_SIZE, length)) & 0xff) != 0) { + LOGGER + .warn( + "Skipping the multirecord of type 0x{} at offset {}: invalid record checksum", + Integer.toHexString(TypeConverter.byteToInt(data[offset])), + offset); + offset += MULTIRECORD_HEADER_SIZE + length; + continue; + } try { list.add(MultiRecordInfo.populateMultiRecord(data, offset)); } catch (RuntimeException e) { @@ -341,11 +359,15 @@ private static void addMultirecords(List list, byte[] data, int multi * @return whether the bytes of the given range add up to zero modulo 256, as every FRU header and area must */ private static boolean isChecksumValid(byte[] data, int offset, int length) { + return (sum(data, offset, length) & 0xff) == 0; + } + + private static int sum(byte[] data, int offset, int length) { int sum = 0; for (int i = offset; i < offset + length; i++) { sum += data[i]; } - return (sum & 0xff) == 0; + return sum; } } diff --git a/src/site/markdown/upgrading.md b/src/site/markdown/upgrading.md index 968c746..21c1e37 100644 --- a/src/site/markdown/upgrading.md +++ b/src/site/markdown/upgrading.md @@ -72,6 +72,21 @@ The decoders follow the IPMI 2.0 and FRU specifications more closely; the visibl * reserved values of the rate unit, modifier unit usage and power restore policy decode to `None` or `Unknown` instead of throwing. +Code that **uses the low-level API** to read FRUs: the `offset` and `countToRead` arguments of +the `ReadFruData` constructors are now **in bytes** whatever the access unit of the device, and +the offset alone is sent in words when Get FRU Inventory Area Info reports a word-addressed FRU +(1.2.02 multiplied both by a "word size" of 16, which read word-addressed FRUs 16 times too far). +A loop that walked a FRU in units of the device now walks it in bytes, with an even offset for +a word-addressed device: + +```java +int size = info.getFruInventoryAreaSize(); // in bytes, whatever the access unit +for (int offset = 0; offset < size; offset += 16) { + connector.sendMessage(handle, new ReadFruData(IpmiVersion.V20, cipherSuite, AuthenticationType.RMCPPlus, + fruId, info.getFruUnit(), offset, Math.min(16, size - offset))); +} +``` + Code that **extends** the library's protocol classes needs the changes below. `QueueElement` lost its `isTimedOut()`, `makeTimedOut()` and `refreshTimestamp()` methods: a timed-out message now leaves the queue at once. diff --git a/src/test/java/org/metricshub/ipmi/core/coding/commands/fru/ReadFruDataTest.java b/src/test/java/org/metricshub/ipmi/core/coding/commands/fru/ReadFruDataTest.java index 387af8c..34b46d7 100644 --- a/src/test/java/org/metricshub/ipmi/core/coding/commands/fru/ReadFruDataTest.java +++ b/src/test/java/org/metricshub/ipmi/core/coding/commands/fru/ReadFruDataTest.java @@ -177,6 +177,28 @@ void truncatedReadKeepsTheCompleteAreas() { assertInstanceOf(BoardInfo.class, records.get(0)); } + @Test + void multirecordWithAnInvalidRecordChecksumIsSkipped() { + byte[] image = image(); + // the data of the last multirecord (the power supply record) starts 5 bytes after its header, 24 bytes long + image[image.length - 24] ^= 0x01; + + List records = decode(image); + + assertEquals(2, records.size(), "board and product; the corrupt power supply record is skipped"); + } + + @Test + void multirecordWithAnInvalidHeaderChecksumEndsTheArea() { + byte[] image = image(); + // header checksum of the last multirecord: byte 4 of its 5-byte header + image[image.length - 24 - 1] ^= 0x01; + + List records = decode(image); + + assertEquals(2, records.size(), "board and product; the area is not read past the corrupt header"); + } + @Test void invalidHeaderChecksumIsRejected() { byte[] image = image(); From 3b967b7f93899a8e50738015f87c18942fe4097b Mon Sep 17 00:00:00 2001 From: Bertrand Martin Date: Fri, 9 Oct 2026 11:16:53 +0200 Subject: [PATCH 5/7] Decode a multirecord whose record checksum is wrong A Dell iDRAC 8 (PowerEdge R630) writes the record checksum of its power supply multirecords off by one (0xEF for data summing to 0x10), on genuine 750 W records. The record checksum mismatch is therefore logged at DEBUG and the length-delimited record decoded anyway, as the area checksums are; the header checksum stays fatal for the area, as a wrong length would derail the walk. Co-Authored-By: Claude Fable 5.1 --- .../ipmi/core/coding/commands/fru/ReadFruData.java | 10 +++++----- .../ipmi/core/coding/commands/fru/ReadFruDataTest.java | 10 ++++++---- 2 files changed, 11 insertions(+), 9 deletions(-) diff --git a/src/main/java/org/metricshub/ipmi/core/coding/commands/fru/ReadFruData.java b/src/main/java/org/metricshub/ipmi/core/coding/commands/fru/ReadFruData.java index 17019cd..67b68a6 100644 --- a/src/main/java/org/metricshub/ipmi/core/coding/commands/fru/ReadFruData.java +++ b/src/main/java/org/metricshub/ipmi/core/coding/commands/fru/ReadFruData.java @@ -331,15 +331,15 @@ private static void addMultirecords(List list, byte[] data, int multi LOGGER.warn("The multirecord at offset {} is truncated: the rest of the multirecord area is skipped", offset); return; } - // The record checksum (header byte 4) is a zero checksum of the record data (section 16.2.6) + // The record checksum (header byte 4) is a zero checksum of the record data (section 16.2.6). Real + // firmware gets it wrong on genuine records (a Dell iDRAC 8 writes it off by one on its power supply + // records), so a mismatch is logged, not fatal: the length-delimited record is decoded anyway if (((data[offset + 3] + sum(data, offset + MULTIRECORD_HEADER_SIZE, length)) & 0xff) != 0) { LOGGER - .warn( - "Skipping the multirecord of type 0x{} at offset {}: invalid record checksum", + .debug( + "The multirecord of type 0x{} at offset {} has an invalid record checksum: decoded anyway", Integer.toHexString(TypeConverter.byteToInt(data[offset])), offset); - offset += MULTIRECORD_HEADER_SIZE + length; - continue; } try { list.add(MultiRecordInfo.populateMultiRecord(data, offset)); diff --git a/src/test/java/org/metricshub/ipmi/core/coding/commands/fru/ReadFruDataTest.java b/src/test/java/org/metricshub/ipmi/core/coding/commands/fru/ReadFruDataTest.java index 34b46d7..890d817 100644 --- a/src/test/java/org/metricshub/ipmi/core/coding/commands/fru/ReadFruDataTest.java +++ b/src/test/java/org/metricshub/ipmi/core/coding/commands/fru/ReadFruDataTest.java @@ -178,14 +178,16 @@ void truncatedReadKeepsTheCompleteAreas() { } @Test - void multirecordWithAnInvalidRecordChecksumIsSkipped() { + void multirecordWithAnInvalidRecordChecksumIsStillDecoded() { byte[] image = image(); - // the data of the last multirecord (the power supply record) starts 5 bytes after its header, 24 bytes long - image[image.length - 24] ^= 0x01; + // the data of the last multirecord (the power supply record) starts 5 bytes after its header, 24 bytes long: + // a Dell iDRAC 8 writes a record checksum off by one on genuine power supply records + image[image.length - 24 - 5 + 3] ^= 0x01; List records = decode(image); - assertEquals(2, records.size(), "board and product; the corrupt power supply record is skipped"); + assertEquals(3, records.size(), "board, product and the power supply record, checksum or not"); + assertEquals(750, assertInstanceOf(PowerSupplyInfo.class, records.get(2)).getCapacity()); } @Test From f087ad032d1d7d892535b0a2646559116a1eb140 Mon Sep 17 00:00:00 2001 From: Bertrand Martin Date: Fri, 9 Oct 2026 11:18:34 +0200 Subject: [PATCH 6/7] Corrupt a data byte, not the header, in the record checksum test Co-Authored-By: Claude Fable 5.1 --- .../ipmi/core/coding/commands/fru/ReadFruDataTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/test/java/org/metricshub/ipmi/core/coding/commands/fru/ReadFruDataTest.java b/src/test/java/org/metricshub/ipmi/core/coding/commands/fru/ReadFruDataTest.java index 890d817..472cbf5 100644 --- a/src/test/java/org/metricshub/ipmi/core/coding/commands/fru/ReadFruDataTest.java +++ b/src/test/java/org/metricshub/ipmi/core/coding/commands/fru/ReadFruDataTest.java @@ -182,7 +182,7 @@ void multirecordWithAnInvalidRecordChecksumIsStillDecoded() { byte[] image = image(); // the data of the last multirecord (the power supply record) starts 5 bytes after its header, 24 bytes long: // a Dell iDRAC 8 writes a record checksum off by one on genuine power supply records - image[image.length - 24 - 5 + 3] ^= 0x01; + image[image.length - 1] ^= 0x01; List records = decode(image); From 49d24d08f119a281a64329e47fb9b9273ad887a0 Mon Sep 17 00:00:00 2001 From: Bertrand Martin Date: Fri, 9 Oct 2026 11:30:45 +0200 Subject: [PATCH 7/7] Name the system board FRU by area priority, not decode order Co-Authored-By: Claude Fable 5.1 --- .../ipmi/client/runner/GetFrusRunner.java | 36 ++++++---- .../ipmi/client/runner/GetFrusRunnerTest.java | 67 +++++++++++++++++++ 2 files changed, 90 insertions(+), 13 deletions(-) diff --git a/src/main/java/org/metricshub/ipmi/client/runner/GetFrusRunner.java b/src/main/java/org/metricshub/ipmi/client/runner/GetFrusRunner.java index 5a7b3d8..79507ff 100644 --- a/src/main/java/org/metricshub/ipmi/client/runner/GetFrusRunner.java +++ b/src/main/java/org/metricshub/ipmi/client/runner/GetFrusRunner.java @@ -26,6 +26,7 @@ import java.util.HashSet; import java.util.List; import java.util.Set; +import java.util.function.Function; import java.util.stream.Collectors; import org.metricshub.ipmi.client.IpmiClientConfiguration; @@ -185,20 +186,29 @@ private void processFruRecord( * @return the name of the system board from its board, product or chassis area, whichever exists */ static String systemBoardName(final List fruRecords) { - for (FruRecord record : fruRecords) { - String name = null; - if (record instanceof BoardInfo) { - name = ((BoardInfo) record).getBoardProductName(); - } else if (record instanceof ProductInfo) { - name = ((ProductInfo) record).getProductName(); - } else if (record instanceof ChassisInfo) { - name = ((ChassisInfo) record).getChassisPartNumber(); - } - if (Utils.isNotBlank(name)) { - return name; - } + // In priority order, whatever the order of the areas in the FRU: board, then product, then chassis + String name = firstName(fruRecords, BoardInfo.class, BoardInfo::getBoardProductName); + if (name == null) { + name = firstName(fruRecords, ProductInfo.class, ProductInfo::getProductName); + } + if (name == null) { + name = firstName(fruRecords, ChassisInfo.class, ChassisInfo::getChassisPartNumber); } - return SYSTEM_BOARD_NAME; + return name == null ? SYSTEM_BOARD_NAME : name; + } + + private static String firstName( + final List fruRecords, + final Class type, + final Function getter) { + return fruRecords + .stream() + .filter(type::isInstance) + .map(type::cast) + .map(getter) + .filter(Utils::isNotBlank) + .findFirst() + .orElse(null); } /** diff --git a/src/test/java/org/metricshub/ipmi/client/runner/GetFrusRunnerTest.java b/src/test/java/org/metricshub/ipmi/client/runner/GetFrusRunnerTest.java index 1fd11f5..cca83c4 100644 --- a/src/test/java/org/metricshub/ipmi/client/runner/GetFrusRunnerTest.java +++ b/src/test/java/org/metricshub/ipmi/client/runner/GetFrusRunnerTest.java @@ -2,14 +2,71 @@ import static org.junit.jupiter.api.Assertions.assertEquals; +import java.util.Arrays; import java.util.Collections; import org.junit.jupiter.api.Test; import org.metricshub.ipmi.client.IpmiResultConverterTest; +import org.metricshub.ipmi.core.coding.commands.fru.record.BoardInfo; +import org.metricshub.ipmi.core.coding.commands.fru.record.ChassisInfo; import org.metricshub.ipmi.core.coding.commands.fru.record.ProductInfo; class GetFrusRunnerTest { + private static byte[] bytes(int... values) { + byte[] result = new byte[values.length]; + for (int i = 0; i < values.length; i++) { + result[i] = (byte) values[i]; + } + return result; + } + + /** A chassis info area: version 1, length 2 (16 bytes), rack mount chassis, part number "PN", end, padding. */ + private static final byte[] CHASSIS_AREA = bytes( + 0x01, + 0x02, + 0x17, + 0xc2, + 'P', + 'N', + 0xc1, + 0x00, + 0x00, + 0x00, + 0x00, + 0x00, + 0x00, + 0x00, + 0x00, + 0x00); + + /** A board info area: version 1, length 3 (24 bytes), English, no date, "IBM", product "X123", end, padding. */ + private static final byte[] BOARD_AREA = bytes( + 0x01, + 0x03, + 0x00, + 0x00, + 0x00, + 0x00, + 0xc3, + 'I', + 'B', + 'M', + 0xc4, + 'X', + '1', + '2', + '3', + 0xc1, + 0x00, + 0x00, + 0x00, + 0x00, + 0x00, + 0x00, + 0x00, + 0x00); + @Test void systemBoardIsNamedAfterWhateverAreaDescribesIt() { ProductInfo product = new ProductInfo(IpmiResultConverterTest.BASE_BOARD_PRODUCT_INFO, 360); @@ -17,4 +74,14 @@ void systemBoardIsNamedAfterWhateverAreaDescribesIt() { assertEquals("System x3650 M2", GetFrusRunner.systemBoardName(Collections.singletonList(product))); assertEquals("System Board", GetFrusRunner.systemBoardName(Collections.emptyList())); } + + @Test + void boardAreaNamesTheSystemBoardWhateverTheAreaOrder() { + ChassisInfo chassis = new ChassisInfo(CHASSIS_AREA, 0); + BoardInfo board = new BoardInfo(BOARD_AREA, 0); + + // decodeFruData() lists the chassis area first: the board product name must still win + assertEquals("X123", GetFrusRunner.systemBoardName(Arrays.asList(chassis, board))); + assertEquals("PN", GetFrusRunner.systemBoardName(Collections.singletonList(chassis))); + } }