Re: [PATCH] Read SPD data from a hexdump (take 2)

[Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>]

Hi Trent,

On Sun, 16 Mar 2008 14:33:23 -0700 (PDT), Trent Piepho wrote:
> This is updated version of the previous patch.  I realized that I used four
> space indent in the last one.  I changed it to 8, and then mode the hexdump
> reading code to another function as it was getting too indented.  Also, it
> can now understand the format of "eeprog".
> 
> This adds a "-x" option to decode_dimms.pl, which lets one supply a list of
> file names to read SPD data from.  It can parse various hexdump formats,
> such as the output from i2cdump and the util-linux and Busybox hexdump
> progams run on a sysfs eeprom file.
> 
> Useful for decoding SPD data that you cut and pasted from a manufacturer's
> website or from a DIMM installed on an embeded system that does not have
> perl/etc, but does have a serial console with busybox.

Sounds like a good idea. Review:

> 
> -----
> --- i2c-tools/eeprom/decode-dimms.pl.orig	2008-03-16 14:19:21.000000000 -0700
> +++ i2c-tools/eeprom/decode-dimms.pl	2008-03-16 14:27:15.000000000 -0700
> @@ -46,7 +46,7 @@
>  use strict;
>  use POSIX;
>  use Fcntl qw(:DEFAULT :seek);
> -use vars qw($opt_html $opt_body $opt_bodyonly $opt_igncheck $use_sysfs
> +use vars qw($opt_html $opt_body $opt_bodyonly $opt_igncheck $use_sysfs $use_hexdump
>  	    @vendors %decode_callback $revision);
> 
>  $revision = '$Revision: 5089 $ ($Date: 2008-01-04 08:34:36 -0800 (Fri, 04 Jan 2008) $)';
> @@ -1081,10 +1081,51 @@
>  	printl $l, $temp;
>  }
> 
> +# Read various hex dump style formats:	hexdump, hexdump -C, i2cdump, eeprog

Space instead of tab after the colon.

> +# note that normal 'hexdump' format on a little-endian system byte-swaps
> +# words, using hexdump -C is better.

The function below chokes on the header line of i2cdump output:

<file>
     0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f    0123456789abcdef
00: 80 08 08 0e 0b 60 48 00 05 50 60 02 82 04 04 00    ?????`H.?P`????.
10: ...
</file>

Can't parse data at eeprom/decode-dimms.pl line 1119, <F> line 1.

Of course I can delete this first line from the file and then it works,
but it would be better if the function could simply ignore this header
line.

