Skip to contentSkip to conversation

GFS: headers

PATCH In review David Teigland 4 participants 5 messages Oct 10, 2005 – Oct 12, 2005

Subsystems (2)
VFS GFS2 file system
lore
Share & tools
lore

David Teigland

author
#0 lore raw

PATCH 1/16 GFS: headers

Central header files that are widely used.

Signed-off-by Ken Preslan <[email protected]>
Signed-off-by David Teigland <[email protected]>

---

 fs/gfs2/gfs2.h              |   63 ++
 fs/gfs2/incore.h            |  701 ++++++++++++++++++++++++++++
 include/linux/gfs2_ioctl.h  |   30 +
 include/linux/gfs2_ondisk.h | 1093 ++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 1887 insertions(+)
fs/gfs2/gfs2.h +63 -0
@@ -0,0 +1,63 @@
+/*
+ * Copyright (C) Sistina Software, Inc.  1997-2003 All rights reserved.
+ * Copyright (C) 2004-2005 Red Hat, Inc.  All rights reserved.
+ *
+ * This copyrighted material is made available to anyone wishing to use,
+ * modify, copy, or redistribute it subject to the terms and conditions
+ * of the GNU General Public License v.2.
+ */
+
+#ifndef __GFS2_DOT_H__
+#define __GFS2_DOT_H__
+
+#include <linux/gfs2_ondisk.h>
+
+#include "locking/harness/lm_interface.h"
+#include "lvb.h"
+#include "incore.h"
+#include "util.h"
+
+enum {
+	NO_CREATE = 0,
+	CREATE = 1,
+};
+
+enum {
+	NO_WAIT = 0,
+	WAIT = 1,
+};
+
+enum {
+	NO_FORCE = 0,
+	FORCE = 1,
+};
+
+/*  Divide num by den.  Round up if there is a remainder.  */
+#define DIV_RU(num, den) (((num) + (den) - 1) / (den))
+#define MAKE_MULT8(x) (((x) + 7) & ~7)
+
+#define GFS2_FAST_NAME_SIZE 8
+
+#define get_v2sdp(sb) ((struct gfs2_sbd *)(sb)->s_fs_info)
+#define set_v2sdp(sb, sdp) (sb)->s_fs_info = (sdp)
+#define get_v2ip(inode) ((struct gfs2_inode *)(inode)->u.generic_ip)
+#define set_v2ip(inode, ip) (inode)->u.generic_ip = (ip)
+#define get_v2fp(file) ((struct gfs2_file *)(file)->private_data)
+#define set_v2fp(file, fp) (file)->private_data = (fp)
+#define get_v2bd(bh) ((struct gfs2_bufdata *)(bh)->b_private)
+#define set_v2bd(bh, bd) (bh)->b_private = (bd)
+#define get_v2db(bh) ((struct gfs2_databuf *)(bh)->b_private)
+#define set_v2db(bh, db) (bh)->b_private = (db)
+
+#define get_transaction ((struct gfs2_trans *)(current->journal_info))
+#define set_transaction(tr) (current->journal_info) = (tr)
+
+#define get_gl2ip(gl) ((struct gfs2_inode *)(gl)->gl_object)
+#define set_gl2ip(gl, ip) (gl)->gl_object = (ip)
+#define get_gl2rgd(gl) ((struct gfs2_rgrpd *)(gl)->gl_object)
+#define set_gl2rgd(gl, rgd) (gl)->gl_object = (rgd)
+#define get_gl2gl(gl) ((struct gfs2_glock *)(gl)->gl_object)
+#define set_gl2gl(gl, gl2) (gl)->gl_object = (gl2)
+
+#endif /* __GFS2_DOT_H__ */
+
fs/gfs2/incore.h +701 -0

701 lines not on this page · raw message

