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 <p...@draigbrady.com> > > * 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.
