dmidecode-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [dmidecode] [PATCH v2] update dmidecode to parse Modern Management C


From: Jean Delvare
Subject: Re: [dmidecode] [PATCH v2] update dmidecode to parse Modern Management Controller blocks
Date: Tue, 7 Aug 2018 14:51:51 +0200

Hi Neil,

On Thu,  2 Aug 2018 10:55:15 -0400, Neil Horman wrote:
> Starting with version 0x300 the SMBIOS specification defined in more
> detail the contents of the management controller type.  DMTF further
> reserved values to define the Redfish host interface specification.
> Update dmidecode to properly parse and present that information

Please provide links to the documents you used to write this patch. I
don't see anything relevant in the SMBIOS specification itself. Every
document needed to understand the code should be mentioned in the
header comment (search for "Additional references:"). Without that
information, I can only review the code from a technical perspective,
but I can't say anything about its functional correctness.

Note: for the time being, the only machine I have access to which
implements type 42 had the type set to 0xF0 (OEM) and an apparently
invalid vendor ID (0xFF0102FF). Your patch changes the output of
dmidecode from useless information to different but equally useless
(and most certainly wrong) information:

 Handle 0x0014, DMI type 42, 12 bytes
 Management Controller Host Interface
-       Interface Type: OEM
-       Vendor ID: 0xFF0102FF
+               Host Interface Type: Unknown
+               Device Type: Unknown
+               Protocol Records (16):
+                       Protocol ID: 17 (Unknown)
+                       Protocol ID: 00 (Unknown)
+                       Protocol ID: 00 (Unknown)
+                       Protocol ID: 00 (Unknown)
+                       Protocol ID: 00 (Unknown)
+                       Protocol ID: 82 (Unknown)
+                       Protocol ID: 00 (Unknown)
+                       Protocol ID: 00 (Unknown)
+                       Protocol ID: 00 (Unknown)
+                       Protocol ID: 00 (Unknown)
+                       Protocol ID: 00 (Unknown)
+                       Protocol ID: 01 (Unknown)
+                       Protocol ID: 00 (Unknown)
+                       Protocol ID: 00 (Unknown)
+                       Protocol ID: 00 (Unknown)
+                       Protocol ID: 38 (Unknown)

> Signed-off-by: Neil Horman <address@hidden>
> CC: address@hidden
> CC: address@hidden
> CC: address@hidden
> CC: address@hidden
> 
> ---
> Change Notes:
> V1->V2) Updated string formatting to print matching number of bytes
>       for unsigned shorts (address@hidden)
> 
>       Adjusted string format for bDescriptor (address@hidden)
> 
>       Prefaced PCI id's with 0x (address@hidden)

Note for next time: please start a new thread for each new iteration of
a patch. Otherwise the comments about different versions of the patch
can interleave and this create confusion.

Robert, thanks for having reviewed v1 of this patch, this is
appreciated.

> ---
>  dmidecode.c | 332 +++++++++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 316 insertions(+), 16 deletions(-)
> 
> diff --git a/dmidecode.c b/dmidecode.c
> index fa6ecf1..ea7619f 100644
> --- a/dmidecode.c
> +++ b/dmidecode.c
> @@ -63,6 +63,7 @@
>  #include <strings.h>
>  #include <stdlib.h>
>  #include <unistd.h>
> +#include <arpa/inet.h>
>  
>  #ifdef __FreeBSD__
>  #include <errno.h>
> @@ -3447,6 +3448,301 @@ static void dmi_tpm_characteristics(u64 code, const 
> char *prefix)
>                               prefix, characteristics[i - 2]);
>  }
>  
> +/*
> + * Type 42 data offsets
> + */

This should all go in the following section of the source file:

/*
 * 7.43 Management Controller Host Interface (Type 42)
 */

The file is pretty large by now, if we don't group the type-specific
code, it will get messy in no time.

