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: Stefan Berger <stefanb@xxxxxxxxxx>
Subject: Re: [Xen-devel] [PATCH] [TESTSUITE] virtual TPM test
From: Ewan Mellor <ewan@xxxxxxxxxxxxx>
Date: Wed, 22 Feb 2006 15:40:33 +0000
Cc: xen-devel@xxxxxxxxxxxxxxxxxxx
Delivery-date: Wed, 22 Feb 2006 16:29:06 +0000
Envelope-to: www-data@xxxxxxxxxxxxxxxxxxx
In-reply-to: <1140537641.5312.11.camel@xxxxxxxxxxxxxxxxxxxxx>
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>
References: <1140537641.5312.11.camel@xxxxxxxxxxxxxxxxxxxxx>
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
User-agent: Mutt/1.5.9i
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.

> 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).

Ewan.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel