[Top] [All Lists]

Re: [PATCH 3/3 v2] xfstests: Add support for sections in config file

To: Lukas Czerner <lczerner@xxxxxxxxxx>
Subject: Re: [PATCH 3/3 v2] xfstests: Add support for sections in config file
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Mon, 1 Jul 2013 11:49:44 +1000
Cc: xfs@xxxxxxxxxxx
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <1372426320-19902-3-git-send-email-lczerner@xxxxxxxxxx>
References: <1372426320-19902-1-git-send-email-lczerner@xxxxxxxxxx> <1372426320-19902-3-git-send-email-lczerner@xxxxxxxxxx>
User-agent: Mutt/1.5.21 (2010-09-15)
On Fri, Jun 28, 2013 at 03:32:00PM +0200, Lukas Czerner wrote:
> This patch add support for sections in the config file. Each section can
> contain configuration options in the format
> OPTION=value
> when one section is processed xfstests will proceed to next section
> until all secitons are processed, or an error occur.
> The name of the section can consist of alphanumeric characters + '_',
> nothing else is allowed. Name of the section is also used to create
> results subdirectory for each section. After all the sections are
> processed summary of all runs is printed out.
> If the config file does not contain sections, or we're not using config
> file at all, nothing is changed and xfstests will work the same way as
> it used to.
> This is very useful for testing file system with different options. Here
> is an example of the config file with sections:
> [ext4_4k_block_size]
> TEST_DEV=/dev/sda
> TEST_DIR=/mnt/test
> SCRATCH_DEV=/dev/sdb
> SCRATCH_MNT=/mnt/test1
> MKFS_OPTIONS="-q -F -b4096"
> FSTYP=ext4
> [ext4_1k_block_size]
> MKFS_OPTIONS="-q -F -b1024"
> [ext4_nojournal]
> MKFS_OPTIONS="-q -F -b4096 -O ^has_journal"
> [ext4_discard_ssd]
> MKFS_OPTIONS="-q -F -b4096"
> TEST_DEV=/dev/sdc
> SCRATCH_DEV=/dev/sdd
> MOUNT_OPTIONS="-o discard"
> Note that once the variable is set it remains set across the sections, so
> you do not have to specify all the options in all sections. However one
> have to make sure that unwanted options are not set from previous
> sections.

I like the idea, but a lot of this needs to be in the (missing)
patch 0 description for the series. YOu also need to describe how
these get invoked, what the output looks like, etc because right now
I can't work it out from this...

> Signed-off-by: Lukas Czerner <lczerner@xxxxxxxxxx>
> ---
>  check         | 371 
> +++++++++++++++++++++++++++++++++-------------------------
>  common/config | 126 ++++++++++++--------
>  common/rc     |  63 +++++-----
>  3 files changed, 326 insertions(+), 234 deletions(-)

This patch probably needs to be broken up, too. A substantial part
of it is indentation changes, which probably should be split into 2
parts - factor code into function, then wrap loop around function.
The change to the summary information should be done as a separate
patch, too. I suspect many of the common/config changes coul dbe
split up, too.

The changing of the $RESULT_BASE should probably also
bein a separate patch, because this is something that we'll need to
discuss as it changes the structure of the output....

Oh, and why make a distinction between no sections and
$OPTIONS_HAVE_SECTIONS in the config file? Surely no sections is
just the same as having 1 section....

> +parse_config_section() {
> +     SECTION=$1
> +     if ! $OPTIONS_HAVE_SECTIONS; then
> +             return 0
> +     fi
> +     eval `sed -e 's/[[:space:]]*\=[[:space:]]*/=/g' \
> +             -e 's/#.*$//' \
> +             -e 's/[[:space:]]*$//' \
> +             -e 's/^[[:space:]]*//' \
> +             -e "s/^\(.*\)=\([^\"']*\)$/export \1=\"\2\"/" \
> +             < $HOST_OPTIONS \
> +             | sed -n -e "/^\[$SECTION\]/,/^\s*\[/{/^[^;].*\=.*/p;}"`
> +}

And line noise like this needs comments explaining the format and
what it is doing. ;)


Dave Chinner

<Prev in Thread] Current Thread [Next in Thread>