[Ocfs2-devel] [PATCH 01/14] ocfs2: Introduce ocfs2_xa_loc

[Joel Becker]

On Thu, Aug 27, 2009 at 03:48:19PM +0800, Tao Ma wrote:
> Joel Becker wrote:
> >+	u64 value_size = le64_to_cpu(entry->xe_value_size);
> >+
> >+	namevalue_size = OCFS2_XATTR_SIZE(entry->xe_name_len);
> >+	if (value_size > OCFS2_XATTR_INLINE_SIZE)
> >+		namevalue_size += OCFS2_XATTR_ROOT_SIZE;
> >+	else
> >+		namevalue_size += OCFS2_XATTR_SIZE(value_size);
> A minor issue. I see a similar part in the function in
> ocfs2_xa_block_wipe_namevalue, so can we create a inline function
> for it? maybe ocfs2_xe_name_value_size?

	See patch 5 :-)

> >@@ -1376,7 +1586,14 @@ static void ocfs2_xattr_set_entry_local(struct inode *inode,
> > {
> > 	size_t name_len = strlen(xi->name);
> > 	int i;
> >+	struct ocfs2_xa_loc loc;
> >+	if (xs->xattr_bh == xs->inode_bh)
> >+		ocfs2_init_dinode_xa_loc(&loc, inode, xs->inode_bh,
> >+					 xs->not_found ? NULL : xs->here);
> >+	else
> >+		ocfs2_init_xattr_block_xa_loc(&loc, xs->xattr_bh,
> >+					      xs->not_found ? NULL : xs->here);
> > 	if (xi->value && xs->not_found) {
> > 		/* Insert the new xattr entry. */
> > 		le16_add_cpu(&xs->header->xh_count, 1);
> >@@ -1415,9 +1632,9 @@ static void ocfs2_xattr_set_entry_local(struct inode *inode,
> > 			       xi->value_len);
> > 			return;
> > 		}
> >+
> > 		/* Remove the old name+value. */
> >-		memmove(first_val + size, first_val, val - first_val);
> >-		memset(first_val, 0, size);
> >+		ocfs2_xa_wipe_namevalue(&loc);
> > 		xs->here->xe_name_hash = 0;
> > 		xs->here->xe_name_offset = 0;
> > 		ocfs2_xattr_set_local(xs->here, 1);
> >@@ -1425,23 +1642,16 @@ static void ocfs2_xattr_set_entry_local(struct inode *inode,
> > 		min_offs += size;
> >-		/* Adjust all value offsets. */
> >-		last = xs->header->xh_entries;
> >-		for (i = 0 ; i < le16_to_cpu(xs->header->xh_count); i++) {
> >-			size_t o = le16_to_cpu(last->xe_name_offset);
> >-
> >-			if (o < offs)
> >-				last->xe_name_offset = cpu_to_le16(o + size);
> >-			last += 1;
> >-		}
> >-
> > 		if (!xi->value) {
> > 			/* Remove the old entry. */
> >-			last -= 1;
> >+			i = le16_to_cpu(xs->header->xh_count);
> >+			i--;
> any reason why not "i = le16_to_cpu(xs->header->xh_count) - 1"?

	No good one.  I moved these couple lines around a bit, so I
probably was just adjusting.

> >+			last = &xs->header->xh_entries[i - 1];
> here is a bug. last should be "&xs->header->xh_entries[i]". In the
> old implementation, the above "for" loop ends with last =
> xh_entries[xh_count].

	My brain wants to protest, but every time I walk the logic you
come out right.  Maybe this used to be above the i-- or something.
Good catch.

Joel

-- 

"Always give your best, never get discouraged, never be petty; always
 remember, others may hate you.  Those who hate you don't win unless
 you hate them.  And then you destroy yourself."
	- Richard M. Nixon

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