[Top][All Lists]
[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.
- [PATCH] New fallocate module, Pádraig Brady, 2009/05/21
- Re: [PATCH] New fallocate module, Bruno Haible, 2009/05/22
- Re: [PATCH] New fallocate module,
Pádraig Brady <=
- Re: [PATCH] New fallocate module, Paul Eggert, 2009/05/22
- Re: [PATCH] New fallocate module, Pádraig Brady, 2009/05/22
- Re: [PATCH] New fallocate module, Pádraig Brady, 2009/05/27
- Re: [PATCH] New fallocate module, Eric Blake, 2009/05/28
- Re: [PATCH] New fallocate module, Pádraig Brady, 2009/05/28
- Re: [PATCH] New fallocate module, Bruno Haible, 2009/05/28
- Re: [PATCH] New fallocate module, Pádraig Brady, 2009/05/28
- Re: [PATCH] New fallocate module, Pádraig Brady, 2009/05/28