include/linux/gfs2_ioctl.h +30 -0
@@ -0,0 +1,30 @@
+/*
+ * Copyright (C) Sistina Software, Inc.  1997-2003 All rights reserved.
+ * Copyright (C) 2004-2005 Red Hat, Inc.  All rights reserved.
+ *
+ * This copyrighted material is made available to anyone wishing to use,
+ * modify, copy, or redistribute it subject to the terms and conditions
+ * of the GNU General Public License v.2.
+ */
+
+#ifndef __GFS2_IOCTL_DOT_H__
+#define __GFS2_IOCTL_DOT_H__
+
+#define _GFS2C_(x)               (('G' << 16) | ('2' << 8) | (x))
+
+/* Ioctls implemented */
+
+#define GFS2_IOCTL_IDENTIFY      _GFS2C_(1)
+#define GFS2_IOCTL_SUPER         _GFS2C_(2)
+
+struct gfs2_ioctl {
+	unsigned int gi_argc;
+	char **gi_argv;
+
+        char __user *gi_data;
+	unsigned int gi_size;
+	uint64_t gi_offset;
+};
+
+#endif /* ___GFS2_IOCTL_DOT_H__ */
+
include/linux/gfs2_ondisk.h +1093 -0

1,093 lines not on this page · raw message

#1 lore raw

fs/gfs2/gfs2.h

Quoted 4 lines · David Teigland
fs/gfs2/gfs2.h +1 -0
 
+#define MAKE_MULT8(x) (((x) + 7) & ~7)

ALIGN()

fs/gfs2/gfs2.h

Quoted 2 lines
fs/gfs2/gfs2.h +2 -0
+#define GFS2_IOCTL_IDENTIFY      _GFS2C_(1)
+#define GFS2_IOCTL_SUPER         _GFS2C_(2)

Since patch is against -mm, please, add 2 entries to Documentation/ioctl-mess.txt. Don't bother with "I: " and "O: ", since they aren't finished yet.

include/linux/gfs2_ondisk.h

Quoted 2 lines
include/linux/gfs2_ondisk.h +0 -0

Please, mark on-disk structures with __le{16,32,64}. It would help typechecking with sparse.

include/linux/gfs2_ondisk.h line 454

Quoted 8 lines
include/linux/gfs2_ondisk.h +8 -0
+#define CPIN_08(s1, s2, member, count) {memcpy((s1->member), (s2->member), (count));}
+#define CPOUT_08(s1, s2, member, count) {memcpy((s2->member), (s1->member), (count));}
+#define CPIN_16(s1, s2, member) {(s1->member) = le16_to_cpu((s2->member));}
+#define CPOUT_16(s1, s2, member) {(s2->member) = cpu_to_le16((s1->member));}
+#define CPIN_32(s1, s2, member) {(s1->member) = le32_to_cpu((s2->member));}
+#define CPOUT_32(s1, s2, member) {(s2->member) = cpu_to_le32((s1->member));}
+#define CPIN_64(s1, s2, member) {(s1->member) = le64_to_cpu((s2->member));}
+#define CPOUT_64(s1, s2, member) {(s2->member) = cpu_to_le64((s1->member));}

Confusing names and implementation. CP{IN,OUT}_08 do memcpy, the rest doesn't. "08" doesn't make sense in CPIN_08, while "16", ... do. CPIN_64() expect fixed-endian value or host-endian? Answer is not obvious until you look at a header. Fingers really want to type CPU every time. I ask you to write a simple script and drop these macros completely.

#2 lore raw
Quoted 7 lines · Alexey Dobriyan

include/linux/gfs2_ioctl.h line 18

Quoted 3 lines
include/linux/gfs2_ioctl.h +2 -0
+#define GFS2_IOCTL_IDENTIFY      _GFS2C_(1)
+#define GFS2_IOCTL_SUPER         _GFS2C_(2)

Since patch is against -mm, please, add 2 entries to Documentation/ioctl-mess.txt. Don't bother with "I: " and "O: ", since they aren't finished yet.

