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@us.ibm.com> _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
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@us.ibm.com>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.> 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 $1You 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). Ewan. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
xen-devel-bounces@lists.xensource.com 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@us.ibm.com> > > Stefan, I have a number of issues with this patch. Firstly, you havemade> significant changes to the scripts in tools/examples, plus a change to > xm-test''s core library, and yet your changelog message makes no mentionof> 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 aresult> as a side effect is poor style. This occurs a number of times in yourpatch. 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 hotplugscripts,> and then relying on the fact that vtpm_delete_instance doesn''t actuallyuse> that value. This is not a reasonable thing to be do. If there''s a needfor a> tpm-manipulating library, separate from the hotplug infrastructure, thenyou> should split vtpm-common.sh into vtpm-common.sh andvtpm-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 asfor> the vif-xyz hotplugging scripts).It''s a hack, indeed. I will change that. -- Stefan> > Ewan. > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xensource.com > http://lists.xensource.com/xen-devel_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
On Wed, Feb 22, 2006 at 11:48:18AM -0500, Stefan Berger wrote:> > > 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.Got you. I prefer foo() { echo ''1'' } ... thing=$(foo "$param1") That way you avoid the global variable, but can still return useful things from the function. With this, you can even have more descriptive return values than integer codes, too. Cheers, Ewan. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
xen-devel-bounces@lists.xensource.com wrote on 02/22/2006 10:40:33 AM:> On Tue, Feb 21, 2006 at 11:00:41AM -0500, Stefan Berger wrote: >Ewan, if I cannot source the vtpm-hotplug-common.sh from the vtpm-delete anymore, I don''t have access to the locking functions (claim_lock, release_lock) in vtpm-hotplug-common.sh which are needed by my script. May I separte those out to locking.sh so they can be ''sourced'' by other scritps as well? Stefan> > 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 hotplugscripts,> and then relying on the fact that vtpm_delete_instance doesn''t actuallyuse> that value. This is not a reasonable thing to be do. If there''s a needfor a> tpm-manipulating library, separate from the hotplug infrastructure, thenyou> should split vtpm-common.sh into vtpm-common.sh andvtpm-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 asfor> the vif-xyz hotplugging scripts). > > Ewan. > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xensource.com > http://lists.xensource.com/xen-devel_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
On Wed, Feb 22, 2006 at 12:05:46PM -0500, Stefan Berger wrote:> xen-devel-bounces@lists.xensource.com wrote on 02/22/2006 10:40:33 AM: > > > On Tue, Feb 21, 2006 at 11:00:41AM -0500, Stefan Berger wrote: > > > > Ewan, if I cannot source the vtpm-hotplug-common.sh from the vtpm-delete > anymore, I don''t have access to the locking functions > (claim_lock, release_lock) in vtpm-hotplug-common.sh which are needed by > my script. May I separte those out to locking.sh so they can be ''sourced'' > by other scritps as well?Sure, that makes sense. Ewan. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel