[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH] New fallocate module

From: Bruno Haible
Subject: Re: [PATCH] New fallocate module
Date: Thu, 28 May 2009 13:36:09 +0200
User-agent: KMail/1.9.9

Pádraig Brady wrote:
> Take 2 attached.

Take 2 review:


The generated fcntl.h should be maximally standalone. Can you better use
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
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

> +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.


reply via email to

[Prev in Thread] Current Thread [Next in Thread]