lkml.org 
[lkml]   [2007]   [May]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 1/5] fallocate() implementation in i86, x86_64 and powerpc
    On Thu, 26 Apr 2007 23:33:32 +0530 "Amit K. Arora" <aarora@linux.vnet.ibm.com> wrote:

    > This patch implements the fallocate() system call and adds support for
    > i386, x86_64 and powerpc.
    >
    > ...
    >
    > +asmlinkage long sys_fallocate(int fd, int mode, loff_t offset, loff_t len)

    Please add a comment over this function which specifies its behaviour.
    Really it should be enough material from which a full manpage can be
    written.

    If that's all too much, this material should at least be spelled out in the
    changelog. Because there's no way in which this change can be fully
    reviewed unless someone (ie: you) tells us what it is setting out to
    achieve.

    If we 100% implement some standard then a URL for what we claim to
    implement would suffice. Given that we're at least using different types from
    posix I doubt if such a thing would be sufficient.

    And given the complexity and potential variability within the filesystem
    implementations of this, I'd expect that _something_ additional needs to be
    said?

    > +{
    > + struct file *file;
    > + struct inode *inode;
    > + long ret = -EINVAL;
    > +
    > + if (len == 0 || offset < 0)
    > + goto out;

    The posix spec implies that negative `len' is permitted - presumably "allocate
    ahead of `offset'". How peculiar.

    > + ret = -EBADF;
    > + file = fget(fd);
    > + if (!file)
    > + goto out;
    > + if (!(file->f_mode & FMODE_WRITE))
    > + goto out_fput;
    > +
    > + inode = file->f_path.dentry->d_inode;
    > +
    > + ret = -ESPIPE;
    > + if (S_ISFIFO(inode->i_mode))
    > + goto out_fput;
    > +
    > + ret = -ENODEV;
    > + if (!S_ISREG(inode->i_mode))
    > + goto out_fput;

    So we return ENODEV against an S_ISBLK fd, as per the posix spec. That
    seems a bit silly of them.

    > + ret = -EFBIG;
    > + if (offset + len > inode->i_sb->s_maxbytes)
    > + goto out_fput;

    This code does handle offset+len going negative, but only by accident, I
    suspect. It happens that s_maxbytes has unsigned type. Perhaps a comment
    here would settle the reader's mind.

    > + if (inode->i_op && inode->i_op->fallocate)
    > + ret = inode->i_op->fallocate(inode, mode, offset, len);
    > + else
    > + ret = -ENOSYS;

    If we _are_ going to support negative `len', as posix suggests, I think we
    should perform the appropriate sanity conversions to `offset' and `len'
    right here, rather than expecting each filesystem to do it.

    If we're not going to handle negative `len' then we should check for it.

    > +out_fput:
    > + fput(file);
    > +out:
    > + return ret;
    > +}
    > +EXPORT_SYMBOL(sys_fallocate);

    I don't believe this needs to be exported to modules?

    > +/*
    > + * fallocate() modes
    > + */
    > +#define FA_ALLOCATE 0x1
    > +#define FA_DEALLOCATE 0x2

    Now those aren't in posix. They should be documented, along with their
    expected semantics.

    > #ifdef __KERNEL__
    >
    > #include <linux/linkage.h>
    > @@ -1125,6 +1131,7 @@ struct inode_operations {
    > ssize_t (*listxattr) (struct dentry *, char *, size_t);
    > int (*removexattr) (struct dentry *, const char *);
    > void (*truncate_range)(struct inode *, loff_t, loff_t);
    > + long (*fallocate)(struct inode *, int, loff_t, loff_t);

    I really do think it's better to put the variable names in definitions such
    as this. Especially when we have two identically-typed variables next to
    each other like that. Quick: which one is the offset and which is the
    length?


    -
    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: 2007-05-04 06:35    [W:0.031 / U:30.020 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site