2
\$\begingroup\$

Please review my System information class.

Systeminfo can be used to retrieve system management data from the BIOS. This data is mostly strings.

Is the code easy to understand and maintainable?

Is the code efficient?

How could it be improved?

systeminfo.hpp:

/*
  SystemInfo class is a C++ wrapper for the System Management BIOS data available on PCs
*/

#ifndef SYSTEMINFO_HPP_
#define SYSTEMINFO_HPP_

#include <string>

class Systeminfo {
public:
    // System information data retrieved on construction and string members populated
    Systeminfo();
    // get product family
    const std::string get_family() const;
    // get manufacturer - generally motherboard or system assembler name
    const std::string get_manufacturer() const;
    // get product name
    const std::string get_productname() const;
    // get BIOS serial number
    const std::string get_serialnumber() const;
    // get SKU / system configuration
    const std::string get_sku() const;
    // get a universally unique identifier for system
    const std::string get_uuid() const;
    // get version of system information
    const std::string get_version() const;

    Systeminfo(Systeminfo const&) = delete;
    Systeminfo& operator=(Systeminfo const&) = delete;

private:
    std::string family_;
    std::string manufacturer_;
    std::string productname_;
    std::string serialnumber_;
    std::string sku_;
    std::string uuid_;
    std::string version_;
};

#endif // SYSTEMINFO_HPP_

systeminfo.cpp:

#include "systeminfo.hpp"

#ifndef _WIN32
#error Currently SystemInfo can only be built for the Windows platform
#endif

#include <Windows.h>

#include <cstdint>
#include <cstdio>
#include <string>

namespace {
    #pragma pack(push) 
    #pragma pack(1)
    /*
    SMBIOS Structure header (System Management BIOS) spec:
    https ://www.dmtf.org/sites/default/files/standards/documents/DSP0134_3.3.0.pdf
    */
    struct SMBIOSHEADER
    {
        uint8_t type;
        uint8_t length;
        uint16_t handle;
    };

    /*
      Structure needed to get the SMBIOS table using GetSystemFirmwareTable API.
      see https://docs.microsoft.com/en-us/windows/win32/api/sysinfoapi/nf-sysinfoapi-getsystemfirmwaretable
    */
    struct SMBIOSData {
        uint8_t  Used20CallingMethod;
        uint8_t  SMBIOSMajorVersion;
        uint8_t  SMBIOSMinorVersion;
        uint8_t  DmiRevision;
        uint32_t  Length;
        uint8_t  SMBIOSTableData[1];
    };

    // System Information (Type 1)
    struct SYSTEMINFORMATION {
        SMBIOSHEADER Header;
        uint8_t Manufacturer;
        uint8_t ProductName;
        uint8_t Version;
        uint8_t SerialNumber;
        uint8_t UUID[16];
        uint8_t WakeUpType;  // Identifies the event that caused the system to power up
        uint8_t SKUNumber;   // identifies a particular computer configuration for sale
        uint8_t Family;
    };
    #pragma pack(pop) 

    // helper to retrieve string at string offset. Optional null string description can be set.
    const char* get_string_by_index(const char* str, int index, const char* null_string_text = "")
    {
        if (0 == index || 0 == *str) {
            return null_string_text;
        }

        while (--index) {
            str += strlen(str) + 1;
        }
        return str;
    }

    // retrieve the BIOS data block from the system
    SMBIOSData* get_bios_data() {
        SMBIOSData *bios_data = nullptr;

        // GetSystemFirmwareTable with arg RSMB retrieves raw SMBIOS firmware table
        // return value is either size of BIOS table or zero if function fails
        DWORD bios_size = GetSystemFirmwareTable('RSMB', 0, NULL, 0);

        if (bios_size > 0) {
            bios_data = (SMBIOSData*)malloc(bios_size);

            // Retrieve the SMBIOS table
            DWORD bytes_retrieved = GetSystemFirmwareTable('RSMB', 0, bios_data, bios_size);

            if (bytes_retrieved != bios_size) {
                free(bios_data);
                bios_data = nullptr;
            }
        }

        return bios_data;
    }


    // locates system information memory block in BIOS table
    SYSTEMINFORMATION* find_system_information(SMBIOSData* bios_data) {

        uint8_t* data = bios_data->SMBIOSTableData;

        while (data < bios_data->SMBIOSTableData + bios_data->Length)
        {
            uint8_t *next;
            SMBIOSHEADER *header = (SMBIOSHEADER*)data;

            if (header->length < 4)
                break;

            //Search for System Information structure with type 0x01 (see para 7.2)
            if (header->type == 0x01 && header->length >= 0x19)
            {
                return (SYSTEMINFORMATION*)header;
            }

            //skip over formatted area
            next = data + header->length;

            //skip over unformatted area of the structure (marker is 0000h)
            while (next < bios_data->SMBIOSTableData + bios_data->Length && (next[0] != 0 || next[1] != 0)) {
                next++;
            }
            next += 2;

            data = next;
        }
        return nullptr;
    }

}

