[parted-devel] [PATCH] Fix the script mode for mkpart and mkpartfs.

[Jim Meyering]

Joel Andres Granados <jgranado at redhat.com> wrote:
> In scripting mode, parted used to ask the user for confirmation
> when the values to be used where not the ones specified by the user.
> * parted/parted.c (do_mkpart, do_mkpartfs): if opt_script_mode is.
>   set fail, if it's not, warn and ask for intervention.
> * tests/Makefile.am : include the new test in the TEST list.
> * tests/t7000-scripting.sh : Distribute new test case.

Hi Joel,

Thanks for pursuing this.

>  parted/parted.c          |   37 +++++++++++++++++++++++++++----------
>  tests/Makefile.am        |    3 ++-
>  tests/t7000-scripting.sh |   45 +++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 74 insertions(+), 11 deletions(-)
>  create mode 100755 tests/t7000-scripting.sh
>
> diff --git a/parted/parted.c b/parted/parted.c
> index 9f79ea4..3b5c07a 100644
> --- a/parted/parted.c
> +++ b/parted/parted.c
> @@ -776,14 +776,25 @@ do_mkpart (PedDevice** dev)
>                          start_sol = ped_unit_format (*dev, part->geom.start);
>                          end_sol   = ped_unit_format (*dev, part->geom.end);
>
> +                        /* In script mode failure to use specified values is fatal.
> +                         * However, in interactive mode, it merely elicits a warning
> +                         * and a prompt for whether to proceed.  The same appies for
> +                         * do_mkpartfs function.
> +                         */
>                          switch (ped_exception_throw (
> -                                PED_EXCEPTION_WARNING,
> -                                PED_EXCEPTION_YES_NO,
> +                                (opt_script_mode
> +                                 ? PED_EXCEPTION_ERROR
> +                                 : PED_EXCEPTION_WARNING),
> +                                (opt_script_mode <<<

Your patch adds a trailing space on the line above.
Please don't do that.  There is another, below.

> +                                 ? PED_EXCEPTION_CANCEL
> +                                 : PED_EXCEPTION_YES_NO),
>                                  _("You requested a partition from %s to %s.\n"
>                                    "The closest location we can manage is "
> -                                  "%s to %s.  "
> -                                  "Is this still acceptable to you?"),
> -                                start_usr, end_usr, start_sol, end_sol))
> +                                  "%s to %s."),
> +                                start_usr, end_usr, start_sol, end_sol,
> +                                (opt_script_mode ? ""
> +                                 : _("  Is this still acceptable to you?"))))

If you compare the above to the patch I proposed, you'll see
a small but important difference in the format string.

> diff --git a/tests/Makefile.am b/tests/Makefile.am
> index ebebf49..fbcad7c 100644
> --- a/tests/Makefile.am
> +++ b/tests/Makefile.am
> @@ -12,7 +12,8 @@ TESTS = \
>    t4100-msdos-partition-limits.sh \
>    t4100-dvh-partition-limits.sh \
>    t5000-tags.sh \
> -  t6000-dm.sh
> +  t6000-dm.sh \
> +  t7000-scripting.sh
>
>  EXTRA_DIST = \
>    $(TESTS) test-lib.sh lvm-utils.sh
> diff --git a/tests/t7000-scripting.sh b/tests/t7000-scripting.sh
...
Thanks for adding the new test script!

> +# The failure message.
> +cat << EOF >> err || fail=1
> +Error: You requested a partition from 512B to 50.7kB.
> +The closest location we can manage is 17.4kB to 33.8kB.
> +EOF
> +
> +test_expect_success \
> +    'Create the test file' \
> +    'dd if=/dev/zero of=testfile bs=512 count=100 2> /dev/null'
> +
> +test_expect_failure \
> +    'Try running parted in scriptmode' \
> +    'parted -s testfile "mklabel gpt mkpart primary ext3 1s -1s" 1> out'

Please change "1>" to just ">".
The latter is more conventional.

> +test_expect_success \
> +    'Compare the real error and the expected one' \
> +    '$compare out err'
> +
> +test_expect_failure \
> +    'Test the interactive mode' \
> +    'echo n | parted ---pretend-input-tty testfile "mklabel gpt mkpart primary ext3 1s -1s"'

Please record the actual output here and compare it with what you
expect, you'll see that the "  Is this still acceptable to you?"
diagnostic is missing.  Also, limit line length to <80, i.e.,

test_expect_failure \
    'Test the interactive mode' \
    'echo n | parted ---pretend-input-tty testfile \
      "mklabel gpt mkpart primary ext3 1s -1s" > out'

The above tests "mkpart", which exercises the changes in the
first function.  However, this patch also affects mkpartfs, so
please run tests of that code, too.