lkml.org 
[lkml]   [2005]   [Aug]   [31]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH][FAT] FAT dirent scan with hin take #3
    Hi,

    Pekka Enberg wrote:
    > Hi,
    >
    > On 8/31/05, Machida, Hiroyuki <machida@sm.sony.co.jp> wrote:
    >
    >>+inline
    >>+static int hint_allocate(struct inode *dir)
    >>+{
    >>+ loff_t *hints;
    >>+ int err = 0;
    >>+
    >>+ if (!MSDOS_I(dir)->scan_hints) {
    >>+ hints = kcalloc(FAT_SCAN_NWAY, sizeof(loff_t), GFP_KERNEL);
    >>+ if (!hints)
    >>+ err = -ENOMEM;
    >
    >
    > Better to bail out here as...
    >
    >
    >>+
    >>+ down(&MSDOS_I(dir)->scan_lock);
    >>+ if (MSDOS_I(dir)->scan_hints)
    >>+ err = -EINVAL;
    >
    >
    > ...you might overwrite -ENOMEM here masking the real problem.
    >

    It's ok. If MSDOS_I(dir)->scan_hints isn't NULL, it's not error case.
    We need just kfree(hints) and return 0.(Assuming kfree() accepts NULL)
    I think EINVAL confuse you.
    >
    >>+ if (!err)
    >>+ MSDOS_I(dir)->scan_hints = hints;
    >>+ up(&MSDOS_I(dir)->scan_lock);
    >>+ if (err == -EINVAL) {
    >
    >
    > Gotos would make error handling much cleaner.
    >
    How about this ?

    if (!MSDOS_I(dir)->scan_hints) {
    hints = kcalllo(....);

    down
    if (MSDOS_I(dir)->scan_hints) {
    up
    goto already_allocated;
    }
    if (hints)
    MSDOS_I(dir)->scan_hints = hints;
    up
    }
    return (hints == 0) ? -ENOMEM : 0;

    already_allocated:
    kfree(hints); /* kfree accepts NULL */
    return 0;



    >
    >>+inline
    >>+static int hint_index_body(const unsigned char *name, int name_len, int check_null)
    >
    >
    > Please consider calling this __hint_index() instead as per normal
    > naming conventions.

    Agree.

    >
    >>+{
    >>+ int i;
    >>+ int val = 0;
    >>+ unsigned char *p = (unsigned char *) name;
    >>+ int id = current->pid;
    >>+
    >>+ for (i=0; i<name_len; i++) {
    >>+ if (check_null && !*p) break;
    >
    >
    > Please put break on separate line. You still have quite a few of these.

    Agree

    >
    >
    >>+ val = ((val << 1) & 0xfe) | ((val & 0x80) ? 1 : 0);
    >>+ val ^= *p;
    >>+ p ++;
    >>+ }
    >>+ id = ((id >> 8) & 0xf) ^ (id & 0xf);
    >>+ val = (val << 1) | (id & 1);
    >>+ return val & (FAT_SCAN_NWAY-1);
    >>+}
    >
    >
    > Pekka


    --
    Hiroyuki Machida machida@sm.sony.co.jp
    SSW Dept. HENC, Sony Corp.
    -
    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: 2005-08-31 15:00    [W:0.028 / U:0.452 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site