bug-gnulib
[Top][All Lists]
Advanced

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

Re: [PATCH] New fallocate module


From: Pádraig Brady
Subject: Re: [PATCH] New fallocate module
Date: Fri, 22 May 2009 12:37:41 +0100
User-agent: Thunderbird 2.0.0.6 (X11/20071008)

Bruno Haible wrote:
> Hello Pádraig,
> 
> The documentation files you sent document the status before any 'fallocate'
> module is added to gnulib. So I committed them for you:
> 
> 2009-05-21  Pádraig Brady  <address@hidden>
> 
>       * doc/glibc-functions/fallocate.texi: New file.
>       * doc/gnulib.texi: Include it.

Thanks a million for looking at this Bruno!

> The fallocate.texi will need a modification that goes along with the
> 'fallocate' module.
> 
>> This is my first gnulib module
> 
> Ok, here are detail comments:
> 
> - fallocate.c should have a GPLv3 copyright header.

oops

> - A rpl_fallocate function that always returns ENOSYS makes no sense for 
> gnulib.
>   In gnulib, we enable a function when we have working code for it. If the
>   replacement can be implemented only on some platforms, we make sure the .h 
> file
>   defines an indicator that users can test with #if or #ifdef.

Well libc, kernel or filesystem could return ENOSYS
so code using fallocate() has to handle it anyway.
I did it like that so gnulib users can call fallocate() unconditionally
for the common optional optimization use case.
Or they can also #if HAVE_FALLOCATE .... #endif if desired.

> 
>   In coreutils, the policy is different: coreutils at some places has dummy
>   stubs, and is willing to compile in function calls to these dummy stubs on
>   platforms that lack the particular functionality. Other projects that use
>   gnulib prefer to use a #if around the code that uses the functionality.
> 
> - In fallocate.c, the "#undef fallocate" should go away. It makes it 
> impossible
>   to make a namespace-clean library by adding a
>       #define fallocate libfoo_private_fallocate
>   to config.h.

ok

> - glibc declares fallocate() in <fcntl.h>. Therefore gnulib should do the 
> same.
>   There is no use in creating a file "fallocate.h"; instead, put the 
> declarations
>   into fcntl.in.h.

hmm. I was wondering about that. The reason I chose fallocate.h was
for platform independence. Also <linux/falloc.h> needs to be handled
independently then.

> - In fallocate.m4 you are doing
>     AC_DEFINE([fallocate], [rpl_fallocate], [replacement stub])
>   This define is better done in the .h file (fcntl.in.h in this case).
>   Otherwise you get problems when a platform has an 'fallocate' function.
>   We used that mix of config.h and *.in.h idiom in the beginning, but it
>   turned out to be more maintainable to put the entire replacement code
>   into the *.in.h file.

ok

> 
> - In fallocate.m4 the invocation of AC_TRY_LINK takes some time (it runs
>   the compiler and linker) and therefore should be protected by a cache
>   variable. AC_CACHE_CHECK is your friend.

ok

> 
>> Note also "fallocate()" functionality is available on
>> solaris using the fcntl(fd, F_ALLOCSP, ...) interface.
>> Hopefully this can be wrapped by this fallocate() at
>> some stage.
> 
> Yes, sure, please do this. F_ALLOCSP and F_ALLOCSP64. The more platforms
> a replacement supports, the better.

ok I'll try and get opensolaris to test this

>> Note fallocate() is unfortunately not available on 32 bit
>> fedora 11 at least when AC_SYS_LARGEFILE is used due to:
>> https://bugzilla.redhat.com/show_bug.cgi?id=500487
> 
> IMO this is not worth working around, because it's likely to be fixed
> quickly in glibc.

My thoughts also.

thanks again!
Pádraig.




reply via email to

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