[Ocfs2-tools-devel] [PATCH 10/12] ocfs2-tools: add xattr support in fsck.ocfs2
Joel Becker
Joel.Becker at oracle.com
Fri Apr 3 17:48:00 PDT 2009
On Wed, Mar 25, 2009 at 10:50:08AM +0800, Tiger Yang wrote:
> +enum xattr_location {
> + IN_INODE = 0,
> + IN_BLOCK,
> + IN_BUCKET
> +};
> +
> +static const char *xattr_object[] = {
> + "inode",
> + "block",
> + "bucket",
> +};
Please add a comment about the mapping and use initializers:
/* These must be kept in sync with xattr_location */
static const char *xattr_object[] = {
[IN_INODE] = "inode",
[IN_BLOCK] = "block",
[IN_BUCKET] = "bcuket",
};
> +/*
> + * This use to indicate the used area of xattr in inode, block and bucket.
> + * The used area include all xattr structs, such as header, entry, name+value.
> + */
> +struct used_area {
> + uint16_t offset; /* the offset of the area */
> + uint16_t length; /* the length of the area */
> + uint16_t entry_num; /* if this area is xattr entry, indicate the
> + number of it (start from 1),
> + otherwise it should be 0. */
> + struct used_area *next; /* indicate the next used area */
> +};
Please prefix the structure fields. eg, "ua_offset", etc. Also, make
used_map its own structure with the total length of the block. That
way, we don't have to special-case the entry number, and you can used
proper 0-based entry numbers.
> +static errcode_t set_used_area(struct used_area *header,
> + uint16_t off, uint16_t len,
> + uint16_t en)
> +{
> + struct used_area *area = NULL;
> + struct used_area *new_area = NULL;
> +
> + if (!header)
> + return OCFS2_ET_INVALID_ARGUMENT;
> +
> + new_area = new_used_area(off, len, en);
> + if (!new_area) {
> + com_err(whoami, OCFS2_ET_NO_MEMORY, "Unable to allocate"
> + " buffer for extended attribute ");
> + return OCFS2_ET_NO_MEMORY;
> + }
> +
> + area = header;
> + while (area->next)
> + area = area->next;
> +
> + new_area->next = area->next;
> + area->next = new_area;
Let's use list_head in the used_area code. That way we can use
list_add_tail() here. Other places can use list_for_each(), etc. So
used_area will have ua_list element, and the used_map structure has
um_areas to hold them.
> +static int check_fits(struct used_area *header, uint16_t off, uint16_t len)
> +{
> + struct used_area *area = NULL;
> +
> + if (!header)
> + return -1;
> +
> + if (off >= header->length)
> + return -1;
We need to compare off+len > header->length. Otherwise we can
have len go past the end of the block.
> + area = header->next;
> + while (area) {
> + if (off >= area->offset &&
> + off < (area->offset + area->length))
> + return -1;
> + area = area->next;
> + }
This loop also needs to consider len:
list_for_each(p, &map->um_areas) {
area = list_entry(p, struct used_area, ua_list);
if ((off + len) <= area->ua_offset)
continue
if ((area->ua_offset + area->ua_length) <= off)
continue;
return -1;
}
> +static errcode_t check_xattr_entry(o2fsck_state *ost,
> + struct ocfs2_dinode *di,
> + struct ocfs2_xattr_header *xh,
> + int *changed,
> + struct xattr_info *xi)
> +{
> + int i, ret = 0;
> + struct used_area *entry = NULL;
> + uint16_t count;
> + struct used_area *used_map;
> +
> +rescan:
> + count = xh->xh_count;
> + used_map = new_used_map(xi->max_offset);
> + /* set xattr header as used area */
> + set_used_area(used_map, 0, sizeof(struct ocfs2_xattr_header), 0);
> +
> + for (i = 0 ; i < xh->xh_count; i++) {
> + struct ocfs2_xattr_entry *xe = &xh->xh_entries[i];
> + uint16_t value_len;
> + uint32_t hash;
> +
> + if (check_fits(used_map, XE_OFFSET(xh, xe), ENTRY_SIZE)) {
> + if (!prompt(ost, PY, PR_XATTR_OFFSET_INVALID,
> + "Extended attribute entry in %s #%"
> + PRIu64" refers to a used area at %u,"
> + " clear this entry?",
> + xattr_object[xi->location], xi->blkno,
> + XE_OFFSET(xh, xe))) {
> + ret = -1;
> + break;
> + } else
> + goto wipe_entry;
> + }
> +
> + /* check and fix name_offset */
> + if (xe->xe_name_offset >= xi->max_offset) {
> + if (!prompt(ost, PY, PR_XATTR_OFFSET_INVALID,
> + "Extended attribute entry in %s #%"PRIu64
> + " refers to an invalid name offset %u,"
> + " clear this entry?",
> + xattr_object[xi->location], xi->blkno,
> + xe->xe_name_offset)) {
> + ret = -1;
> + break;
> + } else
> + goto wipe_entry;
> + }
> +
> + /* check type and value size */
> + if ((ocfs2_xattr_is_local(xe) &&
> + xe->xe_value_size > OCFS2_XATTR_INLINE_SIZE) ||
> + (!ocfs2_xattr_is_local(xe) &&
> + xe->xe_value_size <= OCFS2_XATTR_INLINE_SIZE)) {
> + char *local;
> +
> + if (ocfs2_xattr_is_local(xe))
> + local = "";
> + else
> + local = "not ";
> + if (!prompt(ost, PY, PR_XATTR_LOCATION_INVALID,
> + "Extended attribute entry in %s #%"PRIu64
> + " claims to have value %sin local, but the"
> + " value size is %"PRIu64
> + ", clear this entry?",
> + xattr_object[xi->location], xi->blkno,
> + local, xe->xe_value_size)) {
> + ret = -1;
> + break;
> + } else
> + goto wipe_entry;
> + }
> +
> + /* mark the entry area as used*/
> + set_used_area(used_map, XE_OFFSET(xh, xe), ENTRY_SIZE, i + 1);
> + /* get the value's real size in inode, block or bucket */
> + value_len = ocfs2_xattr_value_real_size(xe->xe_name_len,
> + xe->xe_value_size);
> + if (check_fits(used_map, xe->xe_name_offset, value_len)) {
> + if (!prompt(ost, PY, PR_XATTR_OFFSET_INVALID,
> + "Extended attribute entry in %s #%"PRIu64
> + " refers to a used area at %u,"
> + " clear this entry?",
> + xattr_object[xi->location], xi->blkno,
> + xe->xe_name_offset)) {
> + ret = -1;
> + break;
> + } else {
> + clear_used_area(used_map, XE_OFFSET(xh, xe),
> + ENTRY_SIZE);
> + goto wipe_entry;
> + }
> + }
> + /* mark the value area as used */
> + set_used_area(used_map, xe->xe_name_offset, value_len, 0);
> +
> + /* check and fix name hash */
> + hash = ocfs2_xattr_name_hash(
> + ost->ost_fs->fs_super->id2.i_super.s_uuid_hash,
> + (void *)xh + xe->xe_name_offset, xe->xe_name_len);
> + if (xe->xe_name_hash != hash &&
> + prompt(ost, PY, PR_XATTR_HASH_INVALID,
> + "Extended attribute entry in %s #%"PRIu64
> + " refers to an invalid name hash %u,"
> + " Fix the name hash?",
> + xattr_object[xi->location], xi->blkno,
> + xe->xe_name_hash)) {
> + xe->xe_name_hash = hash;
> + *changed = 1;
> + }
> +
> + continue;
> +wipe_entry:
> + /*
> + * we don't wipe entry at here, just reduce the count,
> + * we will wipe them when we finish the check.
> + */
> + count -= 1;
> + *changed = 1;
> + }
The scan code is nicely done. That's exactly what I was
thinking.
> + i = 1;
> + if (*changed && xh->xh_count != count) {
> + /*
> + * according to used map, remove bad entries from entry area,
> + * and left the name+value in the object.
> + */
> + entry = used_map;
> + while (entry) {
> + if (entry->entry_num) {
> + if (entry->entry_num != i)
> + memcpy(&xh->xh_entries[i - 1],
> + &xh->xh_entries[entry->entry_num - 1],
> + ENTRY_SIZE);
> + i++;
> + }
> + entry = entry->next;
> + }
> + xh->xh_count = i - 1;
> + /*
> + * we need scan the entries again, because we only add valid
> + * entries into used map, some invalid entries's offset happen
> + * to be the position of entries we discard so them could pass
> + * the first check.
> + */
> + free_used_map(used_map);
> + goto rescan;
> + }
But I don't like that we have to rescan here. Rather than
storing the entry number on the used_area, why not store the entire
entry:
new_used_area(off, len, xattr_entry *xe)
{
ua = malloc();
...
memset(ua, 0, sizeof(struct used_area));
ua->ua_offset = off;
ua->ua_length = len;
if (xe) {
memcpy(&ua->ua_xe, xe, ENTRY_SIZE);
ua->ua_xe_valid = 1;
}
return ua;
}
The used_area structures for xattr values will pass NULL in the
xe field, and they will have ua_xe_valid of 0. Now you just copy them
out:
if (*changed && (xh->xh_count != count)) {
i = 0;
list_for_each(p, &used_map->um_areas) {
area = list_entry(p, struct used_area, ua_list);
if (!area->ua_xe_valid)
continue;
memcpy(xh->xh_entries[i], &area->ua_xe, ENTRY_SIZE);
i++;
}
xh->xh_count = i;
}
No need for a rescan.
Everything else looks fine.
Joel
--
"I always thought the hardest questions were those I could not answer.
Now I know they are the ones I can never ask."
- Charlie Watkins
Joel Becker
Principal Software Developer
Oracle
E-mail: joel.becker at oracle.com
Phone: (650) 506-8127
More information about the Ocfs2-tools-devel
mailing list