Re: [flashrom] DMI matching patch

[Carl-Daniel Hailfinger]

On 03.01.2010 01:09, Michael Karcher wrote:
> ^Matching via DMI: If a board is not uniquely identifiable by PCI
> device/subsystem IDs, a string can be specified to be looked for
> (case-sensitive, substring or anchored) for now in one of the following
> DMI items in addition to matching the PCI IDs:
> System Manufacturer
> System Product Name
> System Version
> Baseboard Manufacturer
> Baseboard Product Name
> Baseboard Version
>
> Strings are anchored re-like (^ at the beginning, $ at the end), but
> there are no plans to support full regular expressions. Field selection
> has been intentionally omitted as the match via PCI IDs should be strong
> enough that no cross-pollution between the different strings should ever
> occur, as long as the string is meaningfull.
>
> The match is only made if DMI info is available and the string matches.
> If no DMI info is available and the PCI IDs match, a warning is printed
> as the board can not be autodetected.
>
> It's still open to discussion whether we add an DMI override switch to
> specify a string that will definitely match, and whether this switch is
> only used if no DMI is available or whether it overrides or augments DMI
> data.

Hm. Good question. I'd leave that to future discussion, though.
> DMI data is currently read using dmidecode. This tool is available for
> all major platforms except MacOS X. I heard that there also is a MacOS X
> version of dmidecode, but didn't investigate that.
> Signed-Off-By: Michael Karcher
>
> Revision 2 of this patch:
> adjust coding style (one item per line, FILE *dmidecode_pipe, space
>
> after if, for, while) [carldani, comparing to other flashrom code]
> Implement anchoring of the string [inspired by carldani]
> use snprintf [agaran]
> dmidecode path in separate constant [inspired by carldani]

Thanks. I stripped everything I agree with, the review below only has
the code snippets where I have comments.
> Index: dmi.c
>
> --- dmi.c	(Revision 0)
> +++ dmi.c	(Revision 0)
> @@ -0,0 +1,142 @@
> +/*
> This file is part of the flashrom project.
> Copyright (C) 2009 Michael Karcher
> This program is free software; you can redistribute it and/or modify
> it under the terms of the GNU General Public License as published by
> the Free Software Foundation; either version 2 of the License, or
> (at your option) any later version.
> This program is distributed in the hope that it will be useful,
> but WITHOUT ANY WARRANTY; without even the implied warranty of
> MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> GNU General Public License for more details.
> You should have received a copy of the GNU General Public License
> along with this program; if not, write to the Free Software
> Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301 USA
> */
>
> +#include <string.h>
> +#include <stdio.h>
>
> +#include "flash.h"
>
> +enum dmi_strings {
>    DMI_SYS_VENDOR,
>    DMI_SYS_PRODUCT,
>    DMI_SYS_VERSION,
>    DMI_BB_VENDOR,
>    DMI_BB_PRODUCT,
>    DMI_BB_VERSION,
>    DMI_ID_INVALID /* This must always be the last entry */
>
> +};

Please use tabs instead of spaces for indentation (all enum values above).
> +const char * const dmidecode_names[DMI_ID_INVALID] = {
>    "system-manufacturer",
>    "system-product-name",

Please use tabs instead of spaces for indentation (the two strings above).
> "system-version",
> "baseboard-manufacturer",
> "baseboard-product-name",
> "baseboard-version"
>
> +};
>
> +#define DMI_COMMAND_LEN_MAX 260
> +const char * dmidecode_command = "dmidecode";

Remove space after *.
> +int has_dmi_support = 0;
> +char dmistrings[DMI_ID_INVALID][80];

