[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: Fri, 22 May 2009 13:18:41 +0200
User-agent: KMail/1.9.9

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.

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.

- 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 
  defines an indicator that users can test with #if or #ifdef.

  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.

- 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 
  into fcntl.in.h.

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

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

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

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


reply via email to

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