lkml.org 
[lkml]   [2010]   [Jul]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    SubjectRe: [PATCH 02/18] xstat: Add a pair of system calls to make extended file stats available [ver #6]
    From
    On Wed, Jul 14, 2010 at 7:17 PM, David Howells <dhowells@redhat.com> wrote:
    >
    >        ssize_t ret = xstat(int dfd,
    >                            const char *filename,
    >                            unsigned flags,
    >                            const struct xstat_parameters *params,
    >                            struct xstat *buffer,
    >                            size_t buflen);

    Ugh. So I think this is pretty disgusting. For a few reasons:

    - that whole xstat buffer handling is just a mess. I think you
    already fixed the "xstat_parameters" crud and just made it a simple
    unsigned long and a direct argument, but the "buffer+buflen" thing is
    still disgusting.

    Why not just leave a few empty fields at the end, and make the rule
    be: "We don't just add random crap, so don't expect it to grow widely
    in the future".

    - you use "long long" all over the place. Don't do that. If you want
    a fixed size, say so, and use "u64/s64". That's the _real_ fixed size,
    and "long long" just _happens_ to be the same size on all current
    architectures.

    Put another way: "long" just _happened_ to be 32 bits way back when
    on pretty much all targets. That's where all the 64-bit compatibility
    mess came from. Don't make the same mistake. Besides, if the point is
    to make things be the same, _document_ that point by using a type that
    is explicitly sized.

    - why create that new kind of xstat() that realistically absolutely
    nobody will use outside of some very special cases, and that has no
    real advantages for 99.9% of all people?

    You could make it a "atomic stat+open" by replacing the useless
    "size" return value with a "fd" return value, add a flag saying "we're
    also interested in opening it" (in the same result set flags), and
    instead of that stupid "buflen" input, give the "mode" input that open
    needs.

    Tadaa! You now have something that more people might be interested
    in, if only because it avoids a system call and might be a performance
    win. Who knows. Ask the Wine people what strange
    open-function-from-hell they are interested in.

    >        ssize_t ret = fxstat(unsigned fd,

    Quite frankly, my gut feel is that once you do "xstat(dfd, filename,
    ...)" then it's damn stupid to do a separate "fxstat()", when you
    might as well say that "xtstat(dfd, NULL, ...)" is the same as
    "fxstat(fd, ...)"

    Now, the difference between adding one or two system calls may not be
    huge, but just from a cleanliness angle, I really don't see the point
    of having another fstat variant when the extended xstat() already very
    naturally supports the thing. And let's face it, using a NULL path
    pointer just makes sense if you don't have a path. You already passed
    it a target file descriptor in the dfd.

    Anyway, I didn't look at whether the new xstat fields made any sense,
    but I hated the interface enough that I can't be bothered to. Don't
    make up baroque new things that will never be used. Make a better
    argument for why anybody would use them despite the lack of
    standardization etc. And make sure they are as simple as possible
    (which is why I hate that "buflen" thing etc).

    Linus
    --
    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: 2010-07-19 17:21    [W:4.348 / U:0.072 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site