* [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