Re: [PATCH 07/16] GFS2: Directory handling

[Jan Engelhardt <jengelh@linux01.gwdg.de>]

>+
>+	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).

>+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.

>+static struct gfs2_dirent *gfs2_dirent_scan(struct inode *inode,
>+					    void *buf,
>+					    unsigned int len, gfs2_dscan_t scan,
>+					    const struct qstr *name,
>+					    void *opaque)
>+{
>+	struct gfs2_dirent *dent, *prev;
>+	unsigned offset;
>+	unsigned size;
>+	int ret = 0;
>+
>+	ret = gfs2_dirent_offset(buf);
>+	if (ret < 0)
>+		goto consist_inode;
>+
>+	offset = ret;
>+	prev = NULL;
>+	dent = (struct gfs2_dirent *)(buf + offset);

Nocast.

>+		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;

>+	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.

>+	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);


>+		for (x = sdp->sd_hash_ptrs; x--; from++) {
>+			*to++ = *from;	/*  No endianess worries  */
>+			*to++ = *from;
>+		}

Add /* Hakuna Matata */ and there will never be worries :-)

>+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?

>+	hash_a = be32_to_cpu(dent_a->de_hash);
>+
>+	dent_b = *(struct gfs2_dirent **)b;
>+	hash_b = be32_to_cpu(dent_b->de_hash);
>+
>+	if (hash_a > hash_b)
>+		ret = 1;
>+	else if (hash_a < hash_b)
>+		ret = -1;
>+	else {
>+		unsigned int len_a = be16_to_cpu(dent_a->de_name_len);
>+		unsigned int len_b = be16_to_cpu(dent_b->de_name_len);
>+
>+		if (len_a > len_b)
>+			ret = 1;
>+		else if (len_a < len_b)
>+			ret = -1;
>+		else
>+			ret = memcmp((char *)(dent_a + 1),
>+				     (char *)(dent_b + 1),
>+				     len_a);

Nocast.

>+		error = filldir(opaque, (char *)(dent + 1),

If you case, then cast it directly, in this case, filldir takes "const char *"
as 2nd type.

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

>+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?

>+	memcpy((char*)(dent+1), name->name, name->len);

Last but not least, nocast.


Otherwise just stuff already mentioned.

Jan Engelhardt
-- 

-- 
VGER BF report: H 0.0146386
-
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/