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@xxxxxxxxxx> > 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 :| -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list