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

[Ewan Mellor <ewan@xxxxxxxxxxxxx>]

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