[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH] [TESTSUITE] virtual TPM test
xen-devel-bounces@xxxxxxxxxxxxxxxxxxx wrote on 02/22/2006 10:40:33 AM: > On Tue, Feb 21, 2006 at 11:00:41AM -0500, Stefan Berger wrote: > > > The attached patch adds 2 tests for the virtual TPM to the test suite > > (more to come over time). It skips the test if it cannot be run. It > > builds on top of the previous patch. > > > > Signed-off-by: Stefan Berger <stefanb@xxxxxxxxxx> > > Stefan, I have a number of issues with this patch. Firstly, you have made > significant changes to the scripts in tools/examples, plus a change to > xm-test's core library, and yet your changelog message makes no mention of > them. > > > Index: xen/xen-unstable.hg/tools/examples/vtpm-common.sh > > =================================================================== > > @@ -82,16 +85,15 @@ function find_instance () { > > if [ "$instance" != "" ]; then > > ret=1 > > fi > > - return $ret > > + res=$ret > > } > > Why have you made this change? Setting a global variable to return a result > as a side effect is poor style. This occurs a number of times in your patch. To get rid of the 'set +e' command I cannot return a number in the return statement, otherwise the script just terminates. > > > Index: xen/xen-unstable.hg/tools/examples/vtpm-delete > > =================================================================== > > --- /dev/null > > +++ xen/xen-unstable.hg/tools/examples/vtpm-delete > > @@ -0,0 +1,18 @@ > > +#!/bin/sh > > + > > +# This scripts must be called the following way: > > +# vtpm-delete <domain name> > > + > > +if [ $1 != "remove" ]; then > > + exec sh -c "bash $0 remove $*" > > +fi > > + > > + > > +XENBUS_PATH=" " > > + > > +dir=$(dirname "$0") > > +. "$dir/vtpm-common.sh" > > + > > +shift > > + > > +vtpm_delete_instance $1 > > You are setting XENBUS_PATH just to hack your way into the hotplug scripts, > and then relying on the fact that vtpm_delete_instance doesn't actually use > that value. This is not a reasonable thing to be do. If there's a need for a > tpm-manipulating library, separate from the hotplug infrastructure, then you > should split vtpm-common.sh into vtpm-common.sh and vtpm-hotplug-common.sh (or > similar). Note that vif-common.sh includes xen-hotplug-common.sh and > xen-network-common.sh -- these are split apart for exactly that reason > (xen-network-common.sh being used for the network-xyz scripts as well as for > the vif-xyz hotplugging scripts). It's a hack, indeed. I will change that. -- Stefan > > Ewan. > > _______________________________________________ > Xen-devel mailing list > Xen-devel@xxxxxxxxxxxxxxxxxxx > http://lists.xensource.com/xen-devel _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxxxxxxxx http://lists.xensource.com/xen-devel
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |