diff --git a/AGENTS.md b/AGENTS.md new file mode 100644 index 0000000..e7a49bf --- /dev/null +++ b/AGENTS.md @@ -0,0 +1,37 @@ +# Instructions for AI Agents + +## Code format + +The project has not adopted the MetricsHub Eclipse formatter profile yet (see issue #118). Until it does, match the style of the file you are editing: the `org.metricshub.ipmi.client` packages use tabs, the `org.metricshub.ipmi.core` packages (forked from the Verax IPMI library) use 4 spaces. Do not reformat code you are not otherwise changing. Once `formatter-maven-plugin` is configured, simply run `mvn formatter:format` before committing. + +All Java source files under `src/main/java` must include the proper LGPL-3 license header (the `license-maven-plugin` check covers `main/java/**/*.java` only; tests, Markdown and resources carry no header). When you add a new source file, run `mvn license:update-file-header` before committing (and before building, since the build fails if a source file lacks the header). + +All public methods must have proper Javadoc. Check the output of Maven to identify issues with Javadoc and fix these issues. + +The library targets **Java 8** (`maven.compiler.release` is 8): no `var`, records, switch expressions, text blocks or APIs newer than Java 8 in `src/main`. + +## Build + +The project uses Maven to build. A full build is performed with `mvn verify site` (or `mvn clean verify site` when applicable). CI runs the same on JDK 17. + +@codex, please don't try to use `mvnw` (Maven Wrapper). Maven is already installed and runs perfectly well. + +## Test + +Whenever required, when you add code or when you modify code that is not covered with unit tests, add the corresponding unit tests. All tests must pass with `mvn test`. Don't use the `-q` (silent) option, as you want to see the result of successful tests. Tests are run with the Maven surefire plugin and results are stored in the ./target/surefire-reports directory. + +Unit tests must not depend on a real BMC. Exercising the RMCP+ session code against real hardware is done with throw-away harnesses outside the repository; never commit hostnames, user names or passwords of test systems. When testing against a real BMC, keep in mind that BMCs drop UDP replies under concurrent sessions: repeat a failing request before blaming the library, set a short per-message timeout with `IpmiConnector.setTimeout(handle, ms)`, and end harness `main` methods with `System.exit(0)` because the library leaves non-daemon threads behind after a timeout. + +## Code quality reports + +Code quality reports (checkstyle, pmd/cpd, spotbugs) are generated by `mvn verify site` into ./target/checkstyle-result.xml, ./target/pmd.xml, ./target/cpd.xml and ./target/spotbugsXml.xml. They are not yet gated (issues #114, #115, #116, #117 track the clean-up). Do not add new violations: check the reports for the files you changed before committing and submitting your code. On JDK 21+ the SpotBugs plugin version inherited from the parent POM cannot read the JDK class files; run `mvn com.github.spotbugs:spotbugs-maven-plugin:4.10.4.1:spotbugs` instead. + +## Documentation + +Always make sure that public API changes are properly documented in src/site/markdown/*.md and that README.md is always up-to-date. The published documentation is https://metricshub.org/ipmi-java (see issue #113 for the planned overhaul). + +## IPMI specifics + +- The protocol implementation lives in `org.metricshub.ipmi.core` (RMCP+, RAKP, SDR/FRU/SEL coders, connection state machine); the MetricsHub-facing API is `org.metricshub.ipmi.client` (`IpmiClient`, `IpmiClientConfiguration`, the runners and `IpmiResultConverter`). +- Decoders must never abort a whole SDR repository walk or FRU decode on a record they do not model: skip, log and continue (IPMI 2.0 reserves SDR record types 0xC0-0xFF for OEM use and vendors do emit them). +- Byte-level parsing must follow the IPMI 2.0 specification (section numbers are cited in the code and in the issues); when a finding is "by the spec" but not reproduced on real hardware, say so in the commit or PR. diff --git a/CLAUDE.md b/CLAUDE.md new file mode 100644 index 0000000..43c994c --- /dev/null +++ b/CLAUDE.md @@ -0,0 +1 @@ +@AGENTS.md diff --git a/src/main/java/org/metricshub/ipmi/client/runner/AbstractIpmiRunner.java b/src/main/java/org/metricshub/ipmi/client/runner/AbstractIpmiRunner.java index 66b084b..77ade3f 100644 --- a/src/main/java/org/metricshub/ipmi/client/runner/AbstractIpmiRunner.java +++ b/src/main/java/org/metricshub/ipmi/client/runner/AbstractIpmiRunner.java @@ -40,14 +40,18 @@ import org.metricshub.ipmi.core.coding.security.CipherSuite; import org.metricshub.ipmi.core.common.TypeConverter; import org.metricshub.ipmi.core.connection.Connection; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; /** * This abstract class implements common features required by FRUs, Sensor and Chassis Status runners. - * - * @param Represent the data type managed by the runner + * + * @param Represent the data type managed by the runner */ public abstract class AbstractIpmiRunner implements AutoCloseable, Callable { + private static final Logger LOGGER = LoggerFactory.getLogger(AbstractIpmiRunner.class); + private static final int DEFAULT_LOCAL_UDP_PORT = 0; /** @@ -192,13 +196,18 @@ protected SensorRecord getSensorData(int reservationId) throws Exception { GetSdrResponseData data = (GetSdrResponseData) connector.sendMessage(handle, new GetSdr(IpmiVersion.V20, handle.getCipherSuite(), AuthenticationType.RMCPPlus, reservationId, nextRecId)); - // If getting whole record succeeded we create SensorRecord from - // received data... - SensorRecord sensorDataToPopulate = SensorRecord.populateSensorRecord(data.getSensorRecordData()); + // Some BMCs answer with success but return fewer bytes than the + // record length declared in the header: fall back to the chunked + // read while the current record ID is still known + if (isTruncated(data.getSensorRecordData())) { + return getSensorViaChunks(reservationId); + } - // ... and update the ID of the next record + // Advance to the next record first, so that a record we cannot + // decode never stalls the whole repository walk nextRecId = data.getNextRecordId(); - return sensorDataToPopulate; + + return decodeRecord(data.getSensorRecordData()); } catch (IPMIException e) { @@ -229,14 +238,22 @@ protected SensorRecord getSensorViaChunks(int reservationId) throws Exception { GetSdrResponseData data = (GetSdrResponseData) connector.sendMessage(handle, new GetSdr(IpmiVersion.V20, handle.getCipherSuite(), AuthenticationType.RMCPPlus, reservationId, nextRecId, 0, INITIAL_CHUNK_SIZE)); + byte[] header = data.getSensorRecordData(); + if (header == null || header.length < HEADER_SIZE) { + LOGGER.warn("Skipping SDR record {} on {}: BMC returned {} byte(s) instead of the {}-byte header", nextRecId, + ipmiConfiguration.getHostname(), header == null ? 0 : header.length, HEADER_SIZE); + nextRecId = data.getNextRecordId(); + return null; + } + // The record size is 5th byte of the record. It does not take // into account the size of the header, so we need to add it. - int recSize = TypeConverter.byteToInt(data.getSensorRecordData()[4]) + HEADER_SIZE; - int read = INITIAL_CHUNK_SIZE; + int recSize = TypeConverter.byteToInt(header[HEADER_SIZE - 1]) + HEADER_SIZE; + int read = Math.min(header.length, recSize); byte[] bytes = new byte[recSize]; - System.arraycopy(data.getSensorRecordData(), 0, bytes, 0, data.getSensorRecordData().length); + System.arraycopy(header, 0, bytes, 0, read); // We get the rest of the record in chunks (watch out for // exceeding the record size, since this will result in BMC's @@ -251,19 +268,54 @@ protected SensorRecord getSensorViaChunks(int reservationId) throws Exception { GetSdrResponseData part = (GetSdrResponseData) connector.sendMessage(handle, new GetSdr(IpmiVersion.V20, handle.getCipherSuite(), AuthenticationType.RMCPPlus, reservationId, nextRecId, read, bytesToRead)); + byte[] chunk = part.getSensorRecordData(); + int got = chunk == null ? 0 : Math.min(bytesToRead, chunk.length); + if (got == 0) { + LOGGER.warn("Skipping SDR record {} on {}: BMC returned no data at offset {}", nextRecId, + ipmiConfiguration.getHostname(), read); + nextRecId = data.getNextRecordId(); + return null; + } + // Append the new bytes - System.arraycopy(part.getSensorRecordData(), 0, bytes, read, bytesToRead); + System.arraycopy(chunk, 0, bytes, read, got); - read += bytesToRead; + read += got; } - // Finally we populate the sensor record with the gathered - // data... - SensorRecord sensorDataToPopulate = SensorRecord.populateSensorRecord(bytes); - - // ... and update the ID of the next record + // Advance to the next record, then decode the gathered data nextRecId = data.getNextRecordId(); - return sensorDataToPopulate; + return decodeRecord(bytes); + } + + /** + * Decode a raw SDR record. A record the library cannot model (unknown or malformed type) is logged and skipped + * instead of aborting the whole repository walk. + * + * @param recordData Raw bytes of the SDR record + * @return {@link SensorRecord} instance or null if the record cannot be decoded + */ + SensorRecord decodeRecord(byte[] recordData) { + try { + return SensorRecord.populateSensorRecord(recordData); + } catch (RuntimeException e) { + LOGGER.warn("Skipping undecodable SDR record before id {} on {}: {}", nextRecId, ipmiConfiguration.getHostname(), e.getMessage()); + return null; + } + } + + /** + * Detect a whole-record GetSdr response that is shorter than the record length declared in its own header. Some BMCs + * answer such requests with a success completion code and a truncated body instead of "Cannot return number of + * requested data bytes". + * + * @param recordData Raw bytes returned for a whole-record GetSdr request + * @return true if the header is missing or fewer bytes than declared were returned + */ + static boolean isTruncated(byte[] recordData) { + return recordData == null + || recordData.length < HEADER_SIZE + || recordData.length < TypeConverter.byteToInt(recordData[HEADER_SIZE - 1]) + HEADER_SIZE; } } diff --git a/src/main/java/org/metricshub/ipmi/core/coding/commands/sdr/record/OemRecord.java b/src/main/java/org/metricshub/ipmi/core/coding/commands/sdr/record/OemRecord.java index 4552c00..2c4d6f3 100644 --- a/src/main/java/org/metricshub/ipmi/core/coding/commands/sdr/record/OemRecord.java +++ b/src/main/java/org/metricshub/ipmi/core/coding/commands/sdr/record/OemRecord.java @@ -29,6 +29,16 @@ */ public class OemRecord extends SensorRecord { + /** + * Size of the common SDR record header (record ID, SDR version, record type, record length). + */ + private static final int HEADER_LENGTH = 5; + + /** + * Size of the manufacturer ID field in a C0h OEM record. + */ + private static final int MANUFACTURER_ID_LENGTH = 3; + private int manufacturerId; private byte[] oemData; @@ -37,17 +47,24 @@ public class OemRecord extends SensorRecord { protected void populateTypeSpecficValues(byte[] recordData, SensorRecord record) { - byte[] buffer = new byte[4]; - - System.arraycopy(recordData, 5, buffer, 0, 3); - - buffer[3] = 0; + // Only the C0h OEM record has a defined layout (3-byte manufacturer ID + // followed by OEM data). Vendor-defined record types C1h-FFh carry an + // unknown layout, so keep their whole type-specific payload and leave + // the manufacturer ID at 0 (unknown). + int dataOffset = HEADER_LENGTH; - setManufacturerId(TypeConverter.littleEndianByteArrayToInt(buffer)); + if (recordData[3] == RecordTypes.OEM_RECORD && recordData.length >= HEADER_LENGTH + MANUFACTURER_ID_LENGTH) { + byte[] buffer = new byte[4]; + System.arraycopy(recordData, HEADER_LENGTH, buffer, 0, MANUFACTURER_ID_LENGTH); + setManufacturerId(TypeConverter.littleEndianByteArrayToInt(buffer)); + dataOffset += MANUFACTURER_ID_LENGTH; + } - byte[] data = new byte[recordData.length - 8]; + byte[] data = new byte[Math.max(0, recordData.length - dataOffset)]; - System.arraycopy(recordData, 8, data, 0, data.length); + if (data.length > 0) { + System.arraycopy(recordData, dataOffset, data, 0, data.length); + } setOemData(data); } diff --git a/src/main/java/org/metricshub/ipmi/core/coding/commands/sdr/record/SensorRecord.java b/src/main/java/org/metricshub/ipmi/core/coding/commands/sdr/record/SensorRecord.java index 284165b..17cd9dd 100644 --- a/src/main/java/org/metricshub/ipmi/core/coding/commands/sdr/record/SensorRecord.java +++ b/src/main/java/org/metricshub/ipmi/core/coding/commands/sdr/record/SensorRecord.java @@ -91,6 +91,13 @@ public static SensorRecord populateSensorRecord(byte[] recordData) { sensorRecord = new OemRecord(); break; default: + // IPMI 2.0 Table 43-1 reserves C0h-FFh for OEM records; vendors use + // values above C0h (e.g. NVIDIA/GIGABYTE emit D0h). Never abort the + // SDR repository walk on a record type we don't model. + if (TypeConverter.byteToInt(recType) >= TypeConverter.byteToInt(RecordTypes.OEM_RECORD)) { + sensorRecord = new OemRecord(); + break; + } throw new IllegalArgumentException("Invalid record type: " + recType); } diff --git a/src/test/java/org/metricshub/ipmi/client/runner/AbstractIpmiRunnerTest.java b/src/test/java/org/metricshub/ipmi/client/runner/AbstractIpmiRunnerTest.java new file mode 100644 index 0000000..acd1099 --- /dev/null +++ b/src/test/java/org/metricshub/ipmi/client/runner/AbstractIpmiRunnerTest.java @@ -0,0 +1,74 @@ +package org.metricshub.ipmi.client.runner; + +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertInstanceOf; +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import org.junit.jupiter.api.Test; +import org.metricshub.ipmi.client.IpmiClientConfiguration; +import org.metricshub.ipmi.core.coding.commands.sdr.record.FullSensorRecord; +import org.metricshub.ipmi.core.coding.commands.sdr.record.OemRecord; + +class AbstractIpmiRunnerTest { + + /** + * Minimal concrete runner: no session, no connector, just the decoding helpers under test. + */ + private static final AbstractIpmiRunner RUNNER = new AbstractIpmiRunner( + new IpmiClientConfiguration("bmc", "user", new char[0], null, false, 1)) { + @Override + public Void call() { + return null; + } + }; + + private static byte[] record(int type, byte[] payload) { + byte[] raw = new byte[5 + payload.length]; + raw[2] = 0x51; + raw[3] = (byte) type; + raw[4] = (byte) payload.length; + System.arraycopy(payload, 0, raw, 5, payload.length); + return raw; + } + + @Test + void decodeRecordReturnsOemRecordForVendorDefinedType() { + assertInstanceOf(OemRecord.class, RUNNER.decodeRecord(record(0xd0, new byte[] {1, 2, 3}))); + } + + @Test + void decodeRecordSkipsRecordsItCannotDecodeInsteadOfThrowing() { + // reserved (non-OEM) record type + assertNull(RUNNER.decodeRecord(record(0x05, new byte[] {1}))); + // shorter than the SDR header + assertNull(RUNNER.decodeRecord(new byte[] {0x00})); + // full sensor record whose body is missing: decoder runs out of bytes + assertNull(RUNNER.decodeRecord(record(0x01, new byte[] {1, 2, 3}))); + } + + @Test + void decodeRecordStillDecodesValidRecords() { + // A Full Sensor Record (type 01h) of minimal valid size: 48 bytes, the last one being the ID string type/length + byte[] payload = new byte[43]; + payload[0] = 0x20; // sensor owner: BMC + payload[7] = 0x01; // sensor type: temperature + payload[8] = 0x01; // event/reading type: threshold + payload[16] = 0x01; // base unit: degrees C + payload[42] = (byte) 0xc0; // 8-bit ASCII, zero-length name + assertInstanceOf(FullSensorRecord.class, RUNNER.decodeRecord(record(0x01, payload))); + } + + @Test + void isTruncatedDetectsWholeRecordResponsesShorterThanDeclared() { + byte[] complete = record(0xd0, new byte[] {1, 2, 3, 4, 5}); + assertFalse(AbstractIpmiRunner.isTruncated(complete)); + + byte[] truncated = new byte[complete.length - 2]; + System.arraycopy(complete, 0, truncated, 0, truncated.length); + assertTrue(AbstractIpmiRunner.isTruncated(truncated), "declared length 5 but only 3 payload bytes returned"); + + assertTrue(AbstractIpmiRunner.isTruncated(new byte[] {0x00, 0x00, 0x51}), "header itself is incomplete"); + assertTrue(AbstractIpmiRunner.isTruncated(null)); + } +} diff --git a/src/test/java/org/metricshub/ipmi/core/coding/commands/sdr/record/SensorRecordTest.java b/src/test/java/org/metricshub/ipmi/core/coding/commands/sdr/record/SensorRecordTest.java new file mode 100644 index 0000000..7d513f3 --- /dev/null +++ b/src/test/java/org/metricshub/ipmi/core/coding/commands/sdr/record/SensorRecordTest.java @@ -0,0 +1,82 @@ +package org.metricshub.ipmi.core.coding.commands.sdr.record; + +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.assertThrows; + +import org.junit.jupiter.api.Test; + +class SensorRecordTest { + + /** + * Build a raw SDR record: 2-byte record ID (LE), SDR version 0x51, record type, record length, then the payload. + */ + private static byte[] record(int id, int type, byte[] payload) { + byte[] raw = new byte[5 + payload.length]; + raw[0] = (byte) (id & 0xff); + raw[1] = (byte) ((id >> 8) & 0xff); + raw[2] = 0x51; + raw[3] = (byte) type; + raw[4] = (byte) payload.length; + System.arraycopy(payload, 0, raw, 5, payload.length); + return raw; + } + + @Test + void vendorDefinedRecordTypeIsDecodedAsOemRecordWithWholePayload() { + // Record type D0h as emitted by GIGABYTE/NVIDIA BMCs: vendor-defined layout, no manufacturer ID field + byte[] payload = {0x0a, 0x0b, 0x0c, 0x0d, 0x0e, 0x0f}; + + SensorRecord record = SensorRecord.populateSensorRecord(record(0x0004, 0xd0, payload)); + + OemRecord oem = assertInstanceOf(OemRecord.class, record); + assertEquals(4, oem.getId()); + assertEquals(0, oem.getManufacturerId(), "manufacturer ID is unknown for vendor-defined record types"); + assertArrayEquals(payload, oem.getOemData(), "the complete type-specific payload must be preserved"); + } + + @Test + void lastOemRecordTypeIsAccepted() { + byte[] payload = {0x01, 0x02}; + + OemRecord oem = assertInstanceOf(OemRecord.class, SensorRecord.populateSensorRecord(record(0x0010, 0xff, payload))); + + assertArrayEquals(payload, oem.getOemData()); + } + + @Test + void standardOemRecordTypeC0ParsesManufacturerIdAndData() { + // C0h layout: 3-byte manufacturer ID (LE, 0x000157 = 343) followed by OEM data + byte[] payload = {0x57, 0x01, 0x00, 0x21, 0x22}; + + OemRecord oem = assertInstanceOf(OemRecord.class, SensorRecord.populateSensorRecord(record(0x0020, 0xc0, payload))); + + assertEquals(343, oem.getManufacturerId()); + assertArrayEquals(new byte[] {0x21, 0x22}, oem.getOemData()); + } + + @Test + void shortOemRecordDoesNotThrow() { + // C0h record without even a complete manufacturer ID: no ID is fabricated and the bytes are kept as payload + OemRecord oem = assertInstanceOf(OemRecord.class, SensorRecord.populateSensorRecord(record(0x0030, 0xc0, new byte[] {0x57}))); + + assertEquals(0, oem.getManufacturerId()); + assertArrayEquals(new byte[] {0x57}, oem.getOemData()); + + // header only + OemRecord empty = assertInstanceOf(OemRecord.class, SensorRecord.populateSensorRecord(record(0x0031, 0xc0, new byte[0]))); + assertEquals(0, empty.getOemData().length); + } + + @Test + void unknownStandardRecordTypeIsRejected() { + // 05h is reserved by IPMI 2.0 and not an OEM type: callers are expected to skip it + assertThrows(IllegalArgumentException.class, () -> SensorRecord.populateSensorRecord(record(0x0040, 0x05, new byte[] {0x00}))); + } + + @Test + void recordShorterThanHeaderIsRejected() { + assertThrows(IllegalArgumentException.class, () -> SensorRecord.populateSensorRecord(new byte[] {0x00, 0x00, 0x51})); + } +}