[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [dmidecode] [PATCH v2] update dmidecode to parse Modern Management C
|
From: |
Neil Horman |
|
Subject: |
Re: [dmidecode] [PATCH v2] update dmidecode to parse Modern Management Controller blocks |
|
Date: |
Tue, 7 Aug 2018 11:51:32 -0400 |
|
User-agent: |
Mutt/1.10.1 (2018-07-13) |
On Tue, Aug 07, 2018 at 02:51:51PM +0200, Jean Delvare wrote:
> 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.
>
https://www.dmtf.org/sites/default/files/standards/documents/DSP0270_1.0.0.pdf
and
https://www.dmtf.org/sites/default/files/standards/documents/DSP0134_3.2.0.pdf
Its the SMBIOS reference specification. The above is the latest version, but
the structure layout was added in version 3.0.0 (hence the version check that I
made in the origional case statement). Prior to that this structure is
undefined. Note that with version 3.2.0 there are other management inteface
types besides redfish. However, I'm currently working on a redfish project, and
do not have access to any other interface types, and so have left those uncoded.
I do note, looking at them a bit closer, a few discrepancies. The former lists
the minimal type 42 strucute as being 0x9 bytes, while the smbios spec lists it
as 0xb bytes. That should probably be fixed up.
> 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:
>
How on earth did that all get printed? That looks like the code managed to take
both branches of the version check clause in the type 42 case. The Interface
Type and Vendor ID fields are the legacy printouts from prior to my patch, while
the remainder is the output from decode_management_controller_structure(). I
would expect the former, as I imagine your system has an SMBIOS that conforms to
a version of the spec prior to 3.0.0, but the later should not be there, and the
if/else clause seems pretty clear to me. Can you send me a dump of your smbios
so that I can compare?
also, I'm attaching a copy of my smbios dump for your reference.
Neil
> 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
>
- Re: [dmidecode] [PATCH] update dmidecode to parse Modern Management Controller blocks, (continued)
- Re: [dmidecode] [PATCH] update dmidecode to parse Modern Management Controller blocks, Jean Delvare, 2018/08/07
- Re: [dmidecode] [PATCH] update dmidecode to parse Modern Management Controller blocks, Neil Horman, 2018/08/07
- Re: [dmidecode] [PATCH] update dmidecode to parse Modern Management Controller blocks, Jean Delvare, 2018/08/07
- Re: [dmidecode] [PATCH] update dmidecode to parse Modern Management Controller blocks, Neil Horman, 2018/08/07
- Re: [dmidecode] [PATCH] update dmidecode to parse Modern Management Controller blocks, Jean Delvare, 2018/08/08
- Re: [dmidecode] [PATCH] update dmidecode to parse Modern Management Controller blocks, Neil Horman, 2018/08/08
- Re: [dmidecode] [PATCH] update dmidecode to parse Modern Management Controller blocks, Jean Delvare, 2018/08/09
- Re: [dmidecode] [PATCH] update dmidecode to parse Modern Management Controller blocks, Neil Horman, 2018/08/09
[dmidecode] [PATCH v2] update dmidecode to parse Modern Management Controller blocks, Neil Horman, 2018/08/02
Re: [dmidecode] [PATCH v2] update dmidecode to parse Modern Management Controller blocks, Neil Horman, 2018/08/10
Re: [dmidecode] [PATCH v2] update dmidecode to parse Modern Management Controller blocks, Jean Delvare, 2018/08/12
[dmidecode] [PATCH v3] update dmidecode to parse Modern Management Controller blocks, Neil Horman, 2018/08/07