[Ocfs2-tools-devel] [PATCH 7/9] ocfs2-tools: add xattr support in fsck.ocfs2

[Joel Becker]

On Sat, Jan 24, 2009 at 04:03:58PM +0800, Tiger Yang wrote:
> diff --git a/fsck.ocfs2/fsck.ocfs2.checks.8.in b/fsck.ocfs2/fsck.ocfs2.checks.8.in
> index 22982c0..fbd4b5f 100644
> --- a/fsck.ocfs2/fsck.ocfs2.checks.8.in
> +++ b/fsck.ocfs2/fsck.ocfs2.checks.8.in
> @@ -739,6 +739,30 @@ that this value isn't right.
>  
>  Answering yes change this value to the right number.
>  
> +.SS "XATTR_COUNT_INVALID"
> +For entended attributes in inode, block or bucket,  Fsck has found the
> +counts of them isn't right.
> +
> +Answering yes change this value to the right number.

The count of extended attributes in an inode, block, or bucket is
does not match the number of entries found by fsck.

Answering yes will change this to the correct count.

> +
> +.SS "XATTR_ENTRY_INVALID"
> +For entended attributes entry, Fsck has found that it's name_offset
> +isn't right.
> +
> +Answering yes to remove this xattr entry.

The name_offset field of an extended attribute entry is not correct.
Without a correct name_offset field, the entry cannot be used.

Answering yes will remove this entry.

> +
> +.SS "XATTR_HASH_INVALID"
> +For entended attributes entry, Fsck has found that it's name_hash
> +isn't right.
> +
> +Answering yes change this value to the right hash value.

Extended attributes use a hash of their name for lookup purposes.  The
name_hash of this extended attribute entry is not correct.

Answering yes will change this to the correct hash.

> +
> +.SS "XATTR_BLOCK_INVALID"
> +For entended attributes block, Fsck has found that it's signature
> +isn't right.
> +
> +Answering yes to remove this xattr block.

Extended attributes are stored off an extended attribute block
referenced by the inode.  This inode references an invalid extended
attribute block.

Answering yes will remove this block.

> diff --git a/fsck.ocfs2/include/xattr.h b/fsck.ocfs2/include/xattr.h
> new file mode 100644
> index 0000000..3895482
> --- /dev/null
> +++ b/fsck.ocfs2/include/xattr.h
> @@ -0,0 +1,29 @@
> +/* -*- mode: c; c-basic-offset: 8; -*-
> + * vim: noexpandtab sw=8 ts=8 sts=0:
> + *
> + * xattr.h
> + *
> + * Copyright (C) 2004, 2008 Oracle.  All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public
> + * License version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * General Public License for more details.
> + */
> +#ifndef __O2FSCK_XATTR_H__
> +#define __O2FSCK_XATTR_H__
> +
> +#include "fsck.h"
> +
> +#define OCFS2_IS_VALID_XATTR_BLOCK(ptr)                                 \
> +	(!strcmp((ptr)->xb_signature, OCFS2_XATTR_BLOCK_SIGNATURE))

	This macro isn't used - and is not needed.  Just remove it from
xattr.h

> +static void check_xattr_header(o2fsck_state *ost,
> +			       struct ocfs2_dinode *di,
> +			       struct ocfs2_xattr_header *xh,
> +			       int *changed,
> +			       struct xattr_info *xi)
> +{
> +	uint16_t count = 0;
> +	struct ocfs2_xattr_entry *entry = xh->xh_entries;
> +
> +	while (!IS_LAST_ENTRY(entry)) {
> +		count++;
> +		entry++;
> +	}

	Can the IS_LAST_ENTRY loop walk off the end of the header?  I
think it can.  If the object (inode, block, bucket) is full, the last
xattr_entry runs right up against the first name, right?  I think you
have to keep track of where the values are.  That is, if
((char *)entry - (char *)header) > previous_entry->xe_name_offset),
you've walked past the place where entries crosses into the values.
	Not only that, but whether you detect the end by IS_LAST_ENTRY()
or via the check I just described, you can then use
previous_entry->xe_name_offset to validate xh_free_start (shouldn't it
equal previous_entry->xe_name_offset?).  Once you're sure about
xh_free_start, the calling functions can check xe_name_value_len against
the size of xattr space in the inode/block/bucket.  Tao, do you agree
with this?

> +static errcode_t ocfs2_check_xattr_buckets(o2fsck_state *ost,
> +					   struct ocfs2_dinode *di,
> +					   uint64_t blkno,
> +					   uint32_t clusters)
> +{
> +	int i;
> +	errcode_t ret = 0;
> +	char *bucket = NULL;
> +	struct ocfs2_xattr_header *xh;
> +	int blk_per_bucket = ocfs2_blocks_per_xattr_bucket(ost->ost_fs);
> +	uint32_t bpc = ocfs2_xattr_buckets_per_cluster(ost->ost_fs);
> +	uint32_t num_buckets = clusters * bpc;
> +
> +	ret = ocfs2_malloc_blocks(ost->ost_fs->fs_io, blk_per_bucket, &bucket);
> +	if (ret) {
> +		com_err(whoami, ret, "while allocating room to read bucket "
> +			"of xattr data");
> +		goto out;
> +	}
> +
> +	for (i = 0; i < num_buckets; i++, blkno += blk_per_bucket) {
> +		int changed = 0;
> +		struct xattr_info xi= {
> +			.location = "bucket",
> +			.max_offset = blk_per_bucket * ost->ost_fs->fs_blocksize,
> +			.blkno = blkno,
> +		};
> +
> +		ret = io_read_block(ost->ost_fs->fs_io, blkno,
> +				    blk_per_bucket, bucket);
> +		if (ret) {
> +			com_err(whoami, ret, "while reading blocks of xattr "
> +				"bucket");
> +			goto out;
> +		}
> +
> +		xh = (struct ocfs2_xattr_header *)bucket;
> +		ocfs2_swap_xattrs_to_cpu(xh);
> +		/*
> +		 * The real bucket num in this series of blocks is stored
> +		 * in the 1st bucket.
> +		 */
> +		if (i == 0)
> +			num_buckets = xh->xh_num_buckets;

	You need to check xh_num_buckets.  It needs to fit within the
number of clusters passed to this function.

> +
> +		ret = check_xattr(ost, di, xh, &changed, &xi);
> +		if (ret)
> +			break;
> +		if (changed) {
> +			ocfs2_swap_xattrs_from_cpu(xh);
> +			io_write_block(ost->ost_fs->fs_io, blkno,
> +				       blk_per_bucket, bucket);

	You need to compute the ecc of the bucket before you write it
out.  Later we want to have a read/write_bucket() API, but for now you
can just calculate the ecc here.  Since it's a linear buffer but
possibly larger than a block, you can do:

	if (ocfs2_meta_ecc(ost->ost_fs))
		ocfs2_block_check_compute(bucket,
					  OCFS2_XATTR_BUCKET_SIZE,
					  &xh->xh_check);


	Overall, much more things are checked.  We're almost there.

Joel

-- 

Life's Little Instruction Book #232

	"Keep your promises."

Joel Becker
Principal Software Developer
Oracle
E-mail: joel.becker at oracle.com
Phone: (650) 506-8127