> +
> +/*
> + * Top Level Offsets
> + */
> +#define IFC_TYPE 4
> +#define IFC_SDATA_LEN 5
> +#define IFC_PROTO_RECORD_BASE 7
> +
> +/*
> + * Interface specific data offsets
> + */
> +#define IFC_DEVICE_TYPE 6
> +
> +/*
> + * USB Interface descriptor offsets
> + */
> +#define USB_DESCRIPTOR 7
> +#define USB_VENDOR 0
> +#define USB_PRODUCT 2
> +#define USB_SERIAL_LENGTH 4
> +#define USB_SERIAL_DESCRIPTOR 5
> +
> +/*
> + * PCI Interface descriptor offsets
> + */
> +#define PCI_DESCRIPTOR 7
> +#define PCI_VENDOR 0
> +#define PCI_DEVICE 2
> +#define PCI_SUBVENDOR 4
> +#define PCI_SUBDEVICE 6
> +
> +/*
> + * Protocol Records Offsets
> + */
> +#define IFC_PROTO_COUNT 0
> +
> +/*
> + * Pre Protocol Record Offsets
> + */
> +#define PROTO_REC_ID 0
> +#define PROTO_REC_LEN 1
> +#define PROTO_REC_DATA 2
> +
> +/*
> + * Record data offsets
> + */
> +#define REC_UUID 0
> +#define REC_HOST_IP_ASGN_TYPE 16
> +#define REC_HOST_IP_ADDR_FMT 17
> +#define REC_HOST_ADDR 18
> +#define REC_HOST_MASK 34
> +#define REC_RFSH_DISC_TYPE 50
> +#define REC_RFSH_ADDR_FMT 51
> +#define REC_RFSH_ADDR 52
> +#define REC_RFSH_MASK 68
> +#define REC_RFSH_PORT 84
> +#define REC_RFSH_VLAN 86
> +#define REC_RFSH_HOST_LEN 90
> +#define REC_RFSH_HOSTNAME 91
> +
> +/*
> + * TYPE 42 Data Field Values
> + */
> +
> +/*
> + * Top level values
> + */

What does "top level" means in this context? No idea.

> +#define MCH_NET_HOST_IFC 0x40
> +
> +/*
> + * Interface specific data values
> + */
> +#define IFC_DEVICE_UNKNOWN 0x0
> +#define IFC_DEVICE_USB 0x2
> +#define IFC_DEVICE_PCI 0x3
> +#define IFC_DEVICE_OEM_BASE 0x80
> +
> +/*
> + * Protocol record data values
> + */
> +#define REDFISH_OVER_IP 0x4
> +
> +/*
> + * Protocol record data values
> + */
> +#define IP_UNKNOWN 0
> +#define IP_STATIC 0x1
> +#define IP_DHCP 0x2
> +#define IP_AUTOCONF 0x3
> +#define IP_HOSTSEL 0x4
> +
> +#define ADDR_UNKNOWN 0
> +#define ADDR_IPV4 0x1
> +#define ADDR_IPV6 0x2

I'm quite skeptical about that long list of defines. You will note that
I made the choice to not introduce such defines for any other DMI
structure type. The rationale is that they are only used locally, and
in general only once, and I find it in fact easier to match the
constant directly to the specification, than to have to look up a
separate define. Also note that the code consistently uses hexadecimal
for all field offsets, because that's what the specification uses. You
have a mix of decimal and hexadecimal.

Also all your defines are specific to type 42 but nothing in their name
says so. Imagine how many collisions we would get if we would have
defines for all other types using the same (lack of) pattern... Which
was one of the reasons why I did not go that way in the first place.

In my experience, if the use of a constant directly in the code is not
clear enough, it can be solved by adding a comment, instead of a
define. As a matter of fact, you have many such comments in your code
already (which is very good), making the defines redundant
(IFC_DEVICE_USB and IFC_DEVICE_PCI for example, but also the IP
addresses and masks). Good variable names and printf strings also tend
to clarify what data we are dealing with.

> +static void decode_management_controller_structure(const struct dmi_header 
> *h, const char *prefix)

For consistency, the name of this function should be prefixed with
"dmi_". "decode" and "structure" on the other hand are pretty much
implied and no other type-specific function name has them.

There is no strict 80-column deadline in the dmidecode source code,
however please consider splitting long lines when there is no extra
cost in doing so.

