Re: [Xen-devel] [PATCH] [TESTSUITE] virtual TPM test

[Stefan Berger <stefanb@xxxxxxxxxx>]

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