> +sub readhexdump ($) {

Coding style: opening curly brace goes on the next line. I'd also
suggest renaming the function to read_hexdump.

> +	my $addr = 0;
> +	my $repstart = 0;
> +	my @bytes;
> +
> +	open F, '<', $_[0] or die "Unable to open: $_[0]";
> +	while(<F>) {

Coding style: space after while.

> +		chomp;
> +		if(/^\*$/) {

Coding style: space after if, and again several times below.

> +			$repstart = $addr;
> +			next;
> +		}
> +		/^(?:0000 )?([a-fA-F\d]{2,7}):? ((:?[a-fA-F\d]{4}\s*){8}|(:?[a-fA-F\d]{2}\s*){16})/ ||
> +		/^(?:0000 )?([a-fA-F\d]{2,7}):?\s*$/ or die "Can't parse data";

You could use the /i regexp modifier so that you don't have to specify
both a-f and A-F each time.

> +		$addr = hex $1;
> +		if ($repstart) {
> +			@bytes[$repstart .. ($addr-1)] = ($bytes[$repstart-1]) x ($addr-$repstart);

You repeat the same byte for all the omitted block... this isn't how I
read man hexdump(1). As I understand it, you are supposed to repeat the
last line, not the last byte.

Alternatively you could drop support for this special format, which
doesn't make much sense for a 256-byte EEPROM if you ask me. And in
practice the only cases where it could happen is for rows of 0x00s or
0xffs - it doesn't make a difference if you omit them completely.

> +			$repstart = 0;
> +		}
> +		last unless defined $2;
> +		foreach (split(/\s+/, $2)) {
> +			if(/^(..)(..)$/) {
> +				$bytes[$addr++] = hex($1);
> +				$bytes[$addr++] = hex($2);

I'm unsure if we want to support this ambiguous format. You assume
big-endian byte order here. Even though you document it in the comment
above, users might not read the source code and so might feed the
script with little-endian byte order data and get noise out of the
script.

> +			} elsif(/^(..)$/) {
> +				$bytes[$addr++] = hex($1);
> +			} else {
> +				print "Can't parse hex word '$_'\n";

I fail to see how this could ever happen.

> +			}
> +		}
> +	}
> +	close F;
> +	return @bytes;
> +}
> +
>  sub readspd64 ($$) { # reads 64 bytes from SPD-EEPROM
>  	my ($offset, $dimm_i) = @_;
>  	my @bytes;
> -	if ($use_sysfs) {
> +	if ($use_hexdump) {
> +		@bytes = readhexdump($dimm_i);
> +		return @bytes[$offset..($offset+63)];
> +	} elsif ($use_sysfs) {
>  		# Kernel 2.6 with sysfs
>  		sysopen(HANDLE, "/sys/bus/i2c/drivers/eeprom/$dimm_i/eeprom", O_RDONLY)
>  			or die "Cannot open /sys/bus/i2c/drivers/eeprom/$dimm_i/eeprom";
> @@ -1103,20 +1144,25 @@
>  	return @bytes;
>  }
> 
> +my @dimm_list;

This one probably deserves being moved to the use vars declaration
above.

> +
>  for (@ARGV) {
>      if (/-h/) {
> -		print "Usage: $0 [-c] [-f [-b]]\n",
> +		print "Usage: $0 [-c] [-f [-b]] [-x file [files..]]\n",
>  			"       $0 -h\n\n",
>  			"  -f, --format            print nice html output\n",
>  			"  -b, --bodyonly          don't print html header\n",
>  			"                          (useful for postprocessing the output)\n",
>  			"  -c, --checksum          decode completely even if checksum fails\n",
> +			"  -x,                     Read data from hexdump files\n",
>  			"  -h, --help              display this usage summary\n";
>  		exit;
>      }
>      $opt_html = 1 if (/-f/);
>      $opt_bodyonly = 1 if (/-b/);
>      $opt_igncheck = 1 if (/-c/);
> +    $use_hexdump = 1 if(/-x/);
> +    push @dimm_list, $_ if($use_hexdump && !/^-[fbcx]$/);

In anticipation of more options being added in the future, I'd
use !/^-/ instead. Your variant above also fails if long options are
used.

>  }
>  $opt_body = $opt_html && ! $opt_bodyonly;
> 
> @@ -1136,21 +1182,24 @@
> 
> 
>  my $dimm_count=0;
> -my @dimm_list;
>  my $dir;
> -if ($use_sysfs) { $dir = '/sys/bus/i2c/drivers/eeprom'; }
> -else { $dir = '/proc/sys/dev/sensors'; }
> -if (-d $dir) {
> -	@dimm_list = split(/\s+/, `ls $dir`);
> -} elsif (! -d '/sys/module/eeprom') {
> -	print "No EEPROM found, are you sure the eeprom module is loaded?\n";
> -	exit;
> +
> +if (!$use_hexdump) {
> +	if ($use_sysfs) { $dir = '/sys/bus/i2c/drivers/eeprom'; }
> +	else { $dir = '/proc/sys/dev/sensors'; }
> +	if (-d $dir) {
> +		@dimm_list = split(/\s+/, `ls $dir`);
> +	} elsif (! -d '/sys/module/eeprom') {
> +		print "No EEPROM found, are you sure the eeprom module is loaded?\n";
> +		exit;
> +	}
>  }
> 
>  for my $i ( 0 .. $#dimm_list ) {
>  	$_=$dimm_list[$i];
>  	if (($use_sysfs && /^\d+-\d+$/)
> -	 || (!$use_sysfs && /^eeprom-/)) {
> +	 || (!$use_sysfs&& /^eeprom-/)

Coding style: space before &&.

> +	 || $use_hexdump) {
>  		my @bytes = readspd64(0, $dimm_list[$i]);
>  		my $dimm_checksum = 0;
>  		$dimm_checksum += $bytes[$_] foreach (0 .. 62);
> @@ -1160,15 +1209,18 @@
>  		$dimm_count++;
> 
>  		print "<b><u>" if $opt_html;
> -		printl2 "\n\nDecoding EEPROM", ($use_sysfs ?
> +		printl2 "\n\nDecoding EEPROM",
> +		        $use_hexdump ? $dimm_list[$i] : ($use_sysfs ?
>  			"/sys/bus/i2c/drivers/eeprom/$dimm_list[$i]" :
>  			"/proc/sys/dev/sensors/$dimm_list[$i]");
>  		print "</u></b>" if $opt_html;
>  		print "<table border=1>\n" if $opt_html;
> -		if (($use_sysfs && /^[^-]+-([^-]+)$/)
> -		 || (!$use_sysfs && /^[^-]+-[^-]+-[^-]+-([^-]+)$/)) {
> -			my $dimm_num=$1 - 49;
> -			printl "Guessing DIMM is in", "bank $dimm_num";
> +		if (!$use_hexdump) {
> +			if (($use_sysfs && /^[^-]+-([^-]+)$/)
> +			 || (!$use_sysfs && /^[^-]+-[^-]+-[^-]+-([^-]+)$/)) {
> +				my $dimm_num=$1 - 49;
> +				printl "Guessing DIMM is in", "bank $dimm_num";
> +			}
>  		}
> 
>  # Decode first 3 bytes (0-2)

Please provide an updated patch and I'll commit it.

-- 
Jean Delvare

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c