[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH] virsh: schedinfo --set invalid=value would simply ignore the option



On Tue, May 11, 2010 at 04:00:46PM +0200, Jim Meyering wrote:
> Currently, virsh does this:
> 
>     $ virsh -c test:///default schedinfo 1 --set j=k>/dev/null && echo bad
>     bad
> 
> It should have diagnosed "j=k" as an invalid schedinfo --set argument.
> With the patch below, it does:
> 
>     $ ./virsh -c test:///default schedinfo 1 --set j=k>/dev/null && echo bad
>     error: invalid scheduler option: j=k
>     [Exit 1]
> 
> 
> 
> >From cf56648333c4e5cf73ab3292ba727e33b0ae9708 Mon Sep 17 00:00:00 2001
> From: Jim Meyering <meyering redhat com>
> Date: Tue, 11 May 2010 15:38:21 +0200
> Subject: [PATCH] virsh: schedinfo --set invalid=value would simply ignore the option
> 
> For example, virsh -c test:///default schedinfo 1 --set P=k would
> mistakenly exit successfully, giving no indication that it had failed
> to set the scheduling parameter "P".
> * tools/virsh.c (cmdSchedinfo): Diagnose an invalid --set j=k option,
> rather than silently ignoring it.
> * tests/virsh-schedinfo: New test for the above.
> * tests/Makefile.am (test_scripts): Add it.
> Reported by Jintao Yang in http://bugzilla.redhat.com/586632
> ---
>  tests/Makefile.am     |    1 +
>  tests/virsh-schedinfo |   49 +++++++++++++++++++++++++++++++++++++++++++++++++
>  tools/virsh.c         |   10 ++++++++++
>  3 files changed, 60 insertions(+), 0 deletions(-)
>  create mode 100755 tests/virsh-schedinfo
> 
> diff --git a/tests/Makefile.am b/tests/Makefile.am
> index ef12386..b5e09e3 100644
> --- a/tests/Makefile.am
> +++ b/tests/Makefile.am
> @@ -139,6 +139,7 @@ test_scripts +=				\
>  	undefine			\
>  	vcpupin				\
>  	virsh-all			\
> +	virsh-schedinfo			\
>  	virsh-synopsis
>  endif
> 
> diff --git a/tests/virsh-schedinfo b/tests/virsh-schedinfo
> new file mode 100755
> index 0000000..116014a
> --- /dev/null
> +++ b/tests/virsh-schedinfo
> @@ -0,0 +1,49 @@
> +#!/bin/sh
> +# Ensure that virsh schedinfo --set invalid=val fails
> +
> +# Copyright (C) 2010 Free Software Foundation, Inc.

I think you mean Red Hat :-)

> +
> +# 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/>.
> +
> +: ${srcdir=$(pwd)}
> +: ${abs_top_srcdir=$(pwd)/..}
> +: ${abs_top_builddir=$(pwd)/..}
> +
> +# If $abs_top_builddir/tools/virsh is not early in $PATH, put it there,
> +# so that we can safely invoke "virsh" simply with its name.
> +case $PATH in
> +  $abs_top_builddir/tools/src:$abs_top_builddir/tools/virsh:*) ;;
> +  $abs_top_builddir/tools/virsh:*) ;;
> +  *) PATH=$abs_top_builddir/tools/virsh:$PATH; export PATH ;;
> +esac
> +
> +if test "$VERBOSE" = yes; then
> +  set -x
> +  $abs_top_builddir/tools/virsh --version
> +fi
> +
> +. "$srcdir/test-lib.sh"
> +
> +printf 'Scheduler      : fair\n\n' > exp-out || framework_failure
> +printf 'error: invalid scheduler option: j=k\n' > exp-err || framework_failure
> +
> +fail=0
> +
> +test_url=test:///default
> +
> +virsh -c $test_url schedinfo 1 --set j=k >out 2>err && fail=1
> +compare out exp-out || fail=1
> +compare err exp-err || fail=1
> +
> +(exit $fail); exit $fail
> diff --git a/tools/virsh.c b/tools/virsh.c
> index 0a63f1b..99f383a 100644
> --- a/tools/virsh.c
> +++ b/tools/virsh.c
> @@ -1594,6 +1594,16 @@ cmdSchedinfo(vshControl *ctl, const vshCmd *cmd)
>              ret = virDomainGetSchedulerParameters(dom, params, &nparams);
>              if (ret == -1)
>                  goto cleanup;
> +        } else {
> +            /* See if we've tried to --set var=val.  If so, the fact that
> +               we reach this point (with update == 0) means that "var" did
> +               not match any of the settable parameters.  Report the error.  */
> +            char *var_value_pair = vshCommandOptString(cmd, "set", NULL);
> +            if (var_value_pair) {
> +                vshError(ctl, _("invalid scheduler option: %s"),
> +                         var_value_pair);
> +                goto cleanup;
> +            }
>          }
> 
>          ret_val = TRUE;

ACK, looks good to me - surprisingly easy to catch

Daniel
-- 
|: Red Hat, Engineering, London    -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :|
|: http://autobuild.org        -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]