> +{
> +     u8 *data = h->data;
> +     u8 type = data[IFC_TYPE];
> +     u8 len = data[IFC_SDATA_LEN];
> +     u8 count;
> +     const char *devname[] = {
> +             "Unknown",
> +             "Unknown",
> +             "USB",
> +             "PCI[e]",

I think I would prefer "PCI/PCIe".

> +             "OEM",
> +     };
> +
> +     if (h->length < 0x9) {
> +             printf("%s Invalid structure\n", prefix);
> +             return;
> +     }

We don't print such a message anywhere else. The standard action if a
structure is not long enough is to just stop decoding it silently.

Also, according to version 3.2.0 of the specification, the minimum
length for this structure is now 0xB.

You also need to ensure that 0x06 + len + 1 does fit in h->length.
Otherwise you may end up reading beyond the end of the structure.

> +
> +     printf("%sHost Interface Type: ", prefix);
> +     if (type != MCH_NET_HOST_IFC)
> +             printf("Unknown\n");
> +     else
> +             printf("Network\n");

Could this be consolidated into dmi_management_controller_host_type()?

> +
> +

Extra blank line.

> +     if (len != 0) {
> +             type = data[IFC_DEVICE_TYPE];

This is dependent on the interface type. I don't know about other
types, but at least for the OEM type (0xf0) the above statement is not
valid.

> +             if (type >= IFC_DEVICE_OEM_BASE)
> +                     type = IFC_DEVICE_UNKNOWN;

This causes OEM device types to be reported as "Unknwown" instead of
"OEM".

> +
> +             printf("%sDevice Type: %s\n", prefix, devname[type]);

This is unsafe. type could be well beyond the size of devname[].

The dmidecode.c source file has dozen of examples of how this should be
handled.

> +             if (type == IFC_DEVICE_USB) {
> +                     /* USB */
> +                     u8 *usbdata = &data[USB_DESCRIPTOR];

You need to ensure that len is sufficient to cover all the USB-specific
fields.

> +                     printf("%s\tidVendor: 0x%04x\n", prefix, (unsigned 
> short)usbdata[USB_VENDOR]);

Please use WORD() as discussed somewhere else in this discussion thread.

> +                     printf("%s\tidProduct: 0x%04x\n", prefix, (unsigned 
> short)usbdata[USB_PRODUCT]);
> +                     printf("%s\tSerialNumber:\n", prefix);
> +                     printf("%s\t\tbDescriptor: 0x%02x\n", prefix, 
> usbdata[USB_SERIAL_DESCRIPTOR]);
> +                     /* Note bString is not printable here, so skip it */

The comment is confusing. Why is bString not printable here? You said
too much, or not enough.

> +             } else if (type == IFC_DEVICE_PCI) {
> +                     /* PCI */
> +                     u8 *pcidata = &data[PCI_DESCRIPTOR];

You need to ensure that len is sufficient to cover all the PCI-specific
fields.

> +                     printf("%s\tVendorID: 0x%04x\n", prefix, (unsigned 
> short)pcidata[PCI_VENDOR]);
> +                     printf("%s\tDeviceID: 0x%04x\n", prefix, (unsigned 
> short)pcidata[PCI_DEVICE]);
> +                     printf("%s\tSubVendorID: 0x%04x\n", prefix, (unsigned 
> short)pcidata[PCI_SUBVENDOR]);
> +                     printf("%s\tSubDeviceID: 0x%04x\n", prefix, (unsigned 
> short)pcidata[PCI_SUBDEVICE]);
> +             }
> +             /* Don't mess with unknown types for now */
> +     }
> +
> +     /*
> +      * Move to the Protocol Count Area from Table 1

What is "Table 1"?

> +      * Data[6] points to the start of the interface specific
> +      * data, and Data[5] is the length of that region
> +      */
> +     data = &data[IFC_PROTO_RECORD_BASE+data[5]];

Notice the inconsistent mix of define and raw number, as well as the
mix of case for "data" ;-) At this point, data[5] is already known as
"len", you should use it for consistency. Also, you mention data[6] but
IFC_PROTO_RECORD_BASE is 7, not 6. This is the kind of nasty side
effects of using defines, which I was mentioning previously.

> +
> +     /* Get the protocol records count */
> +     count = (u8)data[IFC_PROTO_COUNT];

Looks wrong to me. At this point, data points to h + 7 + len bytes.
This corresponds to the "Protocol Records" field in the specification.
However the statement above assumes that data is pointing to the
"Number of Protocol Records" field.

