Date: Mon, 31 Oct 2011 10:24:07 -0400 From: John Baldwin <jhb@freebsd.org> To: Jilles Tjoelker <jilles@stack.nl> Cc: arch@freebsd.org Subject: Re: [PATCH] fadvise(2) system call Message-ID: <201110311024.07580.jhb@freebsd.org> In-Reply-To: <20111029214057.GB90408@stack.nl> References: <201110281426.00013.jhb@freebsd.org> <20111029214057.GB90408@stack.nl>
next in thread | previous in thread | raw e-mail | index | archive | help
On Saturday, October 29, 2011 5:40:58 pm Jilles Tjoelker wrote:
> On Fri, Oct 28, 2011 at 02:25:59PM -0400, John Baldwin wrote:
> > I have been working for the last week or so on a patch to add an
> > fadvise(2) system call. It is somewhat similar to madvise(2) except
> > that it operates on a file descriptor instead of a memory region. It
> > also only really makes sense for regular files and does not apply to
> > other file descriptor types.
>
> Cool.
>
> Various other posix_* functions such as posix_spawn() and posix_openpt()
> are implemented directly, not as a wrapper around s/posix_//. I think
> posix_madvise() is only implemented as a wrapper because madvise()
> already existed. Therefore, I don't see a reason why a function named
> fadvise would be useful on its own (except if there are existing
> applications that use that name).
Existing applications use the name and I find it ugly. (I also wish we
had a plain fallocate() instead of just posix_fallocate().) However, if
other folks prefer not having the wrapper I could update it to use the
posix_* name.
> If the advice is FADV_SEQUENTIAL, FADV_RANDOM or FADV_NOREUSE and the
> file descriptor is invalid (fget() fails), the struct fadvise_info is
> leaked.
Gah, fixed (I had thought of that case but forgot to update it when
converting the advice to be malloc'd vs stored in struct file directly).
> The comparisons
>
> + (fa->fa_start != 0 && fa->fa_start == end + 1) ||
> + (uap->offset != 0 && fa->fa_end + 1 == uap->offset))) {
>
> should instead be something like
>
> + (end != OFF_MAX && fa->fa_start == end + 1) ||
> + (fa->fa_end != OFF_MAX && fa->fa_end + 1 == uap->offset))) {
>
> to avoid integer overflow.
Hmm, but the expressions will still work in that case, yes? I already
check for uap->offset and uap->len being negative earlier (so fa_start
and fa_end are always positive), and off_t is signed, so if end is
OFF_MAX, then end + 1 will certainly not == fa_start?
--
John Baldwin
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201110311024.07580.jhb>