Re: [PATCH v2] xfsdump: enable dump header checksums

[Alex Elder <aelder@sgi.com>]

On Tue, 2011-09-20 at 08:05 -0500, Bill Kendall wrote:
> Various structures in a dump file optionally contain a checksum, but
> the code to compute and validate the checksum has not been enabled.
> The checksum code has a negligible performance impact and so this
> patch enables the checksum code unconditionally. Also:
> 
> - make sure all header sizes are multiples of 4 bytes
>   (a requirement of the checksum routine)
> - zero structures to ensure internal padding has a known value
> - fix a bug in dump_extattr_buildrecord() which checksummed
>   the wrong header structure
> - add calc_checksum() and is_checksum_valid() routines to
>   cut down on duplicate code
> 
> Signed-off-by: Bill Kendall <wkendall@sgi.com>

This looks good.  I have a few comments for you
below but unless you decide to send me an update
I'll use this as-is.

Reviewed-by: Alex Elder <aelder@sgi.com>

> ---
>  common/content_inode.h |   25 ++++++++++++++
>  dump/content.c         |   85 +++++++++++-------------------------------------
>  restore/Makefile       |    2 +-
>  restore/content.c      |   40 ++--------------------
>  4 files changed, 49 insertions(+), 103 deletions(-)
> 
> diff --git a/common/content_inode.h b/common/content_inode.h
> index 479fdfc..9c2c1cc 100644
> --- a/common/content_inode.h
> +++ b/common/content_inode.h
> @@ -347,4 +347,29 @@ typedef struct extattrhdr extattrhdr_t;
>  	/* a linux "secure" mode attribute
>  	 */
>  
> +/* Routines for calculating and validating checksums on xfsdump headers.
> + * The header length must be an integral number of u_int32_t's.
> + */
> +static inline u_int32_t
> +calc_checksum(void *bufp, size_t len)
> +{
> +	u_int32_t sum = 0;
> +	u_int32_t *sump = (u_int32_t *)bufp;
> +	u_int32_t *endp = (void *)sump + len

No need to cast a (void *) object to another
pointer type (and vice-versa).

And although gcc allows arithmetic on void pointers,
it is not standard, so (char *) would be a more
portable choice.

The multiple-of-4 assumption would be well stated with
an assertion.

> ;
> +	while (sump < endp)
> +		sum += *sump++;
> +	return ~sum + 1;
> +}
> +
> +static inline bool_t
> +is_checksum_valid(void *bufp, size_t len)
> +{

. . .



_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs