On Fri, 2011-09-23 at 07:45 -0500, Bill Kendall wrote:
> xfsdump previously contained a bug in the code which generated
> a checksum on the header for extended attributes. This bug
> was recently fixed, but a new xfsrestore will fail if it
> encounters an old dump file which had checksums enabled. (This
> is unlikely since checksums have just recently been enabled in
> the build, and the above-mentioned bug was fixed at the same time.)
>
> This patch uses a new flag in an extattrhdr_t to indicate a
> checksum is present. If this is set, the checksum is validated.
> If instead the old checksum flag is set, a warning is issued saying
> the header could not be validated, and xfsrestore will assume the
> header is valid.
>
> Note that with this change a new dump cannot be restored with an
> old restore which has checksums enabled. But as I mentioned, old
> restores do not have checksums enabled.
>
> Signed-off-by: Bill Kendall <wkendall@xxxxxxx>
This looks fine to me. I have two comments for you to
consider though.
Reviewed-by: Alex Elder <aelder@xxxxxxx>
. . .
> @@ -8197,16 +8198,28 @@ read_extattrhdr( drive_t *drivep, extattrhdr_t *ahdrp, bool_t ahcs )
> ahdrp->ah_checksum );
>
> if ( ahcs ) {
> - if ( ! ( ahdrp->ah_flags & EXTATTRHDR_FLAGS_CHECKSUM )) {
> + if ( ahdrp->ah_flags & EXTATTRHDR_FLAGS_CHECKSUM ) {
> + if ( !is_checksum_valid( ahdrp, EXTATTRHDR_SZ )) {
> + mlog( MLOG_NORMAL | MLOG_WARNING, _(
> + "bad extattr header checksum\n") );
> + return RV_CORRUPT;
> + }
> + } else if ( ahdrp->ah_flags & EXTATTRHDR_FLAGS_OLD_CHECKSUM ) {
> + /* possibly a corrupt header, but most likely an old
> + * header, which cannot be verified due to a bug in how
> + * its checksum was calculated.
> + */
> + if ( !warned ) {
The definition of "warned" could be moved inside this
block so it's clearer this is the only place it is
needed.
> + mlog( MLOG_NORMAL | MLOG_WARNING, _(
> + "extattr header checksum "
> + "could not be verified\n") );
Is there any way to slightly change this message so
that someone who saw it would feel like "I got this
warning but it's really OK"? If I were a user and
got this message I would be a little afraid that
it meant something was really wrong with what got
restored--possibly the whole thing, or just on
some unnamed file, never to be found.
Maybe "old-style extattr header checksums being
ignored". (I'm sure you can come up with better,
I just like to offer *something* when I suggest
a change.)
> + warned = BOOL_TRUE;
> + }
> + } else {
> mlog( MLOG_NORMAL | MLOG_WARNING, _(
. . .
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