> +     if (count) {

I believe that this block of code is crying to be a separate function,
that would decode one protocol. Surrounded by a loop to walk over the
list of protocols.

> +             int i, j, k;
> +             u8 rid;
> +             u8 rlen;
> +             u8 *rec = &data[1]; /*first record starts after count value */

Leading space and capital in comment, please.

> +             u8 *rdata;
> +             u8 buf[64];
> +             u8 uuid[37];
> +             u8 assignval;
> +             u8 addrtype;
> +             u8 hlen;
> +             char hname[257];

hlen is 255 maximum. Plus the NUL terminator, 256, not 257. But you
probably don't need that buffer in the first place, see below.

Note: such a long list of local variables is usually a good sign that
some of the code should be moved to helper functions. More about that
below.

> +
> +             const char *assigntype[] = {
> +                     "Unknown",
> +                     "Static",
> +                     "DHCP",
> +                     "AutoConf",
> +                     "Host Selected",
> +             };
> +
> +             const char *addressformat[] = {
> +                     "Unknown",
> +                     "Ipv4",
> +                     "Ipv6",
> +             };

The standard casing is IPv[46] (capital P). There are more occurrences
below.

> +
> +             printf("%sProtocol Records (%02d):\n", prefix, count);
> +             for (i=0; i < count; i++) {

Spaces around "=". Before you attempt to decode a protocol, you have to
ensure that its record fully fits in h->length.

> +                     rid = (u8)rec[PROTO_REC_ID];
> +                     rlen = (u8)rec[PROTO_REC_LEN];

Both casts seem useless.

> +                     rdata = &rec[PROTO_REC_DATA];
> +
> +                     if (rid != REDFISH_OVER_IP) {
> +                             printf("%s\tProtocol ID: %02x (Unknown)\n", 
> prefix, rid);
> +                             goto next;
> +                     }
> +
> +                     printf("%s\tProtocol ID: Redfish over IP\n", prefix);

"Redfish over IP" is the protocol name, not ID. So just "Protocol:"
would do.

> +                     memcpy(buf, &rdata[REC_UUID], 16);
> +
> +                     /* Convert UUID to readable characters */
> +                     for (j=0, k=0; j < 33; j++, k++) {
> +                             if ((j == 8) || (j == 12) || (j == 16) || (j == 
> 20)) {
> +                                     uuid[k] = '-';
> +                                     k++;
> +                             }
> +
> +                             if (j & 0x1)
> +                                     uuid[k] = (buf[j/2] >> 4) + 0x30;
> +                             else
> +                                     uuid[k] = (buf[j/2] & 0x0f) + 0x30;
> +
> +                             if (uuid[j] >= 0x3a)
> +                                     uuid[j] += 7;
> +                     }
> +                     uuid[32] = '\0';
> +                     printf("%s\t\tService UUID: %s\n", prefix, uuid);

Ouch. Please reuse dmi_system_uuid(). If you can't reuse it directly,
adjust it so that you can reuse it. But don't duplicate the effort here.

Also, just for completeness, you declared uuid as a 37 character
buffer, but in the end you NUL-terminate it at offset 32. Makes no
sense to me. "j < 33" in the loop condition looks equally wrong to me.

> +
> +                     assignval = (u8)rdata[REC_HOST_IP_ASGN_TYPE];
> +                     if (assignval > IP_HOSTSEL)
> +                             assignval = IP_UNKNOWN;
> +                     printf("%s\t\tHost IP Assignment Type: %s\n", prefix, 
> assigntype[assignval]);
> +
> +                     addrtype = (u8)rdata[REC_HOST_IP_ADDR_FMT];
> +                     if (addrtype > ADDR_IPV6)
> +                             addrtype = ADDR_UNKNOWN;
> +                     printf("%s\t\tHost IP Address Format: %s\n", prefix, 
> addressformat[addrtype]);

Again, there is a standardized way to deal with such enumerated lists
in dmidecode, please stick to it. Among the obvious benefit of being
consistent, it also allows differentiating between "Unknown" being set
by the BIOS itself and an actual value being set by the BIOS but not
yet known to dmidecode (which is reported as "<OUT OF SPEC>", and is
often addressed by the following version of the specification.)

And again the casts to u8 are not needed.

> +
> +                     /* We only use the Host IP Address and Mask if the 
> assignment type is static */
> +                     if ((assignval == IP_STATIC) || (assignval == 
> IP_AUTOCONF)) {

You don't need all these parentheses.

> +                             /* Prints the Host IP Address */
> +                             printf("%s\t\t%s Address: %s\n", prefix,
> +                                   (addrtype == 0x1 ? "Ipv4" : "Ipv6"),

You assume that the address type must be either IPv4 or IPv6, while you
explicitly test for unknown address type above. This needs proper
"error" handling.

> +                                   (addrtype == 0x1 ?
> +                                     inet_ntop(AF_INET, (char 
> *)&rdata[REC_HOST_ADDR], (char *)buf, 64) :
> +                                     inet_ntop(AF_INET6, (char 
> *)&rdata[REC_HOST_ADDR], (char *)buf, 64)));
> +
> +                             /* Prints the Host IP Mask */
> +                             printf("%s\t\t%s Mask: %s\n", prefix,
> +                                   (addrtype == ADDR_IPV4 ? "Ipv4" : "Ipv6"),
> +                                   (addrtype == ADDR_IPV4 ?
> +                                     inet_ntop(AF_INET, (char 
> *)&rdata[REC_HOST_MASK], (char *)buf, 64) :
> +                                     inet_ntop(AF_INET6, (char 
> *)&rdata[REC_HOST_MASK], (char *)buf, 64)));
> +                     }

Any idea how portable is inet_ntop()? Note that in theory it can fail,
but you don't check for that. I admit I can't see how that would happen
here, but if it did, printf would cause a segfault.

Cast to char* for the source doesn't seem to be needed (inet_ntop takes
void* for src according to the man page).

Several parentheses are again superfluous in the printfs.

There is redundancy among the inet_ntop calls. Instead of:
        addrtype == ADDR_IPV4 ? inet_ntop(AF_INET, ...) : net_ntop(AF_INET6, 
...)
you could do:
        inet_ntop(addrtype == ADDR_IPV4 ? AF_INET : AF_INET6, ...)

The decoding of these two fields is very similar, please consider
refactoring the code to a separate function. Which you could also reuse
below...

> +
> +                     /* Get the Redfish Service IP Discovery Type */
> +                     assignval = (u8)rdata[REC_RFSH_DISC_TYPE];
> +                     if (assignval > IP_HOSTSEL)
> +                             assignval = 0;

Not consistent with how you wrote it for REC_HOST_IP_ASGN_TYPE and
REC_HOST_IP_ADDR_FMT above. Anyway, again, I would prefer if you stick
with the dmidecode way to deal with enumerated values.

> +
> +                     /* Redfish Service IP Discovery type Mirrors Host IP 
> Assignment type */

No need for leading capital for "mirror", methinks.

> +                     printf("%s\t\tRedfish Service IP Discovery Type: %s\n", 
> prefix, assigntype[assignval]);
> +
> +                     /* Get the Redfish Service IP Address Format */
> +                     addrtype = (u8)rdata[REC_RFSH_ADDR_FMT];
> +                     if (addrtype > ADDR_IPV6)
> +                             addrtype = ADDR_UNKNOWN;
> +
> +                     printf("%s\t\tRedfish Service IP Address Format: %s\n", 
> prefix, addressformat[addrtype]);
> +                     if ((assignval == IP_STATIC)  || (assignval == 
> IP_AUTOCONF)) {

Double space before "||". You don't need all these parentheses.

> +                             u16 port;
> +                             u32 vlan;

A blank line here would be appreciated.

> +                             /* Prints the Redfish Service Address */
> +                             printf("%s\t\t%s Redfish Service Address: 
> %s\n", prefix,
> +                                   (addrtype == ADDR_IPV4 ? "Ipv4" : "Ipv6"),
> +                                   (addrtype == ADDR_IPV4 ?
> +                                     inet_ntop(AF_INET, (char 
> *)&rdata[REC_RFSH_ADDR], (char *)buf, 64) :
> +                                     inet_ntop(AF_INET6, (char 
> *)&rdata[REC_RFSH_ADDR], (char *)buf, 64)));
> +
> +                             /* Prints the Redfish Service Mask */
> +                             printf("%s\t\t%s Redfish Service Mask: %s\n", 
> prefix,
> +                                   (addrtype == ADDR_IPV4 ? "Ipv4" : "Ipv6"),
> +                                   (addrtype == ADDR_IPV4 ?
> +                                     inet_ntop(AF_INET, (char 
> *)&rdata[REC_RFSH_MASK], (char *)buf, 64) :
> +                                     inet_ntop(AF_INET6, (char 
> *)&rdata[REC_RFSH_MASK], (char *)buf, 64)));
> +
> +                             port = (u8)rdata[REC_RFSH_PORT];
> +                             vlan = (u16)rdata[REC_RFSH_VLAN];

That cast may crash on IA-64. Use WORD().

Are there holes in the structure? According to your defines, there is
room for 2 bytes for the port and 4 bytes for the vlan. This matches
your variable types, but not the casts above. Please double check.

> +                             printf("%s\t\tRedfish Service Port: %d\n", 
> prefix, port);
> +                             printf("%s\t\tRedfish Service Vlan: %d\n", 
> prefix, vlan);

%u is more appropriate for unsigned values.

> +

Extra blank line.

> +                     }
> +
> +                     hlen = (u8)rdata[REC_RFSH_HOST_LEN];

Useless cast.

> +                     memcpy(hname, &rdata[REC_RFSH_HOSTNAME], hlen);
> +                     hname[hlen] = '\0';
> +                     printf("%s\t\tRedfish Service Hostname: %s\n", prefix, 
> hname);

You can use %s with a precision modifier to avoid the need to copy the
characters to a NUL-terminated buffer. Quoting printf(3):

"If a precision is given, no null byte need be present;"

It's a bit tricky because the precision is not known in advance, but
the following should work:

        printf("%s\t\tRedfish Service Hostname: %*s\n", prefix, hlen,
                &rdata[REC_RFSH_HOSTNAME]);

> +next:
> +                     rec = rec + rlen +1;

You can use "+=".

And I think you are off by 1. Each record is made of ID + length byte +
data, which is 1 + 1 + rlen, so 2 + rlen.

> +             }
> +     }
> +
> +     return;
> +

