* [PATCH] xfsdump: enable dump header checksums
@ 2011-08-29 21:41 Bill Kendall
2011-09-02 14:51 ` Gim Leong Chin
2011-09-19 20:12 ` Alex Elder
0 siblings, 2 replies; 7+ messages in thread
From: Bill Kendall @ 2011-08-29 21:41 UTC (permalink / raw)
To: xfs
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>
---
common/content_inode.h | 20 +++++++++++
dump/content.c | 85 +++++++++++-------------------------------------
restore/Makefile | 2 +-
restore/content.c | 40 ++--------------------
4 files changed, 44 insertions(+), 103 deletions(-)
diff --git a/common/content_inode.h b/common/content_inode.h
index 479fdfc..e119354 100644
--- a/common/content_inode.h
+++ b/common/content_inode.h
@@ -347,4 +347,24 @@ typedef struct extattrhdr extattrhdr_t;
/* a linux "secure" mode attribute
*/
+/* Routines for calculating and validating checksums on xfsdump headers
+ */
+static inline u_int32_t
+calc_checksum(void *startp, void *endp)
+{
+ u_int32_t sum;
+ u_int32_t *sump = (u_int32_t *)startp;
+ for (sum = 0; sump < (u_int32_t *)endp; sum += *sump++);
+ return ~sum + 1;
+}
+
+static inline bool_t
+is_checksum_valid(void *startp, void *endp)
+{
+ u_int32_t sum;
+ u_int32_t *sump = (u_int32_t *)startp;
+ for (sum = 0; sump < (u_int32_t *)endp; sum += *sump++);
+ return sum == 0 ? BOOL_TRUE : BOOL_FALSE;
+}
+
#endif /* CONTENT_INODE_H */
diff --git a/dump/content.c b/dump/content.c
index 9905c88..2cf15ba 100644
--- a/dump/content.c
+++ b/dump/content.c
@@ -585,6 +585,13 @@ content_init( intgen_t argc,
sizeof( content_inode_hdr_t ));
ASSERT( sizeof( extattrhdr_t ) == EXTATTRHDR_SZ );
+ /* must be a multiple of 32-bits for checksums
+ */
+ ASSERT( FILEHDR_SZ % sizeof( u_int32_t ) == 0 );
+ ASSERT( EXTENTHDR_SZ % sizeof( u_int32_t ) == 0 );
+ ASSERT( DIRENTHDR_SZ % sizeof( u_int32_t ) == 0 );
+ ASSERT( EXTATTRHDR_SZ % sizeof( u_int32_t ) == 0 );
+
/* calculate offsets of portions of the write hdr template
*/
dwhdrtemplatep = ( drive_hdr_t * )gwhdrtemplatep->gh_upper;
@@ -1491,8 +1498,7 @@ baseuuidbypass:
var_skip( &fsid, inomap_skip );
/* fill in write header template content info. always produce
- * an inomap and dir dump for each media file. flag the checksums
- * available if so compiled (see -D...CHECKSUM in Makefile).
+ * an inomap and dir dump for each media file.
*/
ASSERT( sizeof( cwhdrtemplatep->ch_specific ) >= sizeof( *scwhdrtemplatep ));
scwhdrtemplatep->cih_mediafiletype = CIH_MEDIAFILETYPE_DATA;
@@ -1506,15 +1512,9 @@ baseuuidbypass:
if ( sc_inv_updatepr ) {
scwhdrtemplatep->cih_dumpattr |= CIH_DUMPATTR_INVENTORY;
}
-#ifdef FILEHDR_CHECKSUM
scwhdrtemplatep->cih_dumpattr |= CIH_DUMPATTR_FILEHDR_CHECKSUM;
-#endif /* FILEHDR_CHECKSUM */
-#ifdef EXTENTHDR_CHECKSUM
scwhdrtemplatep->cih_dumpattr |= CIH_DUMPATTR_EXTENTHDR_CHECKSUM;
-#endif /* EXTENTHDR_CHECKSUM */
-#ifdef DIRENTHDR_CHECKSUM
scwhdrtemplatep->cih_dumpattr |= CIH_DUMPATTR_DIRENTHDR_CHECKSUM;
-#endif /* DIRENTHDR_CHECKSUM */
scwhdrtemplatep->cih_dumpattr |= CIH_DUMPATTR_DIRENTHDR_GEN;
if ( sc_incrpr ) {
scwhdrtemplatep->cih_dumpattr |= CIH_DUMPATTR_INCREMENTAL;
@@ -1528,10 +1528,8 @@ baseuuidbypass:
}
if ( sc_dumpextattrpr ) {
scwhdrtemplatep->cih_dumpattr |= CIH_DUMPATTR_EXTATTR;
-#ifdef EXTATTRHDR_CHECKSUM
scwhdrtemplatep->cih_dumpattr |=
CIH_DUMPATTR_EXTATTRHDR_CHECKSUM;
-#endif /* EXTATTRHDR_CHECKSUM */
}
scwhdrtemplatep->cih_rootino = sc_rootxfsstatp->bs_ino;
@@ -3743,6 +3741,8 @@ dump_extattr_buildrecord( xfs_bstat_t *statp,
namesz, namesrcp,
valuesz );
( void )strcpy( namep, namesrcp );
+
+ memset( ( void * )&tmpah, 0, sizeof( tmpah ));
tmpah.ah_sz = recsz;
ASSERT( EXTATTRHDR_SZ + namesz < UINT16MAX );
tmpah.ah_valoff = ( u_int16_t )( EXTATTRHDR_SZ + namesz );
@@ -3750,17 +3750,8 @@ dump_extattr_buildrecord( xfs_bstat_t *statp,
(( flag & ATTR_ROOT ) ? EXTATTRHDR_FLAGS_ROOT :
(( flag & ATTR_SECURE ) ? EXTATTRHDR_FLAGS_SECURE : 0));
tmpah.ah_valsz = valuesz;
- tmpah.ah_checksum = 0;
-#ifdef EXTATTRHDR_CHECKSUM
- {
- register u_int32_t *sump = ( u_int32_t * )ahdrp;
- register u_int32_t *endp = ( u_int32_t * )( ahdrp + 1 );
- register u_int32_t sum;
tmpah.ah_flags |= EXTATTRHDR_FLAGS_CHECKSUM;
- for ( sum = 0 ; sump < endp ; sum += *sump++ ) ;
- tmpah.ah_checksum = ~sum + 1;
- }
-#endif /* EXTATTRHDR_CHECKSUM */
+ tmpah.ah_checksum = calc_checksum( &tmpah, &tmpah + 1 );
xlate_extattrhdr(ahdrp, &tmpah, -1);
*valuepp = valuep;
@@ -3782,23 +3773,13 @@ dump_extattrhdr( drive_t *drivep,
intgen_t rval;
rv_t rv;
+ memset( ( void * )&ahdr, 0, sizeof( ahdr ));
ahdr.ah_sz = recsz;
ASSERT( valoff < UINT16MAX );
ahdr.ah_valoff = ( u_int16_t )valoff;
- ahdr.ah_flags = ( u_int16_t )flags;
+ ahdr.ah_flags = ( u_int16_t )flags | EXTATTRHDR_FLAGS_CHECKSUM;
ahdr.ah_valsz = valsz;
- ahdr.ah_checksum = 0;
-
-#ifdef EXTATTRHDR_CHECKSUM
- {
- register u_int32_t *sump = ( u_int32_t * )&ahdr;
- register u_int32_t *endp = ( u_int32_t * )( &ahdr + 1 );
- register u_int32_t sum;
- ahdr.ah_flags |= EXTATTRHDR_FLAGS_CHECKSUM;
- for ( sum = 0 ; sump < endp ; sum += *sump++ ) ;
- ahdr.ah_checksum = ~sum + 1;
- }
-#endif /* EXTATTRHDR_CHECKSUM */
+ ahdr.ah_checksum = calc_checksum( &ahdr, &ahdr + 1 );
xlate_extattrhdr(&ahdr, &tmpahdr, 1);
rval = write_buf( ( char * )&tmpahdr,
@@ -5102,11 +5083,6 @@ dump_filehdr( drive_t *drivep,
drive_ops_t *dop = drivep->d_opsp;
register filehdr_t *fhdrp = contextp->cc_filehdrp;
filehdr_t tmpfhdrp;
-#ifdef FILEHDR_CHECKSUM
- register u_int32_t *sump = ( u_int32_t * )fhdrp;
- register u_int32_t *endp = ( u_int32_t * )( fhdrp + 1 );
- register u_int32_t sum;
-#endif /* FILEHDR_CHECKSUM */
intgen_t rval;
rv_t rv;
@@ -5118,13 +5094,8 @@ dump_filehdr( drive_t *drivep,
copy_xfs_bstat(&fhdrp->fh_stat, statp);
}
fhdrp->fh_offset = offset;
- fhdrp->fh_flags = flags;
-
-#ifdef FILEHDR_CHECKSUM
- fhdrp->fh_flags |= FILEHDR_FLAGS_CHECKSUM;
- for ( sum = 0 ; sump < endp ; sum += *sump++ ) ;
- fhdrp->fh_checksum = ~sum + 1;
-#endif /* FILEHDR_CHECKSUM */
+ fhdrp->fh_flags = flags | FILEHDR_FLAGS_CHECKSUM;
+ fhdrp->fh_checksum = calc_checksum( fhdrp, fhdrp + 1 );
xlate_filehdr(fhdrp, &tmpfhdrp, 1);
rval = write_buf( ( char * )&tmpfhdrp,
@@ -5164,11 +5135,6 @@ dump_extenthdr( drive_t *drivep,
drive_ops_t *dop = drivep->d_opsp;
register extenthdr_t *ehdrp = contextp->cc_extenthdrp;
extenthdr_t tmpehdrp;
-#ifdef EXTENTHDR_CHECKSUM
- register u_int32_t *sump = ( u_int32_t * )ehdrp;
- register u_int32_t *endp = ( u_int32_t * )( ehdrp + 1 );
- register u_int32_t sum;
-#endif /* EXTENTHDR_CHECKSUM */
intgen_t rval;
rv_t rv;
char typestr[20];
@@ -5198,15 +5164,10 @@ dump_extenthdr( drive_t *drivep,
( void )memset( ( void * )ehdrp, 0, sizeof( *ehdrp ));
ehdrp->eh_type = type;
- ehdrp->eh_flags = flags;
+ ehdrp->eh_flags = flags | EXTENTHDR_FLAGS_CHECKSUM;
ehdrp->eh_offset = offset;
ehdrp->eh_sz = sz;
-
-#ifdef EXTENTHDR_CHECKSUM
- ehdrp->eh_flags |= EXTENTHDR_FLAGS_CHECKSUM;
- for ( sum = 0 ; sump < endp ; sum += *sump++ ) ;
- ehdrp->eh_checksum = ~sum + 1;
-#endif /* EXTENTHDR_CHECKSUM */
+ ehdrp->eh_checksum = calc_checksum( ehdrp, ehdrp + 1 );
xlate_extenthdr(ehdrp, &tmpehdrp, 1);
rval = write_buf( ( char * )&tmpehdrp,
@@ -5249,11 +5210,6 @@ dump_dirent( drive_t *drivep,
direnthdr_t *tmpdhdrp;
size_t direntbufsz = contextp->cc_mdirentbufsz;
size_t sz;
-#ifdef DIRENTHDR_CHECKSUM
- register u_int32_t *sump = ( u_int32_t * )dhdrp;
- register u_int32_t *endp = ( u_int32_t * )( dhdrp + 1 );
- register u_int32_t sum;
-#endif /* DIRENTHDR_CHECKSUM */
intgen_t rval;
rv_t rv;
@@ -5290,10 +5246,7 @@ dump_dirent( drive_t *drivep,
strcpy( dhdrp->dh_name, name );
}
-#ifdef DIRENTHDR_CHECKSUM
- for ( sum = 0 ; sump < endp ; sum += *sump++ ) ;
- dhdrp->dh_checksum = ~sum + 1;
-#endif /* DIRENTHDR_CHECKSUM */
+ dhdrp->dh_checksum = calc_checksum( dhdrp, dhdrp + 1);
tmpdhdrp = malloc(sz);
xlate_direnthdr(dhdrp, tmpdhdrp, 1);
diff --git a/restore/Makefile b/restore/Makefile
index 78ecc2c..588a8f0 100644
--- a/restore/Makefile
+++ b/restore/Makefile
@@ -103,7 +103,7 @@ LLDLIBS = $(LIBUUID) $(LIBHANDLE) $(LIBATTR) $(LIBRMT)
LTDEPENDENCIES = $(LIBRMT)
LCFLAGS = -DRESTORE -DRMT -DBASED -DDOSOCKS -DINVCONVFIX -DPIPEINVFIX \
- -DEOMFIX -DSESSCPLT -DWHITEPARSE -DDIRENTHDR_CHECKSUM \
+ -DEOMFIX -DSESSCPLT -DWHITEPARSE \
-DF_FSSETDM
default: depend $(LTCOMMAND)
diff --git a/restore/content.c b/restore/content.c
index e3e4994..25849d7 100644
--- a/restore/content.c
+++ b/restore/content.c
@@ -8000,11 +8000,6 @@ read_filehdr( drive_t *drivep, filehdr_t *fhdrp, bool_t fhcs )
drive_ops_t *dop = drivep->d_opsp;
/* REFERENCED */
intgen_t nread;
-#ifdef FILEHDR_CHECKSUM
- register u_int32_t *sump = ( u_int32_t * )fhdrp;
- register u_int32_t *endp = ( u_int32_t * )( fhdrp + 1 );
- register u_int32_t sum;
-#endif /* FILEHDR_CHECKSUM */
intgen_t rval;
filehdr_t tmpfh;
@@ -8041,21 +8036,18 @@ read_filehdr( drive_t *drivep, filehdr_t *fhdrp, bool_t fhcs )
bstatp->bs_ino,
bstatp->bs_mode );
-#ifdef FILEHDR_CHECKSUM
if ( fhcs ) {
if ( ! ( fhdrp->fh_flags & FILEHDR_FLAGS_CHECKSUM )) {
mlog( MLOG_NORMAL | MLOG_WARNING, _(
"corrupt file header\n") );
return RV_CORRUPT;
}
- for ( sum = 0 ; sump < endp ; sum += *sump++ ) ;
- if ( sum ) {
+ if ( !is_checksum_valid( fhdrp, fhdrp + 1)) {
mlog( MLOG_NORMAL | MLOG_WARNING, _(
"bad file header checksum\n") );
return RV_CORRUPT;
}
}
-#endif /* FILEHDR_CHECKSUM */
return RV_OK;
}
@@ -8067,11 +8059,6 @@ read_extenthdr( drive_t *drivep, extenthdr_t *ehdrp, bool_t ehcs )
drive_ops_t *dop = drivep->d_opsp;
/* REFERENCED */
intgen_t nread;
-#ifdef EXTENTHDR_CHECKSUM
- register u_int32_t *sump = ( u_int32_t * )ehdrp;
- register u_int32_t *endp = ( u_int32_t * )( ehdrp + 1 );
- register u_int32_t sum;
-#endif /* EXTENTHDR_CHECKSUM */
intgen_t rval;
extenthdr_t tmpeh;
@@ -8108,21 +8095,18 @@ read_extenthdr( drive_t *drivep, extenthdr_t *ehdrp, bool_t ehcs )
ehdrp->eh_type,
ehdrp->eh_flags );
-#ifdef EXTENTHDR_CHECKSUM
if ( ehcs ) {
if ( ! ( ehdrp->eh_flags & EXTENTHDR_FLAGS_CHECKSUM )) {
mlog( MLOG_NORMAL | MLOG_WARNING, _(
"corrupt extent header\n") );
return RV_CORRUPT;
}
- for ( sum = 0 ; sump < endp ; sum += *sump++ ) ;
- if ( sum ) {
+ if ( !is_checksum_valid( ehdrp, ehdrp + 1)) {
mlog( MLOG_NORMAL | MLOG_WARNING, _(
"bad extent header checksum\n") );
return RV_CORRUPT;
}
}
-#endif /* EXTENTHDR_CHECKSUM */
return RV_OK;
}
@@ -8137,11 +8121,6 @@ read_dirent( drive_t *drivep,
drive_ops_t *dop = drivep->d_opsp;
/* REFERENCED */
intgen_t nread;
-#ifdef DIRENTHDR_CHECKSUM
- register u_int32_t *sump = ( u_int32_t * )dhdrp;
- register u_int32_t *endp = ( u_int32_t * )( dhdrp + 1 );
- register u_int32_t sum;
-#endif /* DIRENTHDR_CHECKSUM */
intgen_t rval;
direnthdr_t tmpdh;
@@ -8180,21 +8159,18 @@ read_dirent( drive_t *drivep,
( size_t )dhdrp->dh_gen,
( size_t )dhdrp->dh_sz );
-#ifdef DIRENTHDR_CHECKSUM
if ( dhcs ) {
if ( dhdrp->dh_sz == 0 ) {
mlog( MLOG_NORMAL | MLOG_WARNING, _(
"corrupt directory entry header\n") );
return RV_CORRUPT;
}
- for ( sum = 0 ; sump < endp ; sum += *sump++ ) ;
- if ( sum ) {
+ if ( !is_checksum_valid( dhdrp, dhdrp + 1)) {
mlog( MLOG_NORMAL | MLOG_WARNING, _(
"bad directory entry header checksum\n") );
return RV_CORRUPT;
}
}
-#endif /* DIRENTHDR_CHECKSUM */
/* if null, return
*/
@@ -8246,11 +8222,6 @@ read_extattrhdr( drive_t *drivep, extattrhdr_t *ahdrp, bool_t ahcs )
drive_ops_t *dop = drivep->d_opsp;
/* REFERENCED */
intgen_t nread;
-#ifdef EXTATTRHDR_CHECKSUM
- register u_int32_t *sump = ( u_int32_t * )ahdrp;
- register u_int32_t *endp = ( u_int32_t * )( ahdrp + 1 );
- register u_int32_t sum;
-#endif /* EXTATTRHDR_CHECKSUM */
intgen_t rval;
extattrhdr_t tmpah;
@@ -8288,21 +8259,18 @@ read_extattrhdr( drive_t *drivep, extattrhdr_t *ahdrp, bool_t ahcs )
ahdrp->ah_valsz,
ahdrp->ah_checksum );
-#ifdef EXTATTRHDR_CHECKSUM
if ( ahcs ) {
if ( ! ( ahdrp->ah_flags & EXTATTRHDR_FLAGS_CHECKSUM )) {
mlog( MLOG_NORMAL | MLOG_WARNING, _(
"corrupt extattr header\n") );
return RV_CORRUPT;
}
- for ( sum = 0 ; sump < endp ; sum += *sump++ ) ;
- if ( sum ) {
+ if ( !is_checksum_valid( ahdrp, ahdrp + 1)) {
mlog( MLOG_NORMAL | MLOG_WARNING, _(
"bad extattr header checksum\n") );
return RV_CORRUPT;
}
}
-#endif /* EXTATTRHDR_CHECKSUM */
return RV_OK;
}
--
1.7.0.4
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] xfsdump: enable dump header checksums
2011-08-29 21:41 [PATCH] xfsdump: enable dump header checksums Bill Kendall
@ 2011-09-02 14:51 ` Gim Leong Chin
2011-09-02 15:02 ` Bill Kendall
2011-09-19 20:12 ` Alex Elder
1 sibling, 1 reply; 7+ messages in thread
From: Gim Leong Chin @ 2011-09-02 14:51 UTC (permalink / raw)
To: xfs, Bill Kendall
Hi Bill,
May I understand:
1) Are the check sums written in the dump file?
2) Which fields would have the check sums?
3) If the checksums at restore time do not agree with the fields, is there any kind of recovery action possible?
Thanks!
GL
--- On Tue, 30/8/11, Bill Kendall <wkendall@sgi.com> wrote:
> From: Bill Kendall <wkendall@sgi.com>
> Subject: [PATCH] xfsdump: enable dump header checksums
> To: xfs@oss.sgi.com
> Date: Tuesday, 30 August, 2011, 5:41 AM
> 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>
> ---
> common/content_inode.h | 20 +++++++++++
> dump/content.c
> | 85
> +++++++++++-------------------------------------
> restore/Makefile |
> 2 +-
> restore/content.c
> | 40 ++--------------------
> 4 files changed, 44 insertions(+), 103 deletions(-)
>
> diff --git a/common/content_inode.h
> b/common/content_inode.h
> index 479fdfc..e119354 100644
> --- a/common/content_inode.h
> +++ b/common/content_inode.h
> @@ -347,4 +347,24 @@ typedef struct extattrhdr
> extattrhdr_t;
> /* a linux "secure" mode attribute
> */
>
> +/* Routines for calculating and validating checksums on
> xfsdump headers
> + */
> +static inline u_int32_t
> +calc_checksum(void *startp, void *endp)
> +{
> + u_int32_t sum;
> + u_int32_t *sump = (u_int32_t *)startp;
> + for (sum = 0; sump < (u_int32_t
> *)endp; sum += *sump++);
> + return ~sum + 1;
> +}
> +
> +static inline bool_t
> +is_checksum_valid(void *startp, void *endp)
> +{
> + u_int32_t sum;
> + u_int32_t *sump = (u_int32_t *)startp;
> + for (sum = 0; sump < (u_int32_t
> *)endp; sum += *sump++);
> + return sum == 0 ? BOOL_TRUE :
> BOOL_FALSE;
> +}
> +
> #endif /* CONTENT_INODE_H */
> diff --git a/dump/content.c b/dump/content.c
> index 9905c88..2cf15ba 100644
> --- a/dump/content.c
> +++ b/dump/content.c
> @@ -585,6 +585,13 @@ content_init( intgen_t argc,
> sizeof(
> content_inode_hdr_t ));
> ASSERT( sizeof( extattrhdr_t ) ==
> EXTATTRHDR_SZ );
>
> + /* must be a multiple of 32-bits for
> checksums
> + */
> + ASSERT( FILEHDR_SZ % sizeof( u_int32_t
> ) == 0 );
> + ASSERT( EXTENTHDR_SZ % sizeof(
> u_int32_t ) == 0 );
> + ASSERT( DIRENTHDR_SZ % sizeof(
> u_int32_t ) == 0 );
> + ASSERT( EXTATTRHDR_SZ % sizeof(
> u_int32_t ) == 0 );
> +
> /* calculate offsets of portions of the
> write hdr template
> */
> dwhdrtemplatep = ( drive_hdr_t *
> )gwhdrtemplatep->gh_upper;
> @@ -1491,8 +1498,7 @@ baseuuidbypass:
> var_skip( &fsid, inomap_skip );
>
> /* fill in write header template
> content info. always produce
> - * an inomap and dir dump for
> each media file. flag the checksums
> - * available if so compiled
> (see -D...CHECKSUM in Makefile).
> + * an inomap and dir dump for
> each media file.
> */
> ASSERT( sizeof(
> cwhdrtemplatep->ch_specific ) >= sizeof(
> *scwhdrtemplatep ));
> scwhdrtemplatep->cih_mediafiletype =
> CIH_MEDIAFILETYPE_DATA;
> @@ -1506,15 +1512,9 @@ baseuuidbypass:
> if ( sc_inv_updatepr ) {
>
> scwhdrtemplatep->cih_dumpattr |= CIH_DUMPATTR_INVENTORY;
> }
> -#ifdef FILEHDR_CHECKSUM
> scwhdrtemplatep->cih_dumpattr |=
> CIH_DUMPATTR_FILEHDR_CHECKSUM;
> -#endif /* FILEHDR_CHECKSUM */
> -#ifdef EXTENTHDR_CHECKSUM
> scwhdrtemplatep->cih_dumpattr |=
> CIH_DUMPATTR_EXTENTHDR_CHECKSUM;
> -#endif /* EXTENTHDR_CHECKSUM */
> -#ifdef DIRENTHDR_CHECKSUM
> scwhdrtemplatep->cih_dumpattr |=
> CIH_DUMPATTR_DIRENTHDR_CHECKSUM;
> -#endif /* DIRENTHDR_CHECKSUM */
> scwhdrtemplatep->cih_dumpattr |=
> CIH_DUMPATTR_DIRENTHDR_GEN;
> if ( sc_incrpr ) {
>
> scwhdrtemplatep->cih_dumpattr |=
> CIH_DUMPATTR_INCREMENTAL;
> @@ -1528,10 +1528,8 @@ baseuuidbypass:
> }
> if ( sc_dumpextattrpr ) {
>
> scwhdrtemplatep->cih_dumpattr |= CIH_DUMPATTR_EXTATTR;
> -#ifdef EXTATTRHDR_CHECKSUM
>
> scwhdrtemplatep->cih_dumpattr |=
>
>
> CIH_DUMPATTR_EXTATTRHDR_CHECKSUM;
> -#endif /* EXTATTRHDR_CHECKSUM */
> }
>
> scwhdrtemplatep->cih_rootino =
> sc_rootxfsstatp->bs_ino;
> @@ -3743,6 +3741,8 @@ dump_extattr_buildrecord( xfs_bstat_t
> *statp,
>
> namesz, namesrcp,
>
> valuesz );
> ( void )strcpy( namep, namesrcp );
> +
> + memset( ( void * )&tmpah, 0,
> sizeof( tmpah ));
> tmpah.ah_sz = recsz;
> ASSERT( EXTATTRHDR_SZ + namesz <
> UINT16MAX );
> tmpah.ah_valoff = ( u_int16_t )(
> EXTATTRHDR_SZ + namesz );
> @@ -3750,17 +3750,8 @@ dump_extattr_buildrecord(
> xfs_bstat_t *statp,
> (( flag &
> ATTR_ROOT ) ? EXTATTRHDR_FLAGS_ROOT :
> (( flag &
> ATTR_SECURE ) ? EXTATTRHDR_FLAGS_SECURE : 0));
> tmpah.ah_valsz = valuesz;
> - tmpah.ah_checksum = 0;
> -#ifdef EXTATTRHDR_CHECKSUM
> - {
> - register u_int32_t *sump = ( u_int32_t
> * )ahdrp;
> - register u_int32_t *endp = ( u_int32_t
> * )( ahdrp + 1 );
> - register u_int32_t sum;
> tmpah.ah_flags |=
> EXTATTRHDR_FLAGS_CHECKSUM;
> - for ( sum = 0 ; sump < endp ; sum +=
> *sump++ ) ;
> - tmpah.ah_checksum = ~sum + 1;
> - }
> -#endif /* EXTATTRHDR_CHECKSUM */
> + tmpah.ah_checksum = calc_checksum(
> &tmpah, &tmpah + 1 );
>
> xlate_extattrhdr(ahdrp, &tmpah,
> -1);
> *valuepp = valuep;
> @@ -3782,23 +3773,13 @@ dump_extattrhdr( drive_t *drivep,
> intgen_t rval;
> rv_t rv;
>
> + memset( ( void * )&ahdr, 0, sizeof(
> ahdr ));
> ahdr.ah_sz = recsz;
> ASSERT( valoff < UINT16MAX );
> ahdr.ah_valoff = ( u_int16_t )valoff;
> - ahdr.ah_flags = ( u_int16_t )flags;
> + ahdr.ah_flags = ( u_int16_t )flags |
> EXTATTRHDR_FLAGS_CHECKSUM;
> ahdr.ah_valsz = valsz;
> - ahdr.ah_checksum = 0;
> -
> -#ifdef EXTATTRHDR_CHECKSUM
> - {
> - register u_int32_t *sump = ( u_int32_t
> * )&ahdr;
> - register u_int32_t *endp = ( u_int32_t
> * )( &ahdr + 1 );
> - register u_int32_t sum;
> - ahdr.ah_flags |=
> EXTATTRHDR_FLAGS_CHECKSUM;
> - for ( sum = 0 ; sump < endp ; sum +=
> *sump++ ) ;
> - ahdr.ah_checksum = ~sum + 1;
> - }
> -#endif /* EXTATTRHDR_CHECKSUM */
> + ahdr.ah_checksum = calc_checksum(
> &ahdr, &ahdr + 1 );
>
> xlate_extattrhdr(&ahdr,
> &tmpahdr, 1);
> rval = write_buf( ( char *
> )&tmpahdr,
> @@ -5102,11 +5083,6 @@ dump_filehdr( drive_t *drivep,
> drive_ops_t *dop = drivep->d_opsp;
> register filehdr_t *fhdrp =
> contextp->cc_filehdrp;
> filehdr_t tmpfhdrp;
> -#ifdef FILEHDR_CHECKSUM
> - register u_int32_t *sump = ( u_int32_t
> * )fhdrp;
> - register u_int32_t *endp = ( u_int32_t
> * )( fhdrp + 1 );
> - register u_int32_t sum;
> -#endif /* FILEHDR_CHECKSUM */
> intgen_t rval;
> rv_t rv;
>
> @@ -5118,13 +5094,8 @@ dump_filehdr( drive_t *drivep,
>
> copy_xfs_bstat(&fhdrp->fh_stat, statp);
> }
> fhdrp->fh_offset = offset;
> - fhdrp->fh_flags = flags;
> -
> -#ifdef FILEHDR_CHECKSUM
> - fhdrp->fh_flags |=
> FILEHDR_FLAGS_CHECKSUM;
> - for ( sum = 0 ; sump < endp ; sum +=
> *sump++ ) ;
> - fhdrp->fh_checksum = ~sum + 1;
> -#endif /* FILEHDR_CHECKSUM */
> + fhdrp->fh_flags = flags |
> FILEHDR_FLAGS_CHECKSUM;
> + fhdrp->fh_checksum = calc_checksum(
> fhdrp, fhdrp + 1 );
>
> xlate_filehdr(fhdrp, &tmpfhdrp,
> 1);
> rval = write_buf( ( char *
> )&tmpfhdrp,
> @@ -5164,11 +5135,6 @@ dump_extenthdr( drive_t *drivep,
> drive_ops_t *dop = drivep->d_opsp;
> register extenthdr_t *ehdrp =
> contextp->cc_extenthdrp;
> extenthdr_t tmpehdrp;
> -#ifdef EXTENTHDR_CHECKSUM
> - register u_int32_t *sump = ( u_int32_t
> * )ehdrp;
> - register u_int32_t *endp = ( u_int32_t
> * )( ehdrp + 1 );
> - register u_int32_t sum;
> -#endif /* EXTENTHDR_CHECKSUM */
> intgen_t rval;
> rv_t rv;
> char typestr[20];
> @@ -5198,15 +5164,10 @@ dump_extenthdr( drive_t *drivep,
>
> ( void )memset( ( void * )ehdrp, 0,
> sizeof( *ehdrp ));
> ehdrp->eh_type = type;
> - ehdrp->eh_flags = flags;
> + ehdrp->eh_flags = flags |
> EXTENTHDR_FLAGS_CHECKSUM;
> ehdrp->eh_offset = offset;
> ehdrp->eh_sz = sz;
> -
> -#ifdef EXTENTHDR_CHECKSUM
> - ehdrp->eh_flags |=
> EXTENTHDR_FLAGS_CHECKSUM;
> - for ( sum = 0 ; sump < endp ; sum +=
> *sump++ ) ;
> - ehdrp->eh_checksum = ~sum + 1;
> -#endif /* EXTENTHDR_CHECKSUM */
> + ehdrp->eh_checksum = calc_checksum(
> ehdrp, ehdrp + 1 );
>
> xlate_extenthdr(ehdrp, &tmpehdrp,
> 1);
> rval = write_buf( ( char *
> )&tmpehdrp,
> @@ -5249,11 +5210,6 @@ dump_dirent( drive_t *drivep,
> direnthdr_t *tmpdhdrp;
> size_t direntbufsz =
> contextp->cc_mdirentbufsz;
> size_t sz;
> -#ifdef DIRENTHDR_CHECKSUM
> - register u_int32_t *sump = ( u_int32_t
> * )dhdrp;
> - register u_int32_t *endp = ( u_int32_t
> * )( dhdrp + 1 );
> - register u_int32_t sum;
> -#endif /* DIRENTHDR_CHECKSUM */
> intgen_t rval;
> rv_t rv;
>
> @@ -5290,10 +5246,7 @@ dump_dirent( drive_t *drivep,
> strcpy(
> dhdrp->dh_name, name );
> }
>
> -#ifdef DIRENTHDR_CHECKSUM
> - for ( sum = 0 ; sump < endp ; sum +=
> *sump++ ) ;
> - dhdrp->dh_checksum = ~sum + 1;
> -#endif /* DIRENTHDR_CHECKSUM */
> + dhdrp->dh_checksum = calc_checksum(
> dhdrp, dhdrp + 1);
>
> tmpdhdrp = malloc(sz);
> xlate_direnthdr(dhdrp, tmpdhdrp, 1);
> diff --git a/restore/Makefile b/restore/Makefile
> index 78ecc2c..588a8f0 100644
> --- a/restore/Makefile
> +++ b/restore/Makefile
> @@ -103,7 +103,7 @@ LLDLIBS = $(LIBUUID) $(LIBHANDLE)
> $(LIBATTR) $(LIBRMT)
> LTDEPENDENCIES = $(LIBRMT)
>
> LCFLAGS = -DRESTORE -DRMT -DBASED -DDOSOCKS -DINVCONVFIX
> -DPIPEINVFIX \
> - -DEOMFIX -DSESSCPLT -DWHITEPARSE
> -DDIRENTHDR_CHECKSUM \
> + -DEOMFIX -DSESSCPLT -DWHITEPARSE \
> -DF_FSSETDM
>
> default: depend $(LTCOMMAND)
> diff --git a/restore/content.c b/restore/content.c
> index e3e4994..25849d7 100644
> --- a/restore/content.c
> +++ b/restore/content.c
> @@ -8000,11 +8000,6 @@ read_filehdr( drive_t *drivep,
> filehdr_t *fhdrp, bool_t fhcs )
> drive_ops_t *dop = drivep->d_opsp;
> /* REFERENCED */
> intgen_t nread;
> -#ifdef FILEHDR_CHECKSUM
> - register u_int32_t *sump = ( u_int32_t
> * )fhdrp;
> - register u_int32_t *endp = ( u_int32_t
> * )( fhdrp + 1 );
> - register u_int32_t sum;
> -#endif /* FILEHDR_CHECKSUM */
> intgen_t rval;
> filehdr_t tmpfh;
>
> @@ -8041,21 +8036,18 @@ read_filehdr( drive_t *drivep,
> filehdr_t *fhdrp, bool_t fhcs )
>
> bstatp->bs_ino,
> bstatp->bs_mode
> );
>
> -#ifdef FILEHDR_CHECKSUM
> if ( fhcs ) {
> if ( ! (
> fhdrp->fh_flags & FILEHDR_FLAGS_CHECKSUM )) {
>
> mlog( MLOG_NORMAL | MLOG_WARNING, _(
>
> "corrupt file header\n") );
>
> return RV_CORRUPT;
> }
> - for ( sum = 0 ; sump
> < endp ; sum += *sump++ ) ;
> - if ( sum ) {
> + if (
> !is_checksum_valid( fhdrp, fhdrp + 1)) {
>
> mlog( MLOG_NORMAL | MLOG_WARNING, _(
>
> "bad file header checksum\n") );
>
> return RV_CORRUPT;
> }
> }
> -#endif /* FILEHDR_CHECKSUM */
>
> return RV_OK;
> }
> @@ -8067,11 +8059,6 @@ read_extenthdr( drive_t *drivep,
> extenthdr_t *ehdrp, bool_t ehcs )
> drive_ops_t *dop = drivep->d_opsp;
> /* REFERENCED */
> intgen_t nread;
> -#ifdef EXTENTHDR_CHECKSUM
> - register u_int32_t *sump = ( u_int32_t
> * )ehdrp;
> - register u_int32_t *endp = ( u_int32_t
> * )( ehdrp + 1 );
> - register u_int32_t sum;
> -#endif /* EXTENTHDR_CHECKSUM */
> intgen_t rval;
> extenthdr_t tmpeh;
>
> @@ -8108,21 +8095,18 @@ read_extenthdr( drive_t *drivep,
> extenthdr_t *ehdrp, bool_t ehcs )
>
> ehdrp->eh_type,
> ehdrp->eh_flags
> );
>
> -#ifdef EXTENTHDR_CHECKSUM
> if ( ehcs ) {
> if ( ! (
> ehdrp->eh_flags & EXTENTHDR_FLAGS_CHECKSUM )) {
>
> mlog( MLOG_NORMAL | MLOG_WARNING, _(
>
> "corrupt extent header\n") );
>
> return RV_CORRUPT;
> }
> - for ( sum = 0 ; sump
> < endp ; sum += *sump++ ) ;
> - if ( sum ) {
> + if (
> !is_checksum_valid( ehdrp, ehdrp + 1)) {
>
> mlog( MLOG_NORMAL | MLOG_WARNING, _(
>
> "bad extent header checksum\n") );
>
> return RV_CORRUPT;
> }
> }
> -#endif /* EXTENTHDR_CHECKSUM */
>
> return RV_OK;
> }
> @@ -8137,11 +8121,6 @@ read_dirent( drive_t *drivep,
> drive_ops_t *dop = drivep->d_opsp;
> /* REFERENCED */
> intgen_t nread;
> -#ifdef DIRENTHDR_CHECKSUM
> - register u_int32_t *sump = ( u_int32_t
> * )dhdrp;
> - register u_int32_t *endp = ( u_int32_t
> * )( dhdrp + 1 );
> - register u_int32_t sum;
> -#endif /* DIRENTHDR_CHECKSUM */
> intgen_t rval;
> direnthdr_t tmpdh;
>
> @@ -8180,21 +8159,18 @@ read_dirent( drive_t *drivep,
> ( size_t
> )dhdrp->dh_gen,
> ( size_t
> )dhdrp->dh_sz );
>
> -#ifdef DIRENTHDR_CHECKSUM
> if ( dhcs ) {
> if ( dhdrp->dh_sz
> == 0 ) {
>
> mlog( MLOG_NORMAL | MLOG_WARNING, _(
>
> "corrupt directory entry header\n") );
>
> return RV_CORRUPT;
> }
> - for ( sum = 0 ; sump
> < endp ; sum += *sump++ ) ;
> - if ( sum ) {
> + if (
> !is_checksum_valid( dhdrp, dhdrp + 1)) {
>
> mlog( MLOG_NORMAL | MLOG_WARNING, _(
>
> "bad directory entry header
> checksum\n") );
>
> return RV_CORRUPT;
> }
> }
> -#endif /* DIRENTHDR_CHECKSUM */
>
> /* if null, return
> */
> @@ -8246,11 +8222,6 @@ read_extattrhdr( drive_t *drivep,
> extattrhdr_t *ahdrp, bool_t ahcs )
> drive_ops_t *dop = drivep->d_opsp;
> /* REFERENCED */
> intgen_t nread;
> -#ifdef EXTATTRHDR_CHECKSUM
> - register u_int32_t *sump = ( u_int32_t
> * )ahdrp;
> - register u_int32_t *endp = ( u_int32_t
> * )( ahdrp + 1 );
> - register u_int32_t sum;
> -#endif /* EXTATTRHDR_CHECKSUM */
> intgen_t rval;
> extattrhdr_t tmpah;
>
> @@ -8288,21 +8259,18 @@ read_extattrhdr( drive_t *drivep,
> extattrhdr_t *ahdrp, bool_t ahcs )
>
> ahdrp->ah_valsz,
>
> ahdrp->ah_checksum );
>
> -#ifdef EXTATTRHDR_CHECKSUM
> if ( ahcs ) {
> if ( ! (
> ahdrp->ah_flags & EXTATTRHDR_FLAGS_CHECKSUM )) {
>
> mlog( MLOG_NORMAL | MLOG_WARNING, _(
>
> "corrupt extattr header\n") );
>
> return RV_CORRUPT;
> }
> - for ( sum = 0 ; sump
> < endp ; sum += *sump++ ) ;
> - if ( sum ) {
> + if (
> !is_checksum_valid( ahdrp, ahdrp + 1)) {
>
> mlog( MLOG_NORMAL | MLOG_WARNING, _(
>
> "bad extattr header checksum\n") );
>
> return RV_CORRUPT;
> }
> }
> -#endif /* EXTATTRHDR_CHECKSUM */
>
> return RV_OK;
> }
> --
> 1.7.0.4
>
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs
>
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] xfsdump: enable dump header checksums
2011-09-02 14:51 ` Gim Leong Chin
@ 2011-09-02 15:02 ` Bill Kendall
0 siblings, 0 replies; 7+ messages in thread
From: Bill Kendall @ 2011-09-02 15:02 UTC (permalink / raw)
To: Gim Leong Chin; +Cc: xfs
On 09/02/2011 09:51 AM, Gim Leong Chin wrote:
> Hi Bill,
>
>
> May I understand:
>
> 1) Are the check sums written in the dump file?
> 2) Which fields would have the check sums?
The headers that precede directory entries, files (inodes),
extents (user data) and extended attributes all have checksum
fields. They are stored in the dump stream and the stream has
flags indicating whether or not the checksums were written so
that restore knows if it should check them.
> 3) If the checksums at restore time do not agree with the fields, is there any kind of recovery action possible?
Possibly, though that's not part of this patch. For example if
a file header was corrupt, it may be possible to skip forward
until the next valid file header is found and continue the
restore from there.
Thanks,
Bill
>
> Thanks!
>
>
> GL
>
> --- On Tue, 30/8/11, Bill Kendall<wkendall@sgi.com> wrote:
>
>> From: Bill Kendall<wkendall@sgi.com>
>> Subject: [PATCH] xfsdump: enable dump header checksums
>> To: xfs@oss.sgi.com
>> Date: Tuesday, 30 August, 2011, 5:41 AM
>> 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>
>> ---
>> common/content_inode.h | 20 +++++++++++
>> dump/content.c
>> | 85
>> +++++++++++-------------------------------------
>> restore/Makefile |
>> 2 +-
>> restore/content.c
>> | 40 ++--------------------
>> 4 files changed, 44 insertions(+), 103 deletions(-)
>>
>> diff --git a/common/content_inode.h
>> b/common/content_inode.h
>> index 479fdfc..e119354 100644
>> --- a/common/content_inode.h
>> +++ b/common/content_inode.h
>> @@ -347,4 +347,24 @@ typedef struct extattrhdr
>> extattrhdr_t;
>> /* a linux "secure" mode attribute
>> */
>>
>> +/* Routines for calculating and validating checksums on
>> xfsdump headers
>> + */
>> +static inline u_int32_t
>> +calc_checksum(void *startp, void *endp)
>> +{
>> + u_int32_t sum;
>> + u_int32_t *sump = (u_int32_t *)startp;
>> + for (sum = 0; sump< (u_int32_t
>> *)endp; sum += *sump++);
>> + return ~sum + 1;
>> +}
>> +
>> +static inline bool_t
>> +is_checksum_valid(void *startp, void *endp)
>> +{
>> + u_int32_t sum;
>> + u_int32_t *sump = (u_int32_t *)startp;
>> + for (sum = 0; sump< (u_int32_t
>> *)endp; sum += *sump++);
>> + return sum == 0 ? BOOL_TRUE :
>> BOOL_FALSE;
>> +}
>> +
>> #endif /* CONTENT_INODE_H */
>> diff --git a/dump/content.c b/dump/content.c
>> index 9905c88..2cf15ba 100644
>> --- a/dump/content.c
>> +++ b/dump/content.c
>> @@ -585,6 +585,13 @@ content_init( intgen_t argc,
>> sizeof(
>> content_inode_hdr_t ));
>> ASSERT( sizeof( extattrhdr_t ) ==
>> EXTATTRHDR_SZ );
>>
>> + /* must be a multiple of 32-bits for
>> checksums
>> + */
>> + ASSERT( FILEHDR_SZ % sizeof( u_int32_t
>> ) == 0 );
>> + ASSERT( EXTENTHDR_SZ % sizeof(
>> u_int32_t ) == 0 );
>> + ASSERT( DIRENTHDR_SZ % sizeof(
>> u_int32_t ) == 0 );
>> + ASSERT( EXTATTRHDR_SZ % sizeof(
>> u_int32_t ) == 0 );
>> +
>> /* calculate offsets of portions of the
>> write hdr template
>> */
>> dwhdrtemplatep = ( drive_hdr_t *
>> )gwhdrtemplatep->gh_upper;
>> @@ -1491,8 +1498,7 @@ baseuuidbypass:
>> var_skip(&fsid, inomap_skip );
>>
>> /* fill in write header template
>> content info. always produce
>> - * an inomap and dir dump for
>> each media file. flag the checksums
>> - * available if so compiled
>> (see -D...CHECKSUM in Makefile).
>> + * an inomap and dir dump for
>> each media file.
>> */
>> ASSERT( sizeof(
>> cwhdrtemplatep->ch_specific )>= sizeof(
>> *scwhdrtemplatep ));
>> scwhdrtemplatep->cih_mediafiletype =
>> CIH_MEDIAFILETYPE_DATA;
>> @@ -1506,15 +1512,9 @@ baseuuidbypass:
>> if ( sc_inv_updatepr ) {
>>
>> scwhdrtemplatep->cih_dumpattr |= CIH_DUMPATTR_INVENTORY;
>> }
>> -#ifdef FILEHDR_CHECKSUM
>> scwhdrtemplatep->cih_dumpattr |=
>> CIH_DUMPATTR_FILEHDR_CHECKSUM;
>> -#endif /* FILEHDR_CHECKSUM */
>> -#ifdef EXTENTHDR_CHECKSUM
>> scwhdrtemplatep->cih_dumpattr |=
>> CIH_DUMPATTR_EXTENTHDR_CHECKSUM;
>> -#endif /* EXTENTHDR_CHECKSUM */
>> -#ifdef DIRENTHDR_CHECKSUM
>> scwhdrtemplatep->cih_dumpattr |=
>> CIH_DUMPATTR_DIRENTHDR_CHECKSUM;
>> -#endif /* DIRENTHDR_CHECKSUM */
>> scwhdrtemplatep->cih_dumpattr |=
>> CIH_DUMPATTR_DIRENTHDR_GEN;
>> if ( sc_incrpr ) {
>>
>> scwhdrtemplatep->cih_dumpattr |=
>> CIH_DUMPATTR_INCREMENTAL;
>> @@ -1528,10 +1528,8 @@ baseuuidbypass:
>> }
>> if ( sc_dumpextattrpr ) {
>>
>> scwhdrtemplatep->cih_dumpattr |= CIH_DUMPATTR_EXTATTR;
>> -#ifdef EXTATTRHDR_CHECKSUM
>>
>> scwhdrtemplatep->cih_dumpattr |=
>>
>>
>> CIH_DUMPATTR_EXTATTRHDR_CHECKSUM;
>> -#endif /* EXTATTRHDR_CHECKSUM */
>> }
>>
>> scwhdrtemplatep->cih_rootino =
>> sc_rootxfsstatp->bs_ino;
>> @@ -3743,6 +3741,8 @@ dump_extattr_buildrecord( xfs_bstat_t
>> *statp,
>>
>> namesz, namesrcp,
>>
>> valuesz );
>> ( void )strcpy( namep, namesrcp );
>> +
>> + memset( ( void * )&tmpah, 0,
>> sizeof( tmpah ));
>> tmpah.ah_sz = recsz;
>> ASSERT( EXTATTRHDR_SZ + namesz<
>> UINT16MAX );
>> tmpah.ah_valoff = ( u_int16_t )(
>> EXTATTRHDR_SZ + namesz );
>> @@ -3750,17 +3750,8 @@ dump_extattr_buildrecord(
>> xfs_bstat_t *statp,
>> (( flag&
>> ATTR_ROOT ) ? EXTATTRHDR_FLAGS_ROOT :
>> (( flag&
>> ATTR_SECURE ) ? EXTATTRHDR_FLAGS_SECURE : 0));
>> tmpah.ah_valsz = valuesz;
>> - tmpah.ah_checksum = 0;
>> -#ifdef EXTATTRHDR_CHECKSUM
>> - {
>> - register u_int32_t *sump = ( u_int32_t
>> * )ahdrp;
>> - register u_int32_t *endp = ( u_int32_t
>> * )( ahdrp + 1 );
>> - register u_int32_t sum;
>> tmpah.ah_flags |=
>> EXTATTRHDR_FLAGS_CHECKSUM;
>> - for ( sum = 0 ; sump< endp ; sum +=
>> *sump++ ) ;
>> - tmpah.ah_checksum = ~sum + 1;
>> - }
>> -#endif /* EXTATTRHDR_CHECKSUM */
>> + tmpah.ah_checksum = calc_checksum(
>> &tmpah,&tmpah + 1 );
>>
>> xlate_extattrhdr(ahdrp,&tmpah,
>> -1);
>> *valuepp = valuep;
>> @@ -3782,23 +3773,13 @@ dump_extattrhdr( drive_t *drivep,
>> intgen_t rval;
>> rv_t rv;
>>
>> + memset( ( void * )&ahdr, 0, sizeof(
>> ahdr ));
>> ahdr.ah_sz = recsz;
>> ASSERT( valoff< UINT16MAX );
>> ahdr.ah_valoff = ( u_int16_t )valoff;
>> - ahdr.ah_flags = ( u_int16_t )flags;
>> + ahdr.ah_flags = ( u_int16_t )flags |
>> EXTATTRHDR_FLAGS_CHECKSUM;
>> ahdr.ah_valsz = valsz;
>> - ahdr.ah_checksum = 0;
>> -
>> -#ifdef EXTATTRHDR_CHECKSUM
>> - {
>> - register u_int32_t *sump = ( u_int32_t
>> * )&ahdr;
>> - register u_int32_t *endp = ( u_int32_t
>> * )(&ahdr + 1 );
>> - register u_int32_t sum;
>> - ahdr.ah_flags |=
>> EXTATTRHDR_FLAGS_CHECKSUM;
>> - for ( sum = 0 ; sump< endp ; sum +=
>> *sump++ ) ;
>> - ahdr.ah_checksum = ~sum + 1;
>> - }
>> -#endif /* EXTATTRHDR_CHECKSUM */
>> + ahdr.ah_checksum = calc_checksum(
>> &ahdr,&ahdr + 1 );
>>
>> xlate_extattrhdr(&ahdr,
>> &tmpahdr, 1);
>> rval = write_buf( ( char *
>> )&tmpahdr,
>> @@ -5102,11 +5083,6 @@ dump_filehdr( drive_t *drivep,
>> drive_ops_t *dop = drivep->d_opsp;
>> register filehdr_t *fhdrp =
>> contextp->cc_filehdrp;
>> filehdr_t tmpfhdrp;
>> -#ifdef FILEHDR_CHECKSUM
>> - register u_int32_t *sump = ( u_int32_t
>> * )fhdrp;
>> - register u_int32_t *endp = ( u_int32_t
>> * )( fhdrp + 1 );
>> - register u_int32_t sum;
>> -#endif /* FILEHDR_CHECKSUM */
>> intgen_t rval;
>> rv_t rv;
>>
>> @@ -5118,13 +5094,8 @@ dump_filehdr( drive_t *drivep,
>>
>> copy_xfs_bstat(&fhdrp->fh_stat, statp);
>> }
>> fhdrp->fh_offset = offset;
>> - fhdrp->fh_flags = flags;
>> -
>> -#ifdef FILEHDR_CHECKSUM
>> - fhdrp->fh_flags |=
>> FILEHDR_FLAGS_CHECKSUM;
>> - for ( sum = 0 ; sump< endp ; sum +=
>> *sump++ ) ;
>> - fhdrp->fh_checksum = ~sum + 1;
>> -#endif /* FILEHDR_CHECKSUM */
>> + fhdrp->fh_flags = flags |
>> FILEHDR_FLAGS_CHECKSUM;
>> + fhdrp->fh_checksum = calc_checksum(
>> fhdrp, fhdrp + 1 );
>>
>> xlate_filehdr(fhdrp,&tmpfhdrp,
>> 1);
>> rval = write_buf( ( char *
>> )&tmpfhdrp,
>> @@ -5164,11 +5135,6 @@ dump_extenthdr( drive_t *drivep,
>> drive_ops_t *dop = drivep->d_opsp;
>> register extenthdr_t *ehdrp =
>> contextp->cc_extenthdrp;
>> extenthdr_t tmpehdrp;
>> -#ifdef EXTENTHDR_CHECKSUM
>> - register u_int32_t *sump = ( u_int32_t
>> * )ehdrp;
>> - register u_int32_t *endp = ( u_int32_t
>> * )( ehdrp + 1 );
>> - register u_int32_t sum;
>> -#endif /* EXTENTHDR_CHECKSUM */
>> intgen_t rval;
>> rv_t rv;
>> char typestr[20];
>> @@ -5198,15 +5164,10 @@ dump_extenthdr( drive_t *drivep,
>>
>> ( void )memset( ( void * )ehdrp, 0,
>> sizeof( *ehdrp ));
>> ehdrp->eh_type = type;
>> - ehdrp->eh_flags = flags;
>> + ehdrp->eh_flags = flags |
>> EXTENTHDR_FLAGS_CHECKSUM;
>> ehdrp->eh_offset = offset;
>> ehdrp->eh_sz = sz;
>> -
>> -#ifdef EXTENTHDR_CHECKSUM
>> - ehdrp->eh_flags |=
>> EXTENTHDR_FLAGS_CHECKSUM;
>> - for ( sum = 0 ; sump< endp ; sum +=
>> *sump++ ) ;
>> - ehdrp->eh_checksum = ~sum + 1;
>> -#endif /* EXTENTHDR_CHECKSUM */
>> + ehdrp->eh_checksum = calc_checksum(
>> ehdrp, ehdrp + 1 );
>>
>> xlate_extenthdr(ehdrp,&tmpehdrp,
>> 1);
>> rval = write_buf( ( char *
>> )&tmpehdrp,
>> @@ -5249,11 +5210,6 @@ dump_dirent( drive_t *drivep,
>> direnthdr_t *tmpdhdrp;
>> size_t direntbufsz =
>> contextp->cc_mdirentbufsz;
>> size_t sz;
>> -#ifdef DIRENTHDR_CHECKSUM
>> - register u_int32_t *sump = ( u_int32_t
>> * )dhdrp;
>> - register u_int32_t *endp = ( u_int32_t
>> * )( dhdrp + 1 );
>> - register u_int32_t sum;
>> -#endif /* DIRENTHDR_CHECKSUM */
>> intgen_t rval;
>> rv_t rv;
>>
>> @@ -5290,10 +5246,7 @@ dump_dirent( drive_t *drivep,
>> strcpy(
>> dhdrp->dh_name, name );
>> }
>>
>> -#ifdef DIRENTHDR_CHECKSUM
>> - for ( sum = 0 ; sump< endp ; sum +=
>> *sump++ ) ;
>> - dhdrp->dh_checksum = ~sum + 1;
>> -#endif /* DIRENTHDR_CHECKSUM */
>> + dhdrp->dh_checksum = calc_checksum(
>> dhdrp, dhdrp + 1);
>>
>> tmpdhdrp = malloc(sz);
>> xlate_direnthdr(dhdrp, tmpdhdrp, 1);
>> diff --git a/restore/Makefile b/restore/Makefile
>> index 78ecc2c..588a8f0 100644
>> --- a/restore/Makefile
>> +++ b/restore/Makefile
>> @@ -103,7 +103,7 @@ LLDLIBS = $(LIBUUID) $(LIBHANDLE)
>> $(LIBATTR) $(LIBRMT)
>> LTDEPENDENCIES = $(LIBRMT)
>>
>> LCFLAGS = -DRESTORE -DRMT -DBASED -DDOSOCKS -DINVCONVFIX
>> -DPIPEINVFIX \
>> - -DEOMFIX -DSESSCPLT -DWHITEPARSE
>> -DDIRENTHDR_CHECKSUM \
>> + -DEOMFIX -DSESSCPLT -DWHITEPARSE \
>> -DF_FSSETDM
>>
>> default: depend $(LTCOMMAND)
>> diff --git a/restore/content.c b/restore/content.c
>> index e3e4994..25849d7 100644
>> --- a/restore/content.c
>> +++ b/restore/content.c
>> @@ -8000,11 +8000,6 @@ read_filehdr( drive_t *drivep,
>> filehdr_t *fhdrp, bool_t fhcs )
>> drive_ops_t *dop = drivep->d_opsp;
>> /* REFERENCED */
>> intgen_t nread;
>> -#ifdef FILEHDR_CHECKSUM
>> - register u_int32_t *sump = ( u_int32_t
>> * )fhdrp;
>> - register u_int32_t *endp = ( u_int32_t
>> * )( fhdrp + 1 );
>> - register u_int32_t sum;
>> -#endif /* FILEHDR_CHECKSUM */
>> intgen_t rval;
>> filehdr_t tmpfh;
>>
>> @@ -8041,21 +8036,18 @@ read_filehdr( drive_t *drivep,
>> filehdr_t *fhdrp, bool_t fhcs )
>>
>> bstatp->bs_ino,
>> bstatp->bs_mode
>> );
>>
>> -#ifdef FILEHDR_CHECKSUM
>> if ( fhcs ) {
>> if ( ! (
>> fhdrp->fh_flags& FILEHDR_FLAGS_CHECKSUM )) {
>>
>> mlog( MLOG_NORMAL | MLOG_WARNING, _(
>>
>> "corrupt file header\n") );
>>
>> return RV_CORRUPT;
>> }
>> - for ( sum = 0 ; sump
>> < endp ; sum += *sump++ ) ;
>> - if ( sum ) {
>> + if (
>> !is_checksum_valid( fhdrp, fhdrp + 1)) {
>>
>> mlog( MLOG_NORMAL | MLOG_WARNING, _(
>>
>> "bad file header checksum\n") );
>>
>> return RV_CORRUPT;
>> }
>> }
>> -#endif /* FILEHDR_CHECKSUM */
>>
>> return RV_OK;
>> }
>> @@ -8067,11 +8059,6 @@ read_extenthdr( drive_t *drivep,
>> extenthdr_t *ehdrp, bool_t ehcs )
>> drive_ops_t *dop = drivep->d_opsp;
>> /* REFERENCED */
>> intgen_t nread;
>> -#ifdef EXTENTHDR_CHECKSUM
>> - register u_int32_t *sump = ( u_int32_t
>> * )ehdrp;
>> - register u_int32_t *endp = ( u_int32_t
>> * )( ehdrp + 1 );
>> - register u_int32_t sum;
>> -#endif /* EXTENTHDR_CHECKSUM */
>> intgen_t rval;
>> extenthdr_t tmpeh;
>>
>> @@ -8108,21 +8095,18 @@ read_extenthdr( drive_t *drivep,
>> extenthdr_t *ehdrp, bool_t ehcs )
>>
>> ehdrp->eh_type,
>> ehdrp->eh_flags
>> );
>>
>> -#ifdef EXTENTHDR_CHECKSUM
>> if ( ehcs ) {
>> if ( ! (
>> ehdrp->eh_flags& EXTENTHDR_FLAGS_CHECKSUM )) {
>>
>> mlog( MLOG_NORMAL | MLOG_WARNING, _(
>>
>> "corrupt extent header\n") );
>>
>> return RV_CORRUPT;
>> }
>> - for ( sum = 0 ; sump
>> < endp ; sum += *sump++ ) ;
>> - if ( sum ) {
>> + if (
>> !is_checksum_valid( ehdrp, ehdrp + 1)) {
>>
>> mlog( MLOG_NORMAL | MLOG_WARNING, _(
>>
>> "bad extent header checksum\n") );
>>
>> return RV_CORRUPT;
>> }
>> }
>> -#endif /* EXTENTHDR_CHECKSUM */
>>
>> return RV_OK;
>> }
>> @@ -8137,11 +8121,6 @@ read_dirent( drive_t *drivep,
>> drive_ops_t *dop = drivep->d_opsp;
>> /* REFERENCED */
>> intgen_t nread;
>> -#ifdef DIRENTHDR_CHECKSUM
>> - register u_int32_t *sump = ( u_int32_t
>> * )dhdrp;
>> - register u_int32_t *endp = ( u_int32_t
>> * )( dhdrp + 1 );
>> - register u_int32_t sum;
>> -#endif /* DIRENTHDR_CHECKSUM */
>> intgen_t rval;
>> direnthdr_t tmpdh;
>>
>> @@ -8180,21 +8159,18 @@ read_dirent( drive_t *drivep,
>> ( size_t
>> )dhdrp->dh_gen,
>> ( size_t
>> )dhdrp->dh_sz );
>>
>> -#ifdef DIRENTHDR_CHECKSUM
>> if ( dhcs ) {
>> if ( dhdrp->dh_sz
>> == 0 ) {
>>
>> mlog( MLOG_NORMAL | MLOG_WARNING, _(
>>
>> "corrupt directory entry header\n") );
>>
>> return RV_CORRUPT;
>> }
>> - for ( sum = 0 ; sump
>> < endp ; sum += *sump++ ) ;
>> - if ( sum ) {
>> + if (
>> !is_checksum_valid( dhdrp, dhdrp + 1)) {
>>
>> mlog( MLOG_NORMAL | MLOG_WARNING, _(
>>
>> "bad directory entry header
>> checksum\n") );
>>
>> return RV_CORRUPT;
>> }
>> }
>> -#endif /* DIRENTHDR_CHECKSUM */
>>
>> /* if null, return
>> */
>> @@ -8246,11 +8222,6 @@ read_extattrhdr( drive_t *drivep,
>> extattrhdr_t *ahdrp, bool_t ahcs )
>> drive_ops_t *dop = drivep->d_opsp;
>> /* REFERENCED */
>> intgen_t nread;
>> -#ifdef EXTATTRHDR_CHECKSUM
>> - register u_int32_t *sump = ( u_int32_t
>> * )ahdrp;
>> - register u_int32_t *endp = ( u_int32_t
>> * )( ahdrp + 1 );
>> - register u_int32_t sum;
>> -#endif /* EXTATTRHDR_CHECKSUM */
>> intgen_t rval;
>> extattrhdr_t tmpah;
>>
>> @@ -8288,21 +8259,18 @@ read_extattrhdr( drive_t *drivep,
>> extattrhdr_t *ahdrp, bool_t ahcs )
>>
>> ahdrp->ah_valsz,
>>
>> ahdrp->ah_checksum );
>>
>> -#ifdef EXTATTRHDR_CHECKSUM
>> if ( ahcs ) {
>> if ( ! (
>> ahdrp->ah_flags& EXTATTRHDR_FLAGS_CHECKSUM )) {
>>
>> mlog( MLOG_NORMAL | MLOG_WARNING, _(
>>
>> "corrupt extattr header\n") );
>>
>> return RV_CORRUPT;
>> }
>> - for ( sum = 0 ; sump
>> < endp ; sum += *sump++ ) ;
>> - if ( sum ) {
>> + if (
>> !is_checksum_valid( ahdrp, ahdrp + 1)) {
>>
>> mlog( MLOG_NORMAL | MLOG_WARNING, _(
>>
>> "bad extattr header checksum\n") );
>>
>> return RV_CORRUPT;
>> }
>> }
>> -#endif /* EXTATTRHDR_CHECKSUM */
>>
>> return RV_OK;
>> }
>> --
>> 1.7.0.4
>>
>> _______________________________________________
>> xfs mailing list
>> xfs@oss.sgi.com
>> http://oss.sgi.com/mailman/listinfo/xfs
>>
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] xfsdump: enable dump header checksums
2011-08-29 21:41 [PATCH] xfsdump: enable dump header checksums Bill Kendall
2011-09-02 14:51 ` Gim Leong Chin
@ 2011-09-19 20:12 ` Alex Elder
2011-09-20 0:18 ` Bill Kendall
1 sibling, 1 reply; 7+ messages in thread
From: Alex Elder @ 2011-09-19 20:12 UTC (permalink / raw)
To: Bill Kendall; +Cc: xfs
On Mon, 2011-08-29 at 16:41 -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>
I have a bunch of questions, and a few minor suggestions.
This is a really good thing to get back into dumps. The
change looks OK to me, but I'd like to hear back from you
and maybe have you submit a new version with minor tweaks
before I commit this.
-Alex
How did you select your checksum algorithm? As long as you're
computing one, perhaps you should use one of the established
standard ones with well-understood properties (like CRC-32).
Oh, now I see it was there in the code already. Thank you for
encapsulating that...
Was this used previously in Irix, and thus needs to be done in
a compatible way? Maybe you could implement this but at a
future date implement EXTENTHDR_FLAGS_CRC32 as another flag
that could provide a better checksum.
The theory in doing this unconditionally is that we might as
well record it, even if the restore program chooses to ignore
it, right?
Interesting that struct direnthdr has no flags field. It
looks like the flag that signals it is in use is in the
content_inode_hdr structure. Any idea why the others
include a flag with each structure instance indicating
they are checksummed?
> ---
> common/content_inode.h | 20 +++++++++++
> dump/content.c | 85 +++++++++++-------------------------------------
> restore/Makefile | 2 +-
> restore/content.c | 40 ++--------------------
> 4 files changed, 44 insertions(+), 103 deletions(-)
>
> diff --git a/common/content_inode.h b/common/content_inode.h
> index 479fdfc..e119354 100644
> --- a/common/content_inode.h
> +++ b/common/content_inode.h
> @@ -347,4 +347,24 @@ typedef struct extattrhdr extattrhdr_t;
> /* a linux "secure" mode attribute
> */
>
> +/* Routines for calculating and validating checksums on xfsdump headers
> + */
I know it's fairly obvious on these simple functions, but it
might be nice to state in the header that the number of bytes
used in the checksum is a multiple of 4, and that endp marks
a point *beyond* the last byte used.
> +static inline u_int32_t
> +calc_checksum(void *startp, void *endp)
> +{
> + u_int32_t sum;
> + u_int32_t *sump = (u_int32_t *)startp;
> + for (sum = 0; sump < (u_int32_t *)endp; sum += *sump++);
Put the semicolon on its own line to make it more obvious.
> + return ~sum + 1;
> +}
> +
> +static inline bool_t
> +is_checksum_valid(void *startp, void *endp)
> +{
> + u_int32_t sum;
> + u_int32_t *sump = (u_int32_t *)startp;
> + for (sum = 0; sump < (u_int32_t *)endp; sum += *sump++);
Here too.
> + return sum == 0 ? BOOL_TRUE : BOOL_FALSE;
> +}
> +
> #endif /* CONTENT_INODE_H */
> diff --git a/dump/content.c b/dump/content.c
> index 9905c88..2cf15ba 100644
> --- a/dump/content.c
> +++ b/dump/content.c
> @@ -585,6 +585,13 @@ content_init( intgen_t argc,
> sizeof( content_inode_hdr_t ));
> ASSERT( sizeof( extattrhdr_t ) == EXTATTRHDR_SZ );
>
> + /* must be a multiple of 32-bits for checksums
> + */
> + ASSERT( FILEHDR_SZ % sizeof( u_int32_t ) == 0 );
> + ASSERT( EXTENTHDR_SZ % sizeof( u_int32_t ) == 0 );
> + ASSERT( DIRENTHDR_SZ % sizeof( u_int32_t ) == 0 );
> + ASSERT( EXTATTRHDR_SZ % sizeof( u_int32_t ) == 0 );
If you take the mental leap to assume sizeof(u_int32_t) == 4,
then these checks can be made at compile time. Others might
not like that mental leap, however.
#if (FILEHDR_SZ % 4)
# error "FILEHDR_SZ must be a multiple of 4 (for checksumming)"
#endif
> +
> /* calculate offsets of portions of the write hdr template
> */
> dwhdrtemplatep = ( drive_hdr_t * )gwhdrtemplatep->gh_upper;
. . .
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] xfsdump: enable dump header checksums
2011-09-19 20:12 ` Alex Elder
@ 2011-09-20 0:18 ` Bill Kendall
2011-09-20 16:55 ` Alex Elder
0 siblings, 1 reply; 7+ messages in thread
From: Bill Kendall @ 2011-09-20 0:18 UTC (permalink / raw)
To: aelder; +Cc: xfs
On 09/19/2011 03:12 PM, Alex Elder wrote:
> On Mon, 2011-08-29 at 16:41 -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>
>
> I have a bunch of questions, and a few minor suggestions.
> This is a really good thing to get back into dumps. The
> change looks OK to me, but I'd like to hear back from you
> and maybe have you submit a new version with minor tweaks
> before I commit this.
>
> -Alex
>
> How did you select your checksum algorithm? As long as you're
> computing one, perhaps you should use one of the established
> standard ones with well-understood properties (like CRC-32).
>
> Oh, now I see it was there in the code already. Thank you for
> encapsulating that...
>
> Was this used previously in Irix, and thus needs to be done in
> a compatible way? Maybe you could implement this but at a
> future date implement EXTENTHDR_FLAGS_CRC32 as another flag
> that could provide a better checksum.
Possibly on IRIX, or possibly there's a Linux distro out there
that enabled the checksums. So yes, I wanted to preserve
compatibility. Switching to a standard checksum in the future
as you suggest is a good idea.
>
> The theory in doing this unconditionally is that we might as
> well record it, even if the restore program chooses to ignore
> it, right?
Right. (You probably noticed this also changes restore to
unconditionally verify the checksum, provided the flags
indicate the checksum was recorded.)
>
> Interesting that struct direnthdr has no flags field. It
> looks like the flag that signals it is in use is in the
> content_inode_hdr structure. Any idea why the others
> include a flag with each structure instance indicating
> they are checksummed?
My guess was that each had a flags field for other purposes,
but that's not true for all cases -- struct extenthdr just
has the single checksum flag. So not sure why direnthdr
was done this way.
>
>> ---
>> common/content_inode.h | 20 +++++++++++
>> dump/content.c | 85 +++++++++++-------------------------------------
>> restore/Makefile | 2 +-
>> restore/content.c | 40 ++--------------------
>> 4 files changed, 44 insertions(+), 103 deletions(-)
>>
>> diff --git a/common/content_inode.h b/common/content_inode.h
>> index 479fdfc..e119354 100644
>> --- a/common/content_inode.h
>> +++ b/common/content_inode.h
>> @@ -347,4 +347,24 @@ typedef struct extattrhdr extattrhdr_t;
>> /* a linux "secure" mode attribute
>> */
>>
>> +/* Routines for calculating and validating checksums on xfsdump headers
>> + */
>
> I know it's fairly obvious on these simple functions, but it
> might be nice to state in the header that the number of bytes
> used in the checksum is a multiple of 4, and that endp marks
> a point *beyond* the last byte used.
I've changed this to be more conventional and take a length
argument rather than an end pointer. Also added a comment
about the length restriction.
>
>> +static inline u_int32_t
>> +calc_checksum(void *startp, void *endp)
>> +{
>> + u_int32_t sum;
>> + u_int32_t *sump = (u_int32_t *)startp;
>> + for (sum = 0; sump< (u_int32_t *)endp; sum += *sump++);
>
> Put the semicolon on its own line to make it more obvious.
I reworked this and the case below to be a while loop
to avoid having a loop with an empty body.
u_int32_t sum = 0;
...
while (sump < endp)
sum += *sump++;
>
>> + return ~sum + 1;
>> +}
>> +
>> +static inline bool_t
>> +is_checksum_valid(void *startp, void *endp)
>> +{
>> + u_int32_t sum;
>> + u_int32_t *sump = (u_int32_t *)startp;
>> + for (sum = 0; sump< (u_int32_t *)endp; sum += *sump++);
>
> Here too.
>
>> + return sum == 0 ? BOOL_TRUE : BOOL_FALSE;
>> +}
>> +
>> #endif /* CONTENT_INODE_H */
>> diff --git a/dump/content.c b/dump/content.c
>> index 9905c88..2cf15ba 100644
>> --- a/dump/content.c
>> +++ b/dump/content.c
>> @@ -585,6 +585,13 @@ content_init( intgen_t argc,
>> sizeof( content_inode_hdr_t ));
>> ASSERT( sizeof( extattrhdr_t ) == EXTATTRHDR_SZ );
>>
>> + /* must be a multiple of 32-bits for checksums
>> + */
>> + ASSERT( FILEHDR_SZ % sizeof( u_int32_t ) == 0 );
>> + ASSERT( EXTENTHDR_SZ % sizeof( u_int32_t ) == 0 );
>> + ASSERT( DIRENTHDR_SZ % sizeof( u_int32_t ) == 0 );
>> + ASSERT( EXTATTRHDR_SZ % sizeof( u_int32_t ) == 0 );
>
> If you take the mental leap to assume sizeof(u_int32_t) == 4,
> then these checks can be made at compile time. Others might
> not like that mental leap, however.
>
> #if (FILEHDR_SZ % 4)
> # error "FILEHDR_SZ must be a multiple of 4 (for checksumming)"
> #endif
I think I prefer the way I have it, if only because there
are other checks on those structure sizes in the same block
of code.
I'll repost with changes based on your comments.
Thanks,
Bill
>
>> +
>> /* calculate offsets of portions of the write hdr template
>> */
>> dwhdrtemplatep = ( drive_hdr_t * )gwhdrtemplatep->gh_upper;
>
> . . .
>
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] xfsdump: enable dump header checksums
2011-09-20 0:18 ` Bill Kendall
@ 2011-09-20 16:55 ` Alex Elder
2011-09-22 16:43 ` Christoph Hellwig
0 siblings, 1 reply; 7+ messages in thread
From: Alex Elder @ 2011-09-20 16:55 UTC (permalink / raw)
To: Bill Kendall; +Cc: xfs
On Mon, 2011-09-19 at 19:18 -0500, Bill Kendall wrote:
> On 09/19/2011 03:12 PM, Alex Elder wrote:
. . .
> > The theory in doing this unconditionally is that we might as
> > well record it, even if the restore program chooses to ignore
> > it, right?
>
> Right. (You probably noticed this also changes restore to
> unconditionally verify the checksum, provided the flags
> indicate the checksum was recorded.)
It *might* be nice to have an option to ignore the
checksum on restore. I don't know though. I was
thinking it might be useful if whatever dumped the
data did a buggy checksum but, well, we have no
evidence that xfsdump has ever done that.
. . .
> > I know it's fairly obvious on these simple functions, but it
> > might be nice to state in the header that the number of bytes
> > used in the checksum is a multiple of 4, and that endp marks
> > a point *beyond* the last byte used.
>
> I've changed this to be more conventional and take a length
> argument rather than an end pointer. Also added a comment
> about the length restriction.
I was going to suggest using length, so that sounds good
to me.
-Alex
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] xfsdump: enable dump header checksums
2011-09-20 16:55 ` Alex Elder
@ 2011-09-22 16:43 ` Christoph Hellwig
0 siblings, 0 replies; 7+ messages in thread
From: Christoph Hellwig @ 2011-09-22 16:43 UTC (permalink / raw)
To: Alex Elder; +Cc: xfs
On Tue, Sep 20, 2011 at 11:55:40AM -0500, Alex Elder wrote:
> On Mon, 2011-09-19 at 19:18 -0500, Bill Kendall wrote:
> > On 09/19/2011 03:12 PM, Alex Elder wrote:
> . . .
> > > The theory in doing this unconditionally is that we might as
> > > well record it, even if the restore program chooses to ignore
> > > it, right?
> >
> > Right. (You probably noticed this also changes restore to
> > unconditionally verify the checksum, provided the flags
> > indicate the checksum was recorded.)
>
> It *might* be nice to have an option to ignore the
> checksum on restore. I don't know though. I was
> thinking it might be useful if whatever dumped the
> data did a buggy checksum but, well, we have no
> evidence that xfsdump has ever done that.
I think we really want this option before cutting a new release.
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2011-09-22 16:43 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-08-29 21:41 [PATCH] xfsdump: enable dump header checksums Bill Kendall
2011-09-02 14:51 ` Gim Leong Chin
2011-09-02 15:02 ` Bill Kendall
2011-09-19 20:12 ` Alex Elder
2011-09-20 0:18 ` Bill Kendall
2011-09-20 16:55 ` Alex Elder
2011-09-22 16:43 ` Christoph Hellwig
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox