Re: [PATCH] New fallocate module

[Pádraig Brady]

Bruno Haible wrote:
> 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?

I had done it like that but changed because of the existing FCHDIR_REPLACEMENT
macro in fcntl.in.h and the hacky way the replacement was done in modules/fcntl.
I'll change it back.

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

I'll remove undef

> 
>> +#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" { }'.

OK

> 
> 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?

I'll figure out where to add that.

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

:) OK
I guess this means we drop the inline.

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

I don't follow. Both return int. ENOSYS to me means it can't be done
because part of the stack doesn't support it. The filesystem is the part
documented as that will be the normal case going forward.

It's interesting to note, the man page you referenced describes the glibc
interface as I would expect it. I.E. no <linux/falloc.h> is required
for the FALLOC_FL_KEEP_SIZE define. I just updated my Fedora 11 system
to the latest glibc-2.10 and it still requires <linux/falloc.h>

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

Really? OK.

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

I'll change all the above also.

thanks again,
Pádraig.