Systeminfo::Systeminfo() {
    SMBIOSData* bios_data = get_bios_data();

    if (bios_data) {
        SYSTEMINFORMATION* sysinfo = find_system_information(bios_data);
        if (sysinfo) {
            const char* str = (const char*)sysinfo + sysinfo->Header.length;

            manufacturer_ = get_string_by_index(str, sysinfo->Manufacturer);
            productname_ = get_string_by_index(str, sysinfo->ProductName);
            serialnumber_ = get_string_by_index(str, sysinfo->SerialNumber);
            version_ = get_string_by_index(str, sysinfo->Version);

            // for v2.1 and later
            if (sysinfo->Header.length > 0x08)
            {
                static const int max_uuid_size{ 50 };
                char uuid[max_uuid_size] = {};
                _snprintf_s(uuid, max_uuid_size, max_uuid_size-1, "%02X%02X%02X%02X-%02X%02X-%02X%02X-%02X%02X-%02X%02X%02X%02X%02X%02X",
                    sysinfo->UUID[0], sysinfo->UUID[1], sysinfo->UUID[2], sysinfo->UUID[3],
                    sysinfo->UUID[4], sysinfo->UUID[5], sysinfo->UUID[6], sysinfo->UUID[7],
                    sysinfo->UUID[8], sysinfo->UUID[9], sysinfo->UUID[10], sysinfo->UUID[11],
                    sysinfo->UUID[12], sysinfo->UUID[13], sysinfo->UUID[14], sysinfo->UUID[15]);

                uuid_ = uuid;
            }

            if (sysinfo->Header.length > 0x19)
            {
                // supported in v 2.4 spec
                sku_ = get_string_by_index(str, sysinfo->SKUNumber);
                family_ = get_string_by_index(str, sysinfo->Family);
            }
        }
        free(bios_data);
    }
}

const std::string Systeminfo::get_family() const {
    return family_;
}

const std::string Systeminfo::get_manufacturer() const {
    return manufacturer_;
}

const std::string Systeminfo::get_productname() const {
    return productname_;
}

const std::string Systeminfo::get_serialnumber() const {
    return serialnumber_;
}

const std::string Systeminfo::get_sku() const {
    return sku_;
}

const std::string Systeminfo::get_uuid() const {
    return uuid_;
}

const std::string Systeminfo::get_version() const {
    return version_;
}

Example program using systeminfo:

#include "systeminfo.hpp"

#include <iostream>
#include <iomanip>
#include <chrono>


int main() {

    auto start = std::chrono::high_resolution_clock::now();

    Systeminfo info;
    std::cout << std::left << ::std::setw(14) << "Manufacturer: " << info.get_manufacturer() << '\n'
        << std::left << ::std::setw(14) << "Product Name: " << info.get_productname() << '\n'
        << std::left << ::std::setw(14) << "Serial No: " << info.get_serialnumber() << '\n'
        << std::left << ::std::setw(14) << "UUID: " << info.get_uuid() << '\n'
        << std::left << ::std::setw(14) << "Version: " << info.get_version() << std::endl;

    if (!info.get_family().empty()) {
        std::cout << std::left << ::std::setw(14) << "Product family: " << info.get_family() << std::endl;
    }

    if (!info.get_sku().empty()) {
        std::cout << std::left << ::std::setw(14) << "SKU/Configuration: " << info.get_sku() << std::endl;
    }

    auto end = std::chrono::high_resolution_clock::now();
    double elapsed_time_ms = std::chrono::duration<double, std::milli>(end - start).count();

    std::cout << "elapsed time for systeminfo: " << elapsed_time_ms << " ms" << std::endl;
}
\$\endgroup\$

1 Answer 1

2
\$\begingroup\$

Consider making Systeminfo a POD

I would separate getting information from the SMBIOS from how to store the result. Make Systeminfo a plain old struct:

struct Systeminfo {
    std::string family;
    std::string manufacturer;
    ...
};

And move the original constructor into a plain function:

Systeminfo get_systeminfo() {
    Systeminfo info;
    ...
    info.family = ...;
    info.manufacturer = ...;
    ...
    return info;
}

The reason is simply because getting the information is a one-shot action, afterwards there is nothing special you can do with the result anymore, so it really doesn't need its own class.

Avoid unnecessary #pragmas

I would avoid using #pragma pack. While the C standard technically allows a compiler to add an arbitrary amount of padding between members, as long as the alignment rules are satisfied, programs need to be able to interoperate with each other, so there are some exact rules of how a struct should be layed out in memory. For example the C ABI on Windows, Linux and other major desktop operating systems will guarantee that in this case, where the members of the structs you declare are all naturally aligned, they will all be tightly packed together.

Add bounds checks

It is possible that a BIOS chip is corrupt, or that the manufacturer just put invalid data in it (yes, this is sadly not uncommon). Consider that strings in the BIOS might not be properly NUL-terminated. So when reading the BIOS data, perform bounds checking to ensure you don't read past the end of it.

Simplify formatting the output

C++ hasn't made formatting strings very easy (although C++20 will fix that with std::format()). But you can simplify your code so you don't need the calls to std::left and std::setw(); just write:

std::cout << "Manufacturer:      " << info.manufacturer << "\n"
          << "Product Name:      " << info.productname << "\n"
          << "Serial No:         " << info.serialnumber << "\n";
          << "UUID:              " << info.uuid << "\n"
          << "Version:           " << info.version << "\n";

If you do want to use programatically format the output, then try to create a function to avoid repeating yourself. For example:

static void format_field(const std::string &name, const std::string &value) {
     std::cout << std::left << std::setw(14) << name << ": " << value << "\n";
}

int main() {
    ...
    format_field("Manufacturer", info.manufacturer);
    format_field("Product Name", info.productname);
    ...
}

That is both much clearer, and whenever you want to change how the output is formatted, you might only have to change one line.

\$\endgroup\$
2
  • \$\begingroup\$ For your bounds checking suggestion, what should I do? Have a maximum read length until find a null char? \$\endgroup\$
    – arcomber
    Sep 8, 2020 at 17:03
  • \$\begingroup\$ @arcomber: You could use ThorsIOUtils (if you don't have C++20) std::cout << IO::make_cppformat("%-14s: %d\n", name, value); With a review needed here :-) codereview.stackexchange.com/q/188812/507 \$\endgroup\$ Sep 9, 2020 at 20:22

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.