lkml.org 
[lkml]   [2006]   [Sep]   [5]   [last100]   RSS Feed
Views: [more markup]  [less markup]  [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH 07/16] GFS2: Directory handling
FromSteven Whitehouse <>
DateTue, 05 Sep 2006 09:44:14 +0100
Hi,

On Mon, 2006-09-04 at 13:35 +0200, Jan Engelhardt wrote:
> >+> >+	return copied;> >+fail:> >+	return (copied) ? copied : error;> >+}> >+> >+typedef int (*gfs2_dscan_t)(const struct gfs2_dirent *dent,
> >+			    const struct qstr *name,> >+			    void *opaque);> > Collect all the typedefs above all functions (applies to all .c files).
> 
ok

> >+static inline int __gfs2_dirent_find(const struct gfs2_dirent *dent,
> >+				     const struct qstr *name, int ret)> >+{> >+	if (dent->de_inum.no_addr != 0 &&> >+	    be32_to_cpu(dent->de_hash) == name->hash &&
> >+	    be16_to_cpu(dent->de_name_len) == name->len &&
> >+	    memcmp((char *)(dent+1), name->name, name->len) == 0)> > Nocast.> 
ok

> >+static struct gfs2_dirent *gfs2_dirent_scan(struct inode *inode,
[lines snipped]
> >+	offset = ret;> >+	prev = NULL;> >+	dent = (struct gfs2_dirent *)(buf + offset);> > Nocast.> 
ok
> >+		dent = (struct gfs2_dirent *)(buf + offset);> > > >+	if ((char *)cur + cur_rec_len >= bh_end) {> >+		if ((char *)cur + cur_rec_len > bh_end) {
> >+			gfs2_consist_inode(dip);
> >+			return -EIO;
> >+		}
> >+		return -ENOENT;
> >+	}> > if((char *)cur + cur_rec_len > bh_end) {
> 	gfs2_consist_inode(dip);
> 	return -EIO;
> } else if((char *)cur + cur_rec_len == bh_end)
> 	return -ENOENT;
> 
ok

> >+	tmp = (struct gfs2_dirent *)((char *)cur + cur_rec_len);
> >+> >+	if ((char *)tmp + be16_to_cpu(tmp->de_rec_len) > bh_end) {> > Aah, this makes my eyes hurt. Though, it is probably a task not too short to
> think of something that would do without casts.> 
Well I've had a crack at this. I've got it down to one cast. Let me know
if you think thats ok now... I managed to factor out some common code at
the same time.

> >+	leaf->lf_depth = cpu_to_be16(depth);> >+	leaf->lf_entries = cpu_to_be16(0);> > 0 is said to be portable across all CPUs, therefore
> > 	leaf->lf_entries = 0;
> 
> should suffice.
> 
> >+	leaf->lf_next = cpu_to_be64(0);
> 
ok, both now fixed.

> > >+		for (x = sdp->sd_hash_ptrs; x--; from++) {> >+			*to++ = *from;	/*  No endianess worries  */> >+			*to++ = *from;> >+		}> > Add /* Hakuna Matata */ and there will never be worries :-)
> :-) I had to use google to understand the reference :-)

> >+static int compare_dents(const void *a, const void *b)
> >+{> >+	struct gfs2_dirent *dent_a, *dent_b;> >+	uint32_t hash_a, hash_b;> >+	int ret = 0;> >+> >+	dent_a = *(struct gfs2_dirent **)a;> > But in this function you can have it const! Or not?
> 
Yes, and now all converted to const
[lines snipped]
> 		ret = memcmp((char *)(dent_a + 1),
> >+				     (char *)(dent_b + 1),> >+				     len_a);
> 
> Nocast.
> 
ok

> >+		error = filldir(opaque, (char *)(dent + 1),> > If you case, then cast it directly, in this case, filldir takes "const char *"
> as 2nd type.
> 
ok

> >+	larr = vmalloc((leaves + entries) * sizeof(void*));>                                                        ^
> Space, to go in line with all the other casts.
> 
fixed

> >+static inline uint32_t gfs2_disk_hash(const char *data, int len)
> >+{> >+        return crc32_le(0xFFFFFFFF, data, len) ^ 0xFFFFFFFF;
> >+}> > Mind using (uint32_t)-1 or (uint32_t)~0 for that?> 
~0 is the variant I've chosen.

> >+	memcpy((char*)(dent+1), name->name, name->len);> > Last but not least, nocast.> 
ok.

The patch is:
http://www.kernel.org/git/?p=linux/kernel/git/steve/gfs2-2.6.git;a=commitdiff;h=2bdbc5d73961c040fdc9b30d985fab3047d697a0

Thanks for doing all this review work, btw. I'll be working on your more
recent emails today,

Steve.


-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

\
 
 \ /
  Last update: 2006-09-05 10:41    [from the cache]
©2003-2008