Re: [Xen-devel] PATCH [base vtpm and libxl patches 2/6] Bug fixes and breakout of vtpm_manager functionality

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

I'm afraid this patch is also whitespace damaged and can't be applied.

If you use git send-email or the hg email extension then this should
avoid this sort of corruption.

Otherwise see
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=blob;f=Documentation/email-clients.txt;hb=HEAD
 for some hints on configuring your email client.

On Fri, 2012-09-21 at 19:57 +0100, Matthew Fioravante wrote:
> Fix numerous memory leaks, IO deadlocks, and other bugs that make
> vtpm_manager barely usable. Also breakout the vtpm_manager compilation
> into separate libraries to facilitate reusing parts of it in mini-os
> domains. Finally, add new programs for communicating with the manager
> correctly to avoid IO deadlock errors.
> 
> Signed off by Matthew Fioravante matthew.fioravante@xxxxxxxxxx
> 
> ---
> Changed since previous:
> * rebased off of latest xen-unstable
> 
> diff --git a/tools/vtpm_manager/Makefile b/tools/vtpm_manager/Makefile
> --- a/tools/vtpm_manager/Makefile
> +++ b/tools/vtpm_manager/Makefile

I think you could/should add yourself to the MAINTAINERS file for
tools/vtpm and vtpm_manager. If you are OK with that please send a patch
to do so.


> @@ -1,9 +1,9 @@
> -XEN_ROOT = $(CURDIR)/../..
> +XEN_ROOT = $(realpath ../..)

$(CURDIR)/../.. is idiom used almost everywhere in our build system.

What was the issue you were trying to address here?

Outside of the Makefiles and licenses I didn't really review this patch
in much detail, since I'm not really familiar with this code base. I
suspect no one other than you really is really qualified) so I think it
might as well just go on in (once the whitespace damage gets fixed). I'd
give it a little more time on list to see if anyone has any other
opinions.

One other barrier to review is that this is a single huge patch which
fixes multiple issues. In general we would prefer more fine grained
series where each patch fixes a single issue. 

> diff --git a/tools/vtpm_manager/tcs/tpmddl.c
> b/tools/vtpm_manager/tcs/tpmddl.c
> --- /dev/null
> +++ b/tools/vtpm_manager/tcs/tpmddl.c
> @@ -0,0 +1,167 @@
> +/*
> + * Copyright (c) 2010-2012 United States Government, as represented by
> + * the Secretary of Defense.  All rights reserved.

What license covers this code?

Other files in the same directory seem to use a 3 clause BSD.

Same question applies to all the other places with only this header.

> + *
> + * THIS SOFTWARE AND ITS DOCUMENTATION ARE PROVIDED AS IS AND WITHOUT
> + * ANY EXPRESS OR IMPLIED WARRANTIES WHATSOEVER. ALL WARRANTIES
> + * INCLUDING, BUT NOT LIMITED TO, PERFORMANCE, MERCHANTABILITY, FITNESS
> + * FOR A PARTICULAR  PURPOSE, AND NONINFRINGEMENT ARE HEREBY
> + * DISCLAIMED. USERS ASSUME THE ENTIRE RISK AND LIABILITY OF USING THE
> + * SOFTWARE.
> + */
[...]
> +diff --git a/tools/vtpm_manager/util/hashtable_itr.c
> b/tools/vtpm_manager/util/hashtable_itr.c
> --- a/tools/vtpm_manager/util/hashtable_itr.c
> +++ b/tools/vtpm_manager/util/hashtable_itr.c
> @@ -32,11 +32,6 @@
>   * SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
>  */
>  
> -/*
> - * There are duplicates of this code in:
> - *  - tools/blktap2/drivers/hashtable_itr.c
> - */

The purpose of this message is to remind people who fix one version that
there are other copies which need fixing too. And perhaps to eventually
motivate someone into reducing the duplication.

Please leave them in place.

> -
>  #include "hashtable.h"
>  #include "hashtable_private.h"
>  #include "hashtable_itr.h"
> diff --git a/tools/vtpm_manager/util/hashtable_itr.h
> b/tools/vtpm_manager/util/hashtable_itr.h
> --- a/tools/vtpm_manager/util/hashtable_itr.h
> +++ b/tools/vtpm_manager/util/hashtable_itr.h
> @@ -32,11 +32,6 @@
>   * SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
>  */
>  
> -/*
> - * There are duplicates of this code in:
> - *  - tools/blktap2/drivers/hashtable_itr.h
> - */

Please leave this....

> diff --git a/tools/vtpm_manager/util/hashtable_private.h
> b/tools/vtpm_manager/util/hashtable_private.h
> --- a/tools/vtpm_manager/util/hashtable_private.h
> +++ b/tools/vtpm_manager/util/hashtable_private.h
> @@ -32,12 +32,6 @@
>   * SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
>  */
>  
> -/*
> - * There are duplicates of this code in:
> - *  - tools/xenstore/hashtable_private.h
> - *  - tools/blktap2/drivers/hashtable_private.h
> - */

... and this (and any other similar things you've removed which I didn't
notice)


Ian.


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel