Re: [PATCH] Trying to finalize loose ends of truncate.c fallocate

[Eric Blake]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

According to Matej Cepl on 2/27/2009 6:36 AM:
> * man/mkfile.8.xml: temporarily example of manpage for mkfile

We don't use .xml man pages.  Rather, with the combination of a template
man/mkfile.x and 'mkfile --help', one is generated by help2man as part of
man/Makefile.am.  You would need to rework this part of the patch to
follow that paradigm.

> * src/mkfile: script to use truncate to emulate mkfile(8)

Why mkfile(8)?  If we add this to coreutils, it seems like it should be in
section 1, not 8.

>  create mode 100644 man/mkfile.8.xml
>  create mode 100755 src/mkfile

Do you really want src/mkfile, or should it be src/mkfile.in in order to
reference @bindir@ and thus guarantee that you are invoking coreutils'
truncate (and not some other application by that name)?  Look back in git
history to when we had id as a shell script for an example.  For
prototyping, a script is okay, but eventually a conversion to C would be
easier to maintain in the coreutils framework (as happened to the id program).

> address@hidden -a
> address@hidden --fallocate
> address@hidden -a
> address@hidden --fallocate
> +When extending file use system call @command{fallocate}, which

s/fallocate/posix_\&/

> +ensures that disk space is allocated for @var{FILE}. After a successful

s/@var{FILE}/@var{file}/ (texinfo automatically upcases @var for info
pages, but you WANT lower case for dvi/pdf output)

> +call subsequent writes in the specified files are guaranteed

s/call/call,/

> +<?xml version="1.0" encoding="UTF-8"?>
> +<!DOCTYPE refentry PUBLIC "-//OASIS//DTD DocBook XML V4.1.2//EN"
> +                   
> "http://www.oasis-open.org/docbook/xml/4.1.2/docbookx.dtd";>
> +<!-- lifted from troff+man by doclifter -->
> +<refentry id='mkfile8'>
> +<!--  Copyright (c) 2007 -->
> +<!--         MATEJ Cepl.  All rights reserved. -->
> +<!--  Copyright (c) 2001, 2003, 2004 -->
> +<!--         HATANOU Tomomi.  All rights reserved. -->

In addition to my above comments about not using .xml, we would need to
reassign copyright to FSF to take this file.

> --- /dev/null
> +++ b/src/mkfile
> @@ -0,0 +1,81 @@
> +#!/bin/bash
> +# the original manpage is on
> +# http://developer.apple.com/documentation/Darwin/Reference\
> +# /ManPages/man8/mkfile.8.html
> +# and
> +# http://docs.sun.com/app/docs/doc/819-2240/6n4htdnbv?a=view
> +SPARSE=0
> +VERBOSE=0
> +OPTFLAGS=""
> +VERSION="1.0"
> +COMMANDNAME="$0"

Use lower-case variables for internal state, to avoid namespace collisions
with other programs.

> +
> +function usage() {
> +   cat - <<\EOF
> +Usage: mkfile OPTION... SIZE FILE

OPTION is optional; make this look like:

mkfile [OPTION]... SIZE FILE

> +Create a new FILE of the specified size (possibly with sparse file) that is
> +suitable for use as NFS-mounted swap areas, or as local swap areas.
> +
> +A FILE argument that does exist is overwritten.
> +
> +Mandatory arguments to long options are mandatory for short options too.
> +  -n create a sparse file (the size is noted, but disk blocks are not
> +      allocated until data is written to them)
> +  -v be verbose (report the size of the file created)
> +  -h display this help and exit

Long option synonyms for these short options?  --version?  And we prefer
to use ONLY the spelling --help (not -h) for help, leaving -h for other
purposes if we need future expansion.

> +
> +SIZE is a number which may be followed by one of the following suffixes:
> +KB 1000, K 1024, MB 1000*1000, M 1024*1024, and so on for G, T, P, E, Z, Y.
> +
> +SIZE may also be prefixed by one of the following modifying characters:
> +`+' extend by, `-' reduce by, `<' at most, `>' at least,
> +`/' round down to multiple of, `%' round up to multiple of.
> +
> +SEE ALSO
> +swapon(8)

Can you guarantee swapon will exist on all platforms where this mkfile
implementation works?

> +
> +HISTORY
> +A command first appeared in SunOS.

We tend not to use HISTORY sections in coreutils.

> +
> +Report truncate bugs to address@hidden
> +GNU coreutils home page: <http://www.gnu.org/software/coreutils/>
> +General help using GNU software: <http://www.gnu.org/gethelp/>
> +Report mkfile translation bugs to <http://translationproject.org/team/>
> +EOF
> +}
> +
> +while getopts ":vnh" OPTION ; do

getopts is not portable.  Again, refer to the old id script to see how to
do this portably.

> +    case $OPTION in
> +       v)
> +          VERBOSE=1
> +       ;;
> +       n)
> +          SPARSE=1
> +       ;;
> +       h)
> +          usage
> +          exit 0

You should not blindly exit 0; usage may have encountered a write error to
stdout, which must be diagnosed.  Again, the id script shows examples of
how painful it is to write portable shell script that still conforms to
GNU coding standards.

> +       ;;
> +       *)
> +          usage
> +          exit 1
> +       ;;
> +   esac
> +done
> +
> +if [ $SPARSE -eq 0 ] ; then
> +   OPTFLAGS=$OPTFLAGS"-a "
> +fi
> +
> +shift $(($OPTIND - 1))

$(()) is not portable.  See the autoconf manual for more details on
portable scripting.

> +SIZE="$1"

Double quotes aren't necessary when copying variables:

SIZE=$1

> +
> +shift 1

'shift n' is not portable, but since you are only shifting once, 'shift'
does the same thing.

> +FILE="$1"
> +
> +truncate $OPTFLAGS $SIZE $FILE

Here's where you need @bindir@, to guarantee you are picking up coreutils'
truncate.  Also, does truncate support --sparse yet?

> +RET=$?
> +
> +if [ $RET -eq 0 ] &&Â [ $VERBOSE -eq 1 ] ; then
> +   echo >&2 "Created file $FILE of the size "$(stat --format="%s" $FILE)"."
> +fi
> diff --git a/src/truncate.c b/src/truncate.c
> index c6f12b7..6eaeac0 100644
> --- a/src/truncate.c
> +++ b/src/truncate.c
> @@ -15,6 +15,7 @@
>     along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
>  
>  /* Written by Pádraig Brady
> +   Extended to use posix_fallocate by MatÄ?j Cepl <address@hidden>
>  
>     This is backwards compatible with the FreeBSD utility, but is more
>     flexible wrt the size specifications and the use of long options,
> @@ -50,10 +51,13 @@ static bool block_mode;
>  /* (-r) Reference file to use size from */
>  static char const *ref_file;
>  
> -static struct option const longopts[] =
> -{
> +/* (-a) Use fallocate to create a file */
> +static bool fallocate_mode;
> +
> +static struct option const longopts[] = {

Don't change formatting for no reason.  The { is on its own line for a reason.

>    {"no-create", no_argument, NULL, 'c'},
>    {"io-blocks", no_argument, NULL, 'o'},
> +  {"fallocate", no_argument, NULL, 'a'},
>    {"reference", required_argument, NULL, 'r'},
>    {"size", required_argument, NULL, 's'},
>    {GETOPT_HELP_OPTION_DECL},
> @@ -71,13 +75,12 @@ typedef enum
>     This supports dd BLOCK size suffixes + lowercase g,t,m for bsd compat
>     Note we don't support dd's b=512, c=1, w=2 or 21x512MiB formats.  */
>  static int
> -parse_len (char const *str, off_t *size)
> +parse_len (char const *str, off_t * size)

Don't change formatting for no reason.  Our coding style wants exactly
'off_t *size'.

>  {
>    enum strtol_error e;
>    intmax_t tmp_size;
>    e = xstrtoimax (str, NULL, 10, &tmp_size, "EgGkKmMPtTYZ0");
> -  if (e == LONGINT_OK
> -      && !(OFF_T_MIN <= tmp_size && tmp_size <= OFF_T_MAX))
> +  if (e == LONGINT_OK && !(OFF_T_MIN <= tmp_size && tmp_size <= OFF_T_MAX))

Don't change formatting for no reason.

>      e = LONGINT_OVERFLOW;
>  
>    if (e == LONGINT_OK)
> @@ -114,6 +117,9 @@ reads as zero bytes.\n\
>  Mandatory arguments to long options are mandatory for short options too.\n\
>  "), stdout);
>        fputs (_("\
> +  -a, --fallocate        when extending a file use fallocate.\n\

s/fallocate/posix_\&/

> +"), stdout);
> +      fputs (_("\
>    -c, --no-create        do not create any files\n\
>  "), stdout);
>        fputs (_("\
> @@ -161,8 +167,7 @@ do_ftruncate (int fd, char const *fname, off_t ssize, 
> rel_mode_t rel_mode)
>            error (0, 0,
>                   _("overflow in %" PRIdMAX
>                     " * %" PRIdMAX " byte blocks for file %s"),
> -                 (intmax_t) ssize, (intmax_t) blksize,
> -                 quote (fname));
> +                 (intmax_t) ssize, (intmax_t) blksize, quote (fname));

Don't reformat.

>            return 1;
>          }
>        ssize *= blksize;
> @@ -210,7 +215,7 @@ do_ftruncate (int fd, char const *fname, off_t ssize, 
> rel_mode_t rel_mode)
>          }
>        else
>          {
> -          if (ssize > OFF_T_MAX - (off_t)fsize)
> +          if (ssize > OFF_T_MAX - (off_t) fsize)

While this change is probably more in line with style we use elsewhere, it
still counts as a spurious reformat.

>              {
>                error (0, 0, _("overflow extending size of file %s"),
>                       quote (fname));
> @@ -224,6 +229,20 @@ do_ftruncate (int fd, char const *fname, off_t ssize, 
> rel_mode_t rel_mode)
>    if (nsize < 0)
>      nsize = 0;
>  
> +  /* posix_fallocate() is available since glibc 2.1.94.
> +   * fallocate is in Linux since kernel 2.6.23.

No need to comment here.  If we have a gnulib module for posix_fallocate,
then gnulib can blindly assume it works.  Other platforms offer it, so a
Linux-centric comment doesn't help.

> +   */
> +  if (fallocate_mode)
> +    {
> +      int ret = 0;
> +      if ((ret = posix_fallocate (fd, 0, nsize)) != 0)

Personally, I would split this:

ret = posix_fallocate (fd, 0, nsize);
if (ret)

> +        {
> +          error (0, ret, _("cannot fallocate size %d for file %s"), nsize,
> +                 quote (fname));
> +          return 1;

Would the gnulib implementation for platforms that lack posix_fallocate be
a no-op, or will it return ENOSYS, or will it perform a write() at the
appropriate offset to guarantee the disk space?

Actually, does using posix_fallocate even make much sense here?  My
understanding is that it guarantees the disk space so that subsequent
writes _to the same file descriptor_ will not fail, even when the current
file size is less than the allocated size; but since we close the file
descriptor when the process exits, and since we are changing the file size
to be the allocated size, are we really buying anything?  In other words,
I'm not sure that posix_fallocate does anything for cross-process
semantics - it seems like it is only useful within a single process.

> +        }
> +    }
> +
>    if (ftruncate (fd, nsize) == -1)      /* note updates mtime & ctime */
>      {
>        /* Complain only when ftruncate fails on a regular file, a
> @@ -255,7 +274,7 @@ int
>  main (int argc, char **argv)
>  {
>    bool got_size = false;
> -  off_t size IF_LINT (= 0);
> +  off_t size IF_LINT ( = 0);

More spurious reformatting.

>    rel_mode_t rel_mode = rm_abs;
>    mode_t omode;
>    int c, errors = 0, fd = -1, oflags;
> @@ -269,10 +288,14 @@ main (int argc, char **argv)
>  
>    atexit (close_stdout);
>  
> -  while ((c = getopt_long (argc, argv, "cor:s:", longopts, NULL)) != -1)
> +  while ((c = getopt_long (argc, argv, "acor:s:", longopts, NULL)) != -1)
>      {
>        switch (c)
>          {
> +        case 'a':
> +          fallocate_mode = true;
> +          break;
> +
>          case 'c':
>            no_create = true;
>            break;
> @@ -330,9 +353,9 @@ main (int argc, char **argv)
>            got_size = true;
>            break;
>  
> -        case_GETOPT_HELP_CHAR;
> +          case_GETOPT_HELP_CHAR;
>  
> -        case_GETOPT_VERSION_CHAR (PROGRAM_NAME, AUTHORS);
> +          case_GETOPT_VERSION_CHAR (PROGRAM_NAME, AUTHORS);

and again.

>  
>          default:
>            usage (EXIT_FAILURE);
> @@ -402,7 +425,6 @@ main (int argc, char **argv)
>            continue;
>          }
>  
> -

and again

>        if (fd != -1)
>          {
>            errors += do_ftruncate (fd, fname, size, rel_mode);
> diff --git a/tests/Makefile.am b/tests/Makefile.am
> index 07e9473..d6e6426 100644
> --- a/tests/Makefile.am
> +++ b/tests/Makefile.am
> @@ -231,6 +231,7 @@ TESTS =                                           \
>    misc/truncate-overflow                     \
>    misc/truncate-parameters                   \
>    misc/truncate-relative                     \
> +  misc/truncate-fallocate                    \
>    misc/tsort                                 \
>    misc/tty-eof                                       \
>    misc/unexpand                                      \
> diff --git a/tests/misc/truncate-fallocate b/tests/misc/truncate-fallocate
> new file mode 100755
> index 0000000..bce4083
> --- /dev/null
> +++ b/tests/misc/truncate-fallocate
> @@ -0,0 +1,57 @@
> +#!/bin/sh
> +
> +# Copyright (C) 2008 Free Software Foundation, Inc.

2009

- --
Don't work too hard, make some time for fun as well!

Eric Blake             address@hidden
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Public key at home.comcast.net/~ericblake/eblake.gpg
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iEYEARECAAYFAkmn9sIACgkQ84KuGfSFAYDrJwCfSK23fxEMpFjTFMpDE/Oxl8uB
5AwAn2pipBLPZ/cDeYELhzgx7n1a7ODG
=pEdU
-----END PGP SIGNATURE-----