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

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(a)redhat.com&gt; 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.

+ +# 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;