Extra blank line. And return itself is not needed for a void function.

> +}
> +
>  /*
>   * Main
>   */
> @@ -4582,22 +4878,26 @@ static void dmi_decode(const struct dmi_header *h, 
> u16 ver)
>  
>               case 42: /* 7.43 Management Controller Host Interface */
>                       printf("Management Controller Host Interface\n");
> -                     if (h->length < 0x05) break;
> -                     printf("\tInterface Type: %s\n",
> -                             
> dmi_management_controller_host_type(data[0x04]));
> -                     /*
> -                      * There you have a type-dependent, variable-length
> -                      * part in the middle of the structure, with no
> -                      * length specifier, so no easy way to decode the
> -                      * common, final part of the structure. What a pity.
> -                      */
> -                     if (h->length < 0x09) break;
> -                     if (data[0x04] == 0xF0)         /* OEM */
> -                     {
> -                             printf("\tVendor ID: 0x%02X%02X%02X%02X\n",
> -                                     data[0x05], data[0x06], data[0x07],
> -                                     data[0x08]);
> -                     }
> +

Unneeded blank line.

> +                     if (ver < 0x300) {

Why are you splitting on version 3.0.0? As far as I can see, the
definition for type 42 structures changed in SMBIOS specification
3.2.0, so you should test for ver < 0x0302. That explains why the
output is now broken on my system (which has ver == 0x0301).

I'm very happy that the DMTF changed it, by the way. The original
definition was simply unusable.

Brace goes on following line. Almost all your curly brace placements
are infringing the coding style used by dmidecode, even though I did
not mention it every time so that we could focus on the code itself. I
don't like it either, but that's how things are for now, please be
consistent. If I would start the project today, I would make different
coding style choices, there is no question about that.

> +                             if (h->length < 0x05) break;
> +                             printf("\tInterface Type: %s\n",
> +                                     
> dmi_management_controller_host_type(data[0x04]));
> +                             /*
> +                              * There you have a type-dependent, 
> variable-length
> +                              * part in the middle of the structure, with no
> +                              * length specifier, so no easy way to decode 
> the
> +                              * common, final part of the structure. What a 
> pity.
> +                              */
> +                             if (h->length < 0x09) break;
> +                             if (data[0x04] == 0xF0)         /* OEM */
> +                             {
> +                                     printf("\tVendor ID: 
> 0x%02X%02X%02X%02X\n",
> +                                             data[0x05], data[0x06], 
> data[0x07],
> +                                             data[0x08]);
> +                             }
> +                     } else
> +                             decode_management_controller_structure(h, 
> "\t\t");

The double tab makes the output too indented, I think the prefix should
be just "\t".

>                       break;
>  
>               case 43: /* 7.44 TPM Device */

One issue with your patch is that the Interface Type Specific Data part
of OEM records is no longer decoded for recent SMBIOS implementations.
This could lead to regressions for some users (although to be honest
type 42 records are not exactly popular, so that's essentially
theoretical...)

By the way, I am considering the possibility to stop decoding type 42
records completely for SMBIOS implementations < 3.2.0 because the
definition was just too crappy. I have yet to see a valid type 42
record other than yours...

Thanks,
-- 
Jean Delvare
SUSE L3 Support



reply via email to

[Prev in Thread] Current Thread [Next in Thread]