Re: [PATCH] New fallocate module

[Bruno Haible]

Pádraig Brady wrote:
> Take 2 attached.

Take 2 review:

> +#ifdef REPLACE_FALLOCATE

The generated fcntl.h should be maximally standalone. Can you better use
#if @REPLACE_FALLOCATE@ and use an AC_SUBSTed variable REPLACE_FALLOCATE
instead of one defined in config.h?

> +# undef fallocate

Why the undef? By default, we want to see warnings from gcc, and silence
them only when needed. If it is because of fallocate64 and LFS, please add
a comment.

> +#include <sys/types.h>

Includes belong at the top, before the 'extern "C" {'. On some systems,
in C++ mode, you cannot include system header files inside 'extern "C" { }'.

Documentation is missing. Can you add a reference to a well written
specification of this function, or otherwise describe
  - what are the arguments?
  - what does the function do?
  - what is the return value?

> +#include <errno.h>
> +static inline int fallocate (int fd, int mode, off_t offset, off_t len)

If you need extra include files, like <errno.h>, it's a sign that you better
create a file lib/fallocate.c now. You will need that anyway later for the
Solaris special-case implementation.

> +  return ENOSYS;

The return value convention described in
  http://www.kernel.org/doc/man-pages/online/pages/man2/fallocate.2.html
is a different one. Either this code or that man page is wrong.

> +#endif
> +
>  #ifdef __cplusplus
>  }
>  #endif

You are inserting this piece of code into the section marked
"Declare overridden functions". IMO it would be more logical near the end
of the file, around line 140.

> +      gl_cv_func_fallocate=yes, gl_cv_func_fallocate=no)])

In gnulib and coreutils, we've started on 2009-01-14 to improve the m4
macro argument quoting. This means, don't omit [ ] brackets as an
"optimization".

> +Description:
> +Ensure fallocate() is available

This is not a very informative description. How about:

fallocate() function: allocate disk space for a file.

> +errno

This dependency on errno is not needed, I think. See
doc/posix-headers/errno.texi for what the 'errno' module brings.

> +Include:
> +

Here you should note which include file the user should #include
in order to use the functionality.

Bruno