[syslinux] [PATCH] Refactor checksize.pl

[Celelibi]

2015-11-19 19:25 UTC+01:00, nico cornu <nicolac76 at yahoo.fr>:
> Le Jeudi 19 novembre 2015 18h43, Celelibi <celelibi at gmail.com> a écrit :
>
>
> 2015-11-19 17:30 UTC+01:00, Nicolas Cornu via Syslinux
> <syslinux at zytor.com>:
>> - Remove padsize argument as it is never used.
>> - Add an usage printed when $file is not set or --help, -h
>>   is the first argument.
>> - Add basic tests for this script.
>> ---
>>  mbr/checksize.pl      | 27 ++++++++++++++++++---------
>>  mbr/checksize_test.sh | 22 ++++++++++++++++++++++
>>  2 files changed, 40 insertions(+), 9 deletions(-)
>>  create mode 100644 mbr/checksize_test.sh
>>
>> diff --git a/mbr/checksize.pl b/mbr/checksize.pl
>> index 4b42327..cbca5b2 100755
>> --- a/mbr/checksize.pl
>> +++ b/mbr/checksize.pl
>> @@ -19,25 +19,34 @@
>>
>>  use bytes;
>>
>> -($file, $maxsize, $padsize) = @ARGV;
>> +($file, $maxsize) = @ARGV;
>> +
>> +if (!defined($file) or $file == "--help" or $file == "-h") {
>> +    $usage = <<"END_USAGE";
>> +Usage: $0 file [maxsize]
>> +If maxsize is not given it will be guessed based on filename:
>> +  - 440 bytes for mbr and gptmbr;
>> +  - 432 bytes for isohdp;
>> +  - 439 bytes for altmbr.
>> +END_USAGE
>> +    die $usage;
>> +}
>>
>>  if (!defined($maxsize)) {
>>      # Defaults based on the filename
>>      if ($file =~ /^mbr[^0-9a-z]/) {
>> -    $maxsize = $padsize = 440;
>> +    $maxsize = 440;
>>      } elsif ($file =~ /^gptmbr[^0-9a-z]/) {
>> -    $maxsize = $padsize = 440;
>> +    $maxsize = 440;
>>      } elsif ($file =~ /^isohdp[fp]x[^0-9a-z]/) {
>> -    $maxsize = $padsize = 432;
>> +    $maxsize = 432;
>>      } elsif ($file =~ /^altmbr[^0-9a-z]/) {
>> -    $maxsize = $padsize = 439;
>> +    $maxsize = 439;
>>      } else {
>>      die "$0: no default size for filename: $file\n";
>>      }
>>  }
>>
>> -$padsize = $maxsize unless(defined($padsize));
>> -
>>  open(FILE, '+<', $file) or die;
>>  @st = stat(FILE);
>>  if (!defined($size = $st[7])) {
>> @@ -46,9 +55,9 @@ if (!defined($size = $st[7])) {
>>  if ($size > $maxsize) {
>>      print STDERR "$file: too big ($size > $maxsize)\n";
>>      exit 1;
>> -} elsif ($size < $padsize) {
>> +} elsif ($size < $maxsize) {
>>      seek(FILE, $size, 0);
>> -    print FILE "\0" x ($padsize-$size);
>> +    print FILE "\0" x ($maxsize-$size);
>>  }
>>
>>  exit 0;
>> diff --git a/mbr/checksize_test.sh b/mbr/checksize_test.sh
>> new file mode 100644
>> index 0000000..d315118
>> --- /dev/null
>> +++ b/mbr/checksize_test.sh
>> @@ -0,0 +1,22 @@
>> +#!/bin/sh
>> +
>> +mkdir -p output_test && cd output_test || exit 1
>> +rm -f a mbr.bin
>> +touch a
>> +# Upgrade size of a file should success
>> +perl ../checksize.pl a 330
>> +[ $(wc -c a | cut -f1 -d" ") -eq 330 ] || exit 2
>> +# Downgrade size of a file should fail
>> +perl ../checksize.pl a 300
>> +[ $? -eq 1 ] || exit 3
>> +rm a
>> +
>> +touch mbr.bin
>> +# Upgrade size of a file
>> +perl ../checksize.pl mbr.bin 330
>> +[ $(wc -c mbr.bin | cut -f1 -d" ") -eq 330 ] || exit 4
>> +# Let's upgrade it by itself
>> +perl ../checksize.pl mbr.bin
>> +[ $(wc -c mbr.bin | cut -f1 -d" ") -eq 440 ] || exit 5
>> +rm mbr.bin
>> +
>> --
>> 2.6.2
>
>> Just to make sure: the goal of the shell script is to check the perl
>> script?
>> If so, I wonder if it wouldn't be better in the test directory if it
>
>> is needed at all. I'll let the others comment on that.
>
> It was only that I modify this script, so I decide to make a test to be
> sure
> of not breaking something.
>
>> I agree with your simplification of checksize.pl, but have you
>> searched through the history to see if there wasn't a good reason to
>> separate maxsize and padsize?
>
>
> Looking in history, first of all, this script only check size.
> hpa make it padding to $padsize. And add in a following commit the defaults
> maxsize and padsize. maxsize and padsize could be different for altmbr.
> In an other following commit, both maxsize and padsize are merged for
> altmbr.

Yeah, I see, the 3 arguments form has only been used for less than one
week between these two commits:
commit 848c8f9a4133e4e0113120a2b311d2352af26228
Date:   Mon Mar 30 18:17:05 2009 -0700

commit f7b5a2254ab7c8aa87679c1b731d2ea285c22e80
Date:   Sat Apr 4 15:32:24 2009 -0700

And only for altmbr.

So, given this feature hasn't been used in more than 6.5 years with no
valid use foreseen from now, I think your refactoring is legit.

> Celelibi
You may want to sign with your own (nick)name. :D


Celelibi