[parted-devel] [PATCH v2] test for the bootcode-in-extended-partition fix

Tue May 26 09:38:33 UTC 2009

Hey Petr.

My comments bellow.
On Mon, May 25, 2009 at 01:47:39PM +0200, Petr Uzel wrote:
> * tests/t2300-dos-label-extended-bootcode.sh: New file.
> * tests/Makefile.am (TESTS): Add t2300-dos-label-extended-bootcode.sh.
> 
> Signed-off-by: Petr Uzel <petr.uzel at suse.cz>
> ---
>  tests/Makefile.am                          |    1 +
>  tests/t2300-dos-label-extended-bootcode.sh |   70 ++++++++++++++++++++++++++++
>  2 files changed, 71 insertions(+), 0 deletions(-)
>  create mode 100755 tests/t2300-dos-label-extended-bootcode.sh
> 
> diff --git a/tests/Makefile.am b/tests/Makefile.am
> index 0beedd4..f2de6a6 100644
> --- a/tests/Makefile.am
> +++ b/tests/Makefile.am
> @@ -10,6 +10,7 @@ TESTS = \
>    t2000-mkfs.sh \
>    t2100-mkswap.sh \
>    t2200-dos-label-recog.sh \
> +  t2300-dos-label-extended-bootcode.sh \
>    t3000-constraints.sh \
>    t3100-resize-ext2-partion.sh \
>    t4000-sun-raid-type.sh \
> diff --git a/tests/t2300-dos-label-extended-bootcode.sh b/tests/t2300-dos-label-extended-bootcode.sh
> new file mode 100755
> index 0000000..5ef70c9
> --- /dev/null
> +++ b/tests/t2300-dos-label-extended-bootcode.sh
> @@ -0,0 +1,70 @@
> +#!/bin/sh
> +
> +# Copyright (C) 2009 Free Software Foundation, Inc.
> +
> +# 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 3 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, see <http://www.gnu.org/licenses/>.
> +
> +test_description='Make sure that parted preserves bootcode in extended partition.'

Having the src code be less than 80 chars in length makes it readable in
most terminals.  And it looks pretty :).  To this objective I would
rewrite this to say :
"
test_description='Ensure parted preserves bootcode in extended partition.'
"

> +
> +: ${srcdir=.}
> +. $srcdir/test-lib.sh
> +
> +
> +dev=loop-file
> +bootcode_size=440
> +bootcode_before=before
> +bootcode_after=after

I don't see the reason for these variables.  With the "before" and
"after" variables you are making it longer to write before and after.
The only reason, to do what you have done (than I can see), is to relate
them (before and after) to bootcode.  While this is not a bad think, it
is overkill IMO as the relation is very clear from the code.  If you
must have this relation explicit, I would use a comment instead of the
variables.

For the bootcode_size variable the same applies.  You are needing 13
chars (bootcode_size) to express 3 (440).  I do understand that not
everyone knows that the first 440 bytes of a disk is the bootcode, but I
would handle this with a comment rather than a variable.

If you don't use these variables, most of the code bellow fits the 80
char restriction, which is good :)

> +
> +
> +test_expect_success \
> +	'Create the test file' \
> +	'dd if=/dev/zero of=$dev bs=1024c count=100 2>/dev/null'

We need to redirect the stderr and stdout.  I would rewrite this line as
follows:
"
dd if=/dev/zero of=$dev bs=1024c count=100 > /dev/null 2>&1'
"

I would make sure we are redirecting stderr and stdout like that, be it to
/dev/null or to a file.  This is something that is not done consistently
in the test, but should be.
Which reminds me.  I have to correct my tests for this too.

> +
> +test_expect_success \
> +	'Create msdos label' \
> +	'parted -s $dev mklabel msdos > out 2>&1'
> +test_expect_success 'Expect no output' 'compare out /dev/null'
> +
> +test_expect_success \
> +	'Create extended partition' \
> +	'parted -s $dev mkpart extended 32s 127s > out 2>&1'
> +test_expect_success 'Expect no output' 'compare out /dev/null'
> +
> +test_expect_success \
> +	'Create logical partition' \
> +	'parted -s $dev mkpart logical 64s 127s > out 2>&1'
> +test_expect_success 'Expect no output' 'compare out /dev/null'
> +
> +test_expect_success \
> +	'Install fake bootcode' \
> +	'dd if=/dev/urandom of=$dev bs=1c seek=16384 count=$bootcode_size conv=notrunc 2> /dev/null'
This is the only line that does not go under 80 chars after changeing
the variables.
"
  'dd if=/dev/urandom of=$dev bs=1c seek=16384 count=440 conv=notrunc > /dev/null 2>&1'
"
You can solve this by using the "\" character.  So the line would end up
looking like this:
"
  'dd if=/dev/urandom of=$dev bs=1c seek=16384 count=440 \
    conv=notrunc > /dev/null 2>&1'
"

> +
> +test_expect_success \
> +	'Save fake bootcode for later comparison' \
> +	'dd if=$dev of=$bootcode_before bs=1 skip=16384 count=$bootcode_size 2>/dev/null'

Here the redirection change.

> +
> +test_expect_success \
> +	'Do something to the label' \
> +	'parted -s $dev rm 5 > out 2>&1'
> +test_expect_success 'Expect no output' 'compare out /dev/null'
> +
> +test_expect_success \
> +	'Extract the bootcode for comparison' \
> +	'dd if=$dev of=$bootcode_after bs=1 skip=16384 count=$bootcode_size 2>/dev/null'

Here the redirection change.

> +
> +test_expect_success \
> +	'Expect bootcode has not changed' \
> +	'compare $bootcode_before $bootcode_after'

Finally, I would not use tabs (in the tests at least).  This is
something that is not consistent in the tests but should be that way
because of the GNU coding standards
(http://www.gnu.org/prep/standards/standards.html#Formatting).  Im
specifically refering to the example in the body of the function part.
Consistency in the coding convention is something that parted needs to
get better at.!!!!!

> +
> +test_done
> -- 
> 1.6.3
> 
> 
> -- 
> Best regards / s pozdravem
> 
> Petr Uzel, Packages maintainer
> ---------------------------------------------------------------------
> SUSE LINUX, s.r.o.                          e-mail: puzel at suse.cz
> Lihovarská 1060/12                          tel: +420 284 028 964
> 190 00 Prague 9                             fax: +420 284 028 951
> Czech Republic                              http://www.suse.cz
> 
> _______________________________________________
> parted-devel mailing list
> parted-devel at lists.alioth.debian.org
> http://lists.alioth.debian.org/mailman/listinfo/parted-devel

-- 
Joel Andres Granados
Brno, Czech Republic, Red Hat.