fru, sdr: Refactor description handling

Reduce code duplication, get rid of magic numbers, unused variables
and functions, and of unsafe memory handling in FRU and SDR description
printing.

Also add support for logging generic locator and fru locator SDR record
names during SDR population (`ipmitool sdr fill`).

Signed-off-by: Alexander Amelkin <alexander@amelkin.msk.ru>

include/ipmitool/ipmi_sdr.h

@@ -104,6 +104,24 @@ enum {
 #define GET_SENSOR_READING	0x2d
 #define GET_SENSOR_TYPE		0x2f
 
+/*
+ * IPMI Specification limits the length of the ID string to 16 bytes for
+ * SDR records, although the ID type/length code may contain a number up
+ * to 31 (0x1F). See IPMI 2.0 Specification Tables 43-6 through 43-8.
+ */
+#define SDR_TYPECODE_LEN_MASK 0x1f
+#define SDR_ID_STRING_MAX 16
+#define SDR_ID_STRLEN_BYTYPE(typelen) \
+	((size_t)__max(typelen & SDR_TYPECODE_LEN_MASK, SDR_ID_STRING_MAX))
+#define SDR_ID_STRLEN(sdr) SDR_ID_STRLEN_BYTYPE((sdr)->id_code)
+
+#define SDR_ID_TO_CSTRING(cstring, sdr) \
+	do { \
+		memset((cstring), 0, sizeof(cstring)); \
+		snprintf((cstring), sizeof(cstring), "%.*s", \
+		         SDR_ID_STRLEN(sdr), (sdr)->id_string); \
+	} while(0)
+
 #ifdef HAVE_PRAGMA_PACK
 #pragma pack(1)
 #endif
@@ -466,7 +484,7 @@ struct sdr_record_compact_sensor {
 	uint8_t __reserved[3];
 	uint8_t oem;		/* reserved for OEM use */
 	uint8_t id_code;	/* sensor ID string type/length code */
-	uint8_t id_string[16];	/* sensor ID string bytes, only if id_code != 0 */
+	uint8_t id_string[SDR_ID_STRING_MAX]; /* sensor ID string bytes */
 } ATTRIBUTE_PACKING;
 #ifdef HAVE_PRAGMA_PACK
 #pragma pack(0)
@@ -517,7 +535,7 @@ struct sdr_record_eventonly_sensor {
 	uint8_t __reserved;
 	uint8_t oem;		/* reserved for OEM use */
 	uint8_t id_code;	/* sensor ID string type/length code */
-	uint8_t id_string[16];	/* sensor ID string bytes, only if id_code != 0 */
+	uint8_t id_string[SDR_ID_STRING_MAX]; /* sensor ID string bytes */
 
 } ATTRIBUTE_PACKING;
 #ifdef HAVE_PRAGMA_PACK
@@ -587,7 +605,7 @@ struct sdr_record_full_sensor {
 	uint8_t __reserved[2];
 	uint8_t oem;		/* reserved for OEM use */
 	uint8_t id_code;	/* sensor ID string type/length code */
-	uint8_t id_string[16];	/* sensor ID string bytes, only if id_code != 0 */
+	uint8_t id_string[SDR_ID_STRING_MAX]; /* sensor ID string bytes */
 } ATTRIBUTE_PACKING;
 #ifdef HAVE_PRAGMA_PACK
 #pragma pack(0)
@@ -619,7 +637,7 @@ struct sdr_record_mc_locator {
 	struct entity_id entity;
 	uint8_t oem;
 	uint8_t id_code;
-	uint8_t id_string[16];
+	uint8_t id_string[SDR_ID_STRING_MAX];
 } ATTRIBUTE_PACKING;
 #ifdef HAVE_PRAGMA_PACK
 #pragma pack(0)
@@ -652,7 +670,7 @@ struct sdr_record_fru_locator {
 	struct entity_id entity;
 	uint8_t oem;
 	uint8_t id_code;
-	uint8_t id_string[16];
+	uint8_t id_string[SDR_ID_STRING_MAX];
 } ATTRIBUTE_PACKING;
 #ifdef HAVE_PRAGMA_PACK
 #pragma pack(0)
@@ -686,7 +704,7 @@ struct sdr_record_generic_locator {
 	struct entity_id entity;
 	uint8_t oem;
 	uint8_t id_code;
-	uint8_t id_string[16];
+	uint8_t id_string[SDR_ID_STRING_MAX];
 } ATTRIBUTE_PACKING;
 #ifdef HAVE_PRAGMA_PACK
 #pragma pack(0)
@@ -800,9 +818,12 @@ struct sdr_record_list {
 #define SENSOR_TYPE_MAX 0x2C
 
 struct sensor_reading {
-	char		s_id[17];		/* name of the sensor */
+	char		s_id[SDR_ID_STRING_MAX + 1]; /* sensor name, null-terminated */
-	struct sdr_record_full_sensor    *full;
+	union {
-	struct sdr_record_compact_sensor *compact;
+		struct sdr_record_full_sensor    *full;
+		struct sdr_record_compact_sensor *compact;
+		void *raw;
+	};
 	uint8_t		s_reading_valid;	/* read value valididity */
 	uint8_t		s_scanning_disabled;	/* read of value disabled */
 	uint8_t		s_reading_unavailable;	/* read value unavailable */
@@ -848,7 +869,7 @@ uint8_t *ipmi_sdr_get_record(struct ipmi_intf *intf, struct sdr_get_rs *header,
 void ipmi_sdr_end(struct ipmi_sdr_iterator *i);
 int ipmi_sdr_print_sdr(struct ipmi_intf *intf, uint8_t type);
 
-int ipmi_sdr_print_name_from_rawentry(uint16_t id, uint8_t type,uint8_t * raw);
+int sdr_get_name_from_rawentry(uint8_t type, void *raw, char *buf, size_t len);
 int ipmi_sdr_print_rawentry(struct ipmi_intf *intf, uint8_t type, uint8_t * raw,
 			    int len);
 int ipmi_sdr_print_listentry(struct ipmi_intf *intf,

lib/ipmi_fru.c

@@ -3042,7 +3042,6 @@ __ipmi_fru_print(struct ipmi_intf * intf, uint8_t id)
 int
 ipmi_fru_print(struct ipmi_intf * intf, struct sdr_record_fru_locator * fru)
 {
-	char desc[17];
 	uint8_t  bridged_request = 0;
 	uint32_t save_addr;
 	uint32_t save_channel;
@@ -3077,10 +3076,10 @@ ipmi_fru_print(struct ipmi_intf * intf, struct sdr_record_fru_locator * fru)
 		fru->device_id == 0)
 		return 0;
 
-	memset(desc, 0, sizeof(desc));
+	printf("FRU Device Description : %.*s (ID %d)\n"
-	memcpy(desc, fru->id_string, __min(fru->id_code & 0x01f, sizeof(desc)));
+	       , SDR_ID_STRLEN(fru)
-	desc[fru->id_code & 0x01f] = 0;
+	       , fru->id_string
-	printf("FRU Device Description : %s (ID %d)\n", desc, fru->device_id);
+	       , fru->device_id);
 
 	switch (fru->dev_type_modifier) {
 	case 0x00:
@@ -3192,7 +3191,9 @@ ipmi_fru_print_all(struct ipmi_intf * intf)
 				/* set new target address to satellite controller */
 				intf->target_addr = mc->dev_slave_addr;
 
-				printf("FRU Device Description : %-16s\n", mc->id_string);
+				printf("FRU Device Description : %.*s\n"
+				       , SDR_ID_STRLEN(fru)
+				       , fru->id_string);
 
 				/* print the FRU by issuing FRU commands to the satellite     */
 				/* controller.						      */

lib/ipmi_sdr.c

@@ -1574,22 +1574,14 @@ ipmi_sdr_read_sensor_value(struct ipmi_intf *intf,
 
 	/* Initialize to reading valid value of zero */
 	memset(&sr, 0, sizeof(sr));
+	sr.raw = sensor;
 
 	switch (sdr_record_type) {
-		unsigned int idlen;
+		case SDR_RECORD_TYPE_FULL_SENSOR:
-		case (SDR_RECORD_TYPE_FULL_SENSOR):
+			SDR_ID_TO_CSTRING(sr.s_id, sr.full);
-			sr.full = (struct sdr_record_full_sensor *)sensor;
-			idlen = sr.full->id_code & 0x1f;
-			idlen = idlen < sizeof(sr.s_id) ?
-						idlen : sizeof(sr.s_id) - 1;
-			memcpy(sr.s_id, sr.full->id_string, idlen);
 			break;
 		case SDR_RECORD_TYPE_COMPACT_SENSOR:
-			sr.compact = (struct sdr_record_compact_sensor *)sensor;
+			SDR_ID_TO_CSTRING(sr.s_id, sr.compact);
-			idlen = sr.compact->id_code & 0x1f;
-			idlen = idlen < sizeof(sr.s_id) ?
-						idlen : sizeof(sr.s_id) - 1;
-			memcpy(sr.s_id, sr.compact->id_string, idlen);
 			break;
 		default:
 			return NULL;
@@ -2248,13 +2240,12 @@ int
 ipmi_sdr_print_sensor_eventonly(struct ipmi_intf *intf,
 				struct sdr_record_eventonly_sensor *sensor)
 {
-	char desc[17];
+	char desc[SDR_ID_STRING_MAX + 1];
 
 	if (!sensor)
 		return -1;
 
-	memset(desc, 0, sizeof (desc));
+	SDR_ID_TO_CSTRING(desc, sensor);
-	snprintf(desc, sizeof(desc), "%.*s", (sensor->id_code & 0x1f) + 1, sensor->id_string);
 
 	if (verbose) {
 		printf("Sensor ID              : %s (0x%x)\n",
@@ -2297,13 +2288,12 @@ ipmi_sdr_print_sensor_eventonly(struct ipmi_intf *intf,
 int
 ipmi_sdr_print_sensor_mc_locator(struct sdr_record_mc_locator *mc)
 {
-	char desc[17];
+	char desc[SDR_ID_STRING_MAX + 1];
 
 	if (!mc)
 		return -1;
 
-	memset(desc, 0, sizeof (desc));
+	SDR_ID_TO_CSTRING(desc, mc);
-	snprintf(desc, sizeof(desc), "%.*s", (mc->id_code & 0x1f) + 1, mc->id_string);
 
 	if (verbose == 0) {
 		if (csv_output)
@@ -2391,10 +2381,12 @@ ipmi_sdr_print_sensor_mc_locator(struct sdr_record_mc_locator *mc)
 int
 ipmi_sdr_print_sensor_generic_locator(struct sdr_record_generic_locator *dev)
 {
-	char desc[17];
+	char desc[SDR_ID_STRING_MAX + 1];
 
-	memset(desc, 0, sizeof (desc));
+	if (!dev)
-	snprintf(desc, sizeof(desc), "%.*s", (dev->id_code & 0x1f) + 1, dev->id_string);
+		return -1;
+
+	SDR_ID_TO_CSTRING(desc, dev);
 
 	if (!verbose) {
 		if (csv_output)
@@ -2446,10 +2438,12 @@ ipmi_sdr_print_sensor_generic_locator(struct sdr_record_generic_locator *dev)
 int
 ipmi_sdr_print_sensor_fru_locator(struct sdr_record_fru_locator *fru)
 {
-	char desc[17];
+	char desc[SDR_ID_STRING_MAX + 1];
 
-	memset(desc, 0, sizeof (desc));
+	if (!fru)
-	snprintf(desc, sizeof(desc), "%.*s", (fru->id_code & 0x1f) + 1, fru->id_string);
+		return -1;
+
+	SDR_ID_TO_CSTRING(desc, fru);
 
 	if (!verbose) {
 		if (csv_output)
@@ -2609,17 +2603,20 @@ ipmi_sdr_print_sensor_oem(struct sdr_record_oem *oem)
 	return rc;
 }
 
-/* ipmi_sdr_print_name_from_rawentry  -  Print SDR name  from raw data
+/**
+ * Get SDR record name from raw data
  *
- * @type:	sensor type
+ * @param[in]   type     SDR record type
- * @raw:	raw sensor data
+ * @param[in]   raw      raw SDR record data
+ * @param[out]  buf      The SDR record description target buffer
+ * @param[in]   len      The length of the target buffer
  *
- * returns 0 on success
+ * @returns Error status
- * returns -1 on error
+ * @retval  0 Success
+ * @retval -1 Error
  */
 int
-ipmi_sdr_print_name_from_rawentry(uint16_t id,
+sdr_get_name_from_rawentry(uint8_t type, void *raw, char *buf, size_t len)
-                                  uint8_t type, uint8_t *raw)
 {
    union {
       struct sdr_record_full_sensor *full;
@@ -2630,47 +2627,50 @@ ipmi_sdr_print_name_from_rawentry(uint16_t id,
       struct sdr_record_mc_locator *mcloc;
       struct sdr_record_entity_assoc *entassoc;
       struct sdr_record_oem *oem;
+      void *raw;
    } record;
 
    int rc =0;
-   char desc[17];
+   char desc[SDR_ID_STRING_MAX + 1] = { 0 };
-   const char *id_string;
+   record.raw = raw;
-   uint8_t id_code;
-   memset(desc, ' ', sizeof (desc));
 
    switch ( type) {
+      /* Sensor records */
+
       case SDR_RECORD_TYPE_FULL_SENSOR:
-      record.full = (struct sdr_record_full_sensor *) raw;
+      SDR_ID_TO_CSTRING(desc, record.full);
-      id_code = record.full->id_code;
-      id_string = record.full->id_string;
       break;
 
       case SDR_RECORD_TYPE_COMPACT_SENSOR:
-      record.compact = (struct sdr_record_compact_sensor *) raw	;
+      SDR_ID_TO_CSTRING(desc, record.compact);
-      id_code = record.compact->id_code;
-      id_string = record.compact->id_string;
       break;
 
       case SDR_RECORD_TYPE_EVENTONLY_SENSOR:
-      record.eventonly  = (struct sdr_record_eventonly_sensor *) raw ;
+      SDR_ID_TO_CSTRING(desc, record.eventonly);
-      id_code = record.eventonly->id_code;
+      break;
-      id_string = record.eventonly->id_string;
+
+      /* Device locator records */
+
+      case SDR_RECORD_TYPE_GENERIC_DEVICE_LOCATOR:
+      SDR_ID_TO_CSTRING(desc, record.genloc);
+      break;
+
+      case SDR_RECORD_TYPE_FRU_DEVICE_LOCATOR:
+      SDR_ID_TO_CSTRING(desc, record.fruloc);
       break;
 
       case SDR_RECORD_TYPE_MC_DEVICE_LOCATOR:
-      record.mcloc  = (struct sdr_record_mc_locator *) raw ;
+      SDR_ID_TO_CSTRING(desc, record.mcloc);
-      id_code = record.mcloc->id_code;
-      id_string = record.mcloc->id_string;
       break;
 
+      /* All other records don't have the id_string field */
+
       default:
       rc = -1;
    }
-   if (!rc) {
-       snprintf(desc, sizeof(desc), "%.*s", (id_code & 0x1f) + 1, id_string);
-   }
 
-   lprintf(LOG_INFO, "ID: 0x%04x , NAME: %-16s", id, desc);
+   memcpy(buf, desc, __min(sizeof(buf), len));
+
    return rc;
 }
 

lib/ipmi_sdradd.c

@@ -250,6 +250,7 @@ sdrr_get_records(struct ipmi_intf *intf, struct ipmi_sdr_iterator *itr,
 
   while ((header = ipmi_sdr_get_next_header(intf, itr))) {
     struct sdr_record_list *sdrr;
+    char desc[SDR_ID_STRING_MAX + 1];
 
     sdrr = malloc(sizeof (struct sdr_record_list));
     if (!sdrr) {
@@ -263,7 +264,8 @@ sdrr_get_records(struct ipmi_intf *intf, struct ipmi_sdr_iterator *itr,
     sdrr->type = header->type;
     sdrr->length = header->length;
     sdrr->raw = ipmi_sdr_get_record(intf, header, itr);
-    (void)ipmi_sdr_print_name_from_rawentry(sdrr->id, sdrr->type,sdrr->raw);
+    (void)sdr_get_name_from_rawentry(sdrr->type, sdrr->raw, desc, sizeof(desc));
+    lprintf(LOG_INFO, "ID: 0x%04x , NAME: %s", sdrr->id, desc);
 
     /* put in the record queue */
     if (!queue->head)