> On Jan 19 2008 12:05, Miklos Szeredi wrote:
> >+
> >+/*
> >+ * Write full pathname from the root of the filesystem into the buffer.
> >+ */
> >+char *dentry_path(struct dentry *dentry, char *buf, int buflen)
>
> Hm, this functions looks very much like __d_path(). Is it
> an unintentional copy?
It is similar, but there is a significant difference: __d_path() shows
the path relative to the current process's root (possibly crossing
mount points), while dentry_path() shows the path relative to the
_filesystem's_ root (ignoring mount points, and current root).
> >@@ -601,28 +635,29 @@ static inline void mangle(struct seq_fil
> > seq_escape(m, s, " \t\n\\");
> > }
> >
> >+static struct proc_fs_info {
>
> These do not seem to be static data. Please mark them as const.
>
> static const struct proc_fs_info.
OK.
> >+ int flag;
> >+ char *str;
>
> const char *str.
OK.
> >+ seq_putc(m, ' ');
> >+ seq_puts(m, sb->s_flags & MS_RDONLY ? "ro" : "rw");
> >+ for (fs_infop = fs_info; fs_infop->flag; fs_infop++) {
> >+ if (sb->s_flags & fs_infop->flag)
> >+ seq_puts(m, fs_infop->str);
> >+ }
>
> {} could go.
The former is more readable, I think. My rule is: surround anything
with {} if it's more than one line, even if not strictly necessary.
This is especially true for nested if/else statements, but makes sense
for 'for' as well.
> >+ if (sb->s_op->show_options)
> >+ err = sb->s_op->show_options(m, mnt);
> >+ if (IS_MNT_SHARED(mnt)) {
> >+ seq_printf(m, " shared:%i", get_peer_group_id(mnt));
> >+ if (IS_MNT_SLAVE(mnt))
> >+ seq_printf(m, ",slave:%i", get_master_id(mnt));
> >+ } else if (IS_MNT_SLAVE(mnt)) {
> >+ seq_printf(m, " slave:%i", get_master_id(mnt));
> >+ } else if (IS_MNT_UNBINDABLE(mnt)) {
> >+ seq_printf(m, " unbindable");
> >+ } else {
> >+ seq_printf(m, " private");
> >+ }
> >+ seq_putc(m, '\n');
> >+ return err;
> >+}
> >+
> >+struct seq_operations mountinfo_op = {
> I think this can go const.
OK
> >--- linux.orig/include/linux/mount.h 2008-01-18 19:21:38.000000000 +0100
> >+++ linux/include/linux/mount.h 2008-01-18 23:39:35.000000000 +0100
> >@@ -56,6 +56,7 @@ struct vfsmount {
> > struct list_head mnt_slave; /* slave list entry */
> > struct vfsmount *mnt_master; /* slave is on master->mnt_slave_list */
> > struct mnt_namespace *mnt_ns; /* containing namespace */
> >+ int mnt_id; /* mount identifier */
> > /*
> > * We put mnt_count & mnt_expiry_mark at the end of struct vfsmount
> > * to let these frequently modified fields in a separate cache line
>
> Should/could it be unsigned int, or does it need to store -1?
IDA returns int, so why change it? Going over 2^31 number of mounts
doesn't seem likely in the near future.
Thanks,
Miklos
-
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
