- 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
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.2Just 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. 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? Celelibi
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. Celelibi
> 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 > >> 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. > > 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? > > > CelelibiFor what my personal opinion might be worth... I don't like the idea of changing "UI" (e.g. command line options that might be used by some users) without very well-thought reasoning. In the particular case of "padsize" vs. "maxsize" (options / parameters of the Perl script), there used to be a separate use of them. Although there _seems_ to be no use for having both of them simultaneously at this time _for a normal build of Syslinux_, we cannot discard / disregard the possibility that some script in some distro might want to take advantage of each of them independently, especially for some debugging situation (e.g. some buggy / old BIOS). Another potential (hypothetical) use-case might be some "bootable USB tool" making use of some/many/all parts of the Syslinux code. Let's not forget that some distros are still using older versions of Syslinux (even v. 3.xx), and (unnecessarily) changing command line options makes the updates (unnecessarily) more difficult, generally speaking. I am not against changing code, but, as common user, I differentiate between "internal-only" code and code that "interacts" with users. I don't know how much this particular suggested change is necessary / desired, and of course I don't know how much it would potentially affect third-party tools / users; these comments are more general than this particular email thread. Example: the "simple" change (since Syslinux v.6.xx) of the location of binary files (such as "mbr.bin") by moving them to the "bios/" sub-directory tree has had a clear impact on third-party tools and distros; most common users (and more-than-one developer / maintainer of third-party tools / packages) have a negative view / opinion regarding this change of directory. BTW, let's not forget the existence of the "diag/*" files in Syslinux. Regards, Ady. PS: Let's also not forget the existence of other methods / code to generate / install MBRs and/or VBRs such as: _ "sys.com" / "sys.exe" (v.3.8) from FreeDOS; _ "sys-freedos-linux"; _ "ms-sys" ( _code in C_ ) and its forks such as: _ "ms-sys-free" (current latest code is older than its parent); _ a fork of "ms-sys" by Pete Batard (used in RUFUS) with additional recent patches. Common users and third-party scripts might rather use the Perl script for simplicity, so having scripts available is helpful. Having code written in C for the generation / installation of MBRs / VBRs might be (at least in part, or as a first step) an alternative to the generation of the current syslinux / extlinux installers, and/or eventually an alternative installation method.