On May 8 2007 09:22, Thomas Gleixner wrote:
>> @@ -0,0 +1,14 @@
>> +obj-$(CONFIG_LOGFS) += logfs.o
>> +
>> +logfs-y += compr.o
>> +logfs-y += dir.o
>> +logfs-y += file.o
>> +logfs-y += gc.o
>> +logfs-y += inode.o
>> +logfs-y += journal.o
>> +logfs-y += memtree.o
>> +logfs-y += readwrite.o
>> +logfs-y += segment.o
>> +logfs-y += super.o
>> +logfs-y += progs/fsck.o
>> +logfs-y += progs/mkfs.o
>
>Please use either tabs or spaces. Preferrably tabs
Or just put it on one line?
logfs-y += compr.o dir.o file.o gc.o ...
>> +
>> +#define LOGFS_IF_VALID 0x00000001 /* inode exists */
>> +#define LOGFS_IF_EMBEDDED 0x00000002 /* data embedded in block pointers */
>> +#define LOGFS_IF_ZOMBIE 0x00000004 /* inode was already deleted
>> */
>> +#define LOGFS_IF_STILLBORN 0x40000000 /* couldn't write inode in creat() */
>> +#define LOGFS_IF_INVALID 0x80000000 /* inode does not exist */
>
>Are these bit values or enum type ?
Does it make any difference? As long as a bitvalue fits into an 'int',
I don't think so.
>
>> +struct logfs_disk_inode {
>> + be16 di_mode;
>> + be16 di_pad;
>> + be32 di_flags;
>> + be32 di_uid;
>> + be32 di_gid;
>> +
>> + be64 di_ctime;
>> + be64 di_mtime;
>> +
>> + be32 di_refcount;
>> + be32 di_generation;
>> + be64 di_used_bytes;
>> +
>> + be64 di_size;
>> + be64 di_data[LOGFS_EMBEDDED_FIELDS];
>> +}packed;
>> +
>> +
>> +#define LOGFS_MAX_NAMELEN 255
>
>Please put define on top
>
>> +struct logfs_disk_dentry {
>> + be64 ino; /* inode pointer */
>> + be16 namelen;
>> + u8 type;
>> + u8 name[LOGFS_MAX_NAMELEN];
>> +}packed;
>> +
>> +
>> +#define OBJ_TOP_JOURNAL 1 /* segment header for master journal */
>> +#define OBJ_JOURNAL 2 /* segment header for journal */
>> +#define OBJ_OSTORE 3 /* segment header for ostore */
>> +#define OBJ_BLOCK 4 /* data block */
>> +#define OBJ_INODE 5 /* inode */
>> +#define OBJ_DENTRY 6 /* dentry */
>
>enum please
>
>> +struct logfs_object_header {
>> + be32 crc; /* checksum */
>> + be16 len; /* length of object, header not included */
>> + u8 type; /* node type */
>> + u8 compr; /* compression type */
>> + be64 ino; /* inode number */
>> + be64 pos; /* file position */
>> +}packed;
>
>For all structs:
>
>Please use kernel doc struct comments.
>
>> +
>> +struct logfs_segment_header {
>> + be32 crc; /* checksum */
>> + be16 len; /* length of object, header not included */
>> + u8 type; /* node type */
>> + u8 level; /* GC level */
>> + be32 segno; /* segment number */
>> + be32 ec; /* erase count */
>> + be64 gec; /* global erase count (write time) */
>> +}packed;
>> +
>> +enum {
>> + COMPR_NONE = 0,
>> + COMPR_ZLIB = 1,
>> +};
>
>Please name the enums and use the same enum for the according fields and
>the function arguments.
>
>> +
>> +/* Journal entries come in groups of 16. First group contains individual
>> + * entries, next groups contain one entry per level */
>> +enum {
>> + JEG_BASE = 0,
>> + JE_FIRST = 1,
>> +
>> + JE_COMMIT = 1, /* commits all previous entries */
>> + JE_ABORT = 2, /* aborts all previous entries */
>> + JE_DYNSB = 3,
>> + JE_ANCHOR = 4,
>> + JE_ERASECOUNT = 5,
>> + JE_SPILLOUT = 6,
>> + JE_DELTA = 7,
>> + JE_BADSEGMENTS = 8,
>> + JE_AREAS = 9, /* area description sans wbuf */
>> + JEG_WBUF = 0x10, /* write buffer for segments */
>> +
>> + JE_LAST = 0x1f,
>> +};
>
>same here
>
>> +
>> +////////////////////////////////////////////////////////////////////////////////
>> +////////////////////////////////////////////////////////////////////////////////
>
>Eew.
>
>> +
>> +#define LOGFS_SUPER(sb) ((struct logfs_super*)(sb->s_fs_info))
>> +#define LOGFS_INODE(inode) container_of(inode, struct logfs_inode,
>> vfs_inode)
>
>lowercase inlines please
>
>> +
>> + /* 0 reserved for gc markers */
>> +#define LOGFS_INO_MASTER 1 /* inode file */
>> +#define LOGFS_INO_ROOT 2 /* root directory */
>> +#define LOGFS_INO_ATIME 4 /* atime for all inodes */
>> +#define LOGFS_INO_BAD_BLOCKS 5 /* bad blocks */
>> +#define LOGFS_INO_OBSOLETE 6 /* obsolete block count */
>> +#define LOGFS_INO_ERASE_COUNT 7 /* erase count */
>> +#define LOGFS_RESERVED_INOS 16
>
>enum ?
>
>> +struct logfs_super {
>> + //struct super_block *s_sb; /* should get removed... */
>
>Please do so
>
>> + be64 *s_rblock;
>> + be64 *s_wblock[LOGFS_MAX_LEVELS];
>
>Please comment the non obvious ones instead of the self explaining
>
>> + u64 s_free_bytes; /* number of free bytes */
>
>
>> +#define journal_for_each(__i) for (__i=0; __i<LOGFS_JOURNAL_SEGS; __i++)
>
> __i = 0; __i < LOGFS_JOURNAL_SEGS;
>
>> +void logfs_crash_dump(struct super_block *sb);
>> +#define LOGFS_BUG(sb) do { \
>> + struct super_block *__sb = sb; \
>
>Why do we need a local variable here ?
>
>> + logfs_crash_dump(__sb); \
>> + BUG(); \
>> +} while(0)
>
>> +static inline u8 logfs_type(struct inode *inode)
>> +{
>> + return (inode->i_mode >> 12) & 15;
>
>What's 12 and 15 ? Constants perhaps ?
12 bits, that's "07777" in octal, and means to get rid of the permissions
to get at the filetype. Though I am not sure if & 15 is still needed then.
>> +static int __logfs_readdir(struct file *file, void *buf, filldir_t filldir)
>> +{
>> + err = read_dir(dir, &dd, pos);
>> + if (err == -EOF)
>> + break;
>
> -EOF results in a return code 0 ?
Results in a return code -256.
>> +static int logfs_delete_dd(struct inode *dir, struct logfs_disk_dentry *dd,
>> + loff_t pos)
>> +{
>> + int err;
>> +
>> + err = read_dir(dir, dd, pos);
>> + if (err == -EOF) /* don't expose internal errnos */
>> + err = -EIO;
>
>Interesting. Why is EOF morphed to EIO ?
..and if that was right, why is not the same thing done above?
Jan
--
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
