[libvirt] [PATCH] virsh: schedinfo --set invalid=value would simply ignore the option
Daniel P. Berrange
berrange at redhat.com
Tue May 11 14:26:31 UTC 2010
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 at 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 :|
More information about the libvir-list
mailing list