I'm not so happy about the static allocation here. Can't we use malloc()
for the read buffer and strdup() for filling in dmistrings?
> +void dmi_init(void)
> +{
> FILE *dmidecode_pipe;
> int i;
> for (i = 0; i < DMI_ID_INVALID; i++)
> {
> char commandline[DMI_COMMAND_LEN_MAX+40];
> snprintf(commandline, sizeof(commandline),
>          "%s -s %s", dmidecode_command, dmidecode_names[i]);
> dmidecode_pipe = popen(commandline, "r");

The popen() man page suggests fflush(NULL) before popen(). Not sure.
I'm not seeing any error checking on popen() here.
> fgets(dmistrings[i], sizeof(dmistrings[i]), dmidecode_pipe);

Hm. Check the result of fgets()?
> if (ferror(dmidecode_pipe))
> {
> 	printf_debug("DMI pipe read error");
> 	pclose(dmidecode_pipe);
> 	return;
> }
> /* Toss all output above 80 characters away to prevent
>    deadlock on pclose. */
> while (!feof(dmidecode_pipe))
> 	getc(dmidecode_pipe);
> if (pclose(dmidecode_pipe) != 0)
> {
> 	printf_debug("DMI pipe close error");
> 	return;
> }
> /* chomp trailing newline */
> if (dmistrings[i][0] != 0 &&
>     dmistrings[i][strlen(dmistrings[i]) - 1] == '\n')
>    	dmistrings[i][strlen(dmistrings[i]) - 1] = 0;

Please use tabs instead of spaces for indentation (line above).
> printf_debug("DMI string %d: %s\n", i, dmistrings[i]);
> }
> has_dmi_support = 1;
>
> +}
>
> +/**
> Does an substring/prefix/postfix/whole-string match.
> The pattern is matched as-is. The only metacharacters supported are '^'
> at the beginning and '$' at the end. So you can look for "^prefix",
> "suffix$", "substring" or "^complete string$".
> @param value The string to check.
> @param pattern The pattern.
> @return Nonzero if pattern matches.
> */
>
> +static int dmi_compare(const char * value, const char * pattern)

Please remove the space after *.
> +{
>    int anchored = 0;
>    int patternlen;
>    /* The empty string is part of all strings */
>    if (pattern[0] == 0)
>            return 1;
>    if (pattern[0] == '^') {
>            anchored = 1;
>            pattern++;
>    }
>    patternlen = strlen(pattern);
>    if (pattern[patternlen - 1] == '$') {
>            int valuelen = strlen(value);
>            patternlen--;
>            if(patternlen > valuelen)
>                    return 0;
>            /* full string match: require same length */
>            if(anchored && valuelen != patternlen)

Additional () please. Precedence is nice, but readability is even better.
>                    return 0;
>            /* start character to make ends match */
>            value += valuelen - patternlen;
>            anchored = 1;
>    }
>    if (anchored)
>            return strncmp(value, pattern, patternlen) == 0;
>    else
>            return strstr(value, pattern) != NULL;
>
> +}

Please use tabs instead of spaces for indentation (whole function above).
> +int dmi_match(const char * identifier)
> +{
> int i;
> if (!has_dmi_support)
> return 0;
> for (i = 0;i < DMI_ID_INVALID; i++)
> if (dmi_compare(dmistrings[i], identifier))

I believe the "match any of the strings" policy is fundamentally wrong.
If you and Luc think specifying multiple strings for the match is
overkill/overdesign, please do at least allow specifying the DMI string
this should be compared with.
Since this is a policy decision, we could have the inner loop here
inside #ifdef FIXME_POLICY_DECISION or whatever (resulting in a
non-match for all inputs) to get the infrstructure merged, then finish
the policy decision in a separate patch.
> 	return 1;
> return 0;
>
> +}
> Index: board_enable.c
> ===================================================================
> --- board_enable.c	(Revision 824)
> +++ board_enable.c	(Arbeitskopie)
> @@ -1162,6 +1162,11 @@
> provide an as complete set of pci ids as possible; autodetection is the
> preferred behaviour and we would like to make sure that matches are unique.
> If PCI IDs are not sufficient for board matching, the match can be further
> constrained by a string that has to be present in the DMI database for
> the baseboard or the system entry. The match type is case-sensitive
> substring match.

Maybe add some text which tells people to always include DMI IDs if they
create new board matches. Not sure. It would certainly tighten board
matching, and we don't know how bad the subsystem ID overlap between
different boards is.
> The coreboot ids are used two fold. When running with a coreboot firmware,
> the ids uniquely matches the coreboot board identification string. When a
> legacy bios is installed and when autodetection is not possible, these ids
>
> @@ -1314,6 +1319,18 @@
>    		}
>    	}
> if (board->dmi_identifier) {
>         if (!has_dmi_support) {
>                 fprintf(stderr, "WARNING: Can't autodetect %s %s,"
>                        " DMI info unavailable.\n",
>                        board->vendor_name, board->board_name);
>                 continue;
>                    } else {
>                            if (!dmi_match(board->dmi_identifier))
>                                    continue;
>                    }
>            }

Some of the indentation in the block above uses spaces instead of tabs.
Spaces are only OK if they are less than 8 and if they are preceded by
tabs. The most common use case for spaces is indenting function
arguments or expressions.

One more iteration, and I think you can commit.

Regards,
Carl-Daniel