Actually, a patch which is partially against -linus and partially against -mm causes me considerable grief. I usually end up having to split it up.

So a separate patch to update -mm-only stuff would be preferred.

author
#3 lore raw
In reply to Alexey Dobriyan
Quoted 2 lines · Alexey Dobriyan

Please, mark on-disk structures with __le{16,32,64}. It would help typechecking with sparse.

Yes, that's something we're working on.

Quoted 7 lines
Quoted 7 lines
include/linux/gfs2_ondisk.h +7 -0
+#define CPIN_08(s1, s2, member, count) {memcpy((s1->member), (s2->member), (count));}
+#define CPOUT_08(s1, s2, member, count) {memcpy((s2->member), (s1->member), (count));}
+#define CPIN_16(s1, s2, member) {(s1->member) = le16_to_cpu((s2->member));}
+#define CPOUT_16(s1, s2, member) {(s2->member) = cpu_to_le16((s1->member));}
+#define CPIN_32(s1, s2, member) {(s1->member) = le32_to_cpu((s2->member));}
+#define CPOUT_32(s1, s2, member) {(s2->member) = cpu_to_le32((s1->member));}
+#define CPIN_64(s1, s2, member) {(s1->member) = le64_to_cpu((s2->member));}
Quoted 8 lines
Quoted 1 line

+#define CPOUT_64(s1, s2, member) {(s2->member) = cpu_to_le64((s1->member));}

Confusing names and implementation. CP{IN,OUT}_08 do memcpy, the rest doesn't. "08" doesn't make sense in CPIN_08, while "16", ... do. CPIN_64() expect fixed-endian value or host-endian? Answer is not obvious until you look at a header. Fingers really want to type CPU every time. I ask you to write a simple script and drop these macros completely.

I find this to be an ideal situation for macros, actually, cutting out a lot of tedious repetition. I think it clarifies the code considerably in the end. The macros are defined immediately above their use in gfs2_ondisk.h and there's this comment explaining what's going on:

/*
* gfs2_xxx_in - read in an xxx struct
* first arg: the cpu-order structure
* buf: the disk-order buffer
*
* gfs2_xxx_out - write out an xxx struct
* first arg: the cpu-order structure
* buf: the disk-order buffer
*
* gfs2_xxx_print - print out an xxx struct
* first arg: the cpu-order structure
*/

If others said the same thing as you I wouldn't hesitate to admit I'm wrong, but several other picky style reviewers haven't said anything...

Thanks,
Dave

#4 lore raw
In reply to David Teigland

Hi!

include/linux/gfs2_ondisk.h line 457

Quoted 13 lines
include/linux/gfs2_ondisk.h +13 -0
+#ifdef WANT_GFS2_CONVERSION_FUNCTIONS
+
+#define CPIN_08(s1, s2, member, count) {memcpy((s1->member), (s2->member), (count));}
+#define CPOUT_08(s1, s2, member, count) {memcpy((s2->member), (s1->member), (count));}
+#define CPIN_16(s1, s2, member) {(s1->member) = le16_to_cpu((s2->member));}
+#define CPOUT_16(s1, s2, member) {(s2->member) = cpu_to_le16((s1->member));}
+#define CPIN_32(s1, s2, member) {(s1->member) = le32_to_cpu((s2->member));}
+#define CPOUT_32(s1, s2, member) {(s2->member) = cpu_to_le32((s1->member));}
+#define CPIN_64(s1, s2, member) {(s1->member) = le64_to_cpu((s2->member));}
+#define CPOUT_64(s1, s2, member) {(s2->member) = cpu_to_le64((s1->member));}
+
+#define pv(struct, member, fmt) printk("  "#member" = "fmt"\n", struct->member);
+#define pa(struct, member, count) print_array(#member, struct->member, count);

Nice way to obfuscate code, I'd say. Can you just inline those macros? This is very hard to read...
Pavel

Signature · 1 line
if you have sharp zaurus hardware you don't need... you know my address