WARNING - OLD ARCHIVES

This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
   
 
 
Xen 
 
Home Products Support Community News
 
   
 

xen-devel

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

To: Ewan Mellor <ewan@xxxxxxxxxxxxx>
Subject: Re: [Xen-devel] [PATCH] [TESTSUITE] virtual TPM test
From: Stefan Berger <stefanb@xxxxxxxxxx>
Date: Wed, 22 Feb 2006 11:48:18 -0500
Cc: xen-devel@xxxxxxxxxxxxxxxxxxx
Delivery-date: Wed, 22 Feb 2006 16:49:45 +0000
Envelope-to: www-data@xxxxxxxxxxxxxxxxxxx
In-reply-to: <20060222154033.GL28961@xxxxxxxxxxxxxxxxxxxxxx>
List-help: <mailto:xen-devel-request@lists.xensource.com?subject=help>
List-id: Xen developer discussion <xen-devel.lists.xensource.com>
List-post: <mailto:xen-devel@lists.xensource.com>
List-subscribe: <http://lists.xensource.com/cgi-bin/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=subscribe>
List-unsubscribe: <http://lists.xensource.com/cgi-bin/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=unsubscribe>
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx

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