fallocate: --punch option parsing error diagnostics irritating

fallocate: --punch option parsing error diagnostics irritating

[Bernhard Voelker]

<naive_user>

Let's create a file:

  $ dd if=/dev/zero of=/tmp/x bs=1000000 count=1 status=none

  $ ls -ldog /tmp/x
  -rw-r--r-- 1 1000000 Jun  6 08:32 /tmp/x

Now, somehow I want to punch holes into it, use -p:

  $ ./fallocate -p /tmp/x
  fallocate: no length argument specified

Well, okay then try with -l:

  $ ./fallocate -p -l 10000 /tmp/x
  fallocate: only -n mode can be used with --zero-range

Huh? I didn't specify neither -n nor -z.

Well, let's try -n then:

  $ ./fallocate -p -n /tmp/x
  fallocate: no length argument specified

Reading 'man fallocate' regarding --punch doesn't help either:

  -p, --punch-hole
        Punch holes in the file, the range should not
        exceed the length of the file.

What does "punching" mean? Please explain without using the
word "punch".
And how would one specify a "range"? There's no option with
such a "range" argument.
Finally, please add some examples to the man page.

Thanks!

</naive_user>

Re: fallocate: --punch option parsing error diagnostics irritating

[Dale R. Worley]

> From: Bernhard Voelker <mail@bernhard-voelker.de>

> What does "punching" mean? Please explain without using the
> word "punch".

It means to set the contents of a range of bytes in the file to zero,
and then to de-allocate the disk pages that record that range of
bytes, leaving a "hole" -- a region of the file that exists as far as
read() calls is concerned, but does not occupy actual disk pages.  Of
necessity, the bytes in a hole must be zero.

> And how would one specify a "range"? There's no option with
> such a "range" argument.

Ugh, that's badly written.  Without -p, fallocate forces the system to
allocate blocks to a file which will appear to have zero contents.
But for some filesystems, the disk blocks don't actually have to be
set to zero, there's a way to mark them as "needs to be set to zero
before the user can read it".  The part of the file that is allocated
in this way starts at "offset" bytes and ends just before
"offset+length" bytes.  That's the "range".

With -p, the specified region of the file is also effectively set to
zeros, but the disk blocks aren't allocated.  And it appears that if
the blocks are allocated now, they are deallocated. 

Dale

Re: fallocate: --punch option parsing error diagnostics irritating

[Karel Zak]

On Wed, Jun 25, 2014 at 10:16:54PM +0200, Bernhard Voelker wrote:
>   $ ./fallocate -p -l 10000 /tmp/x
>   fallocate: only -n mode can be used with --zero-range
> 
> Huh? I didn't specify neither -n nor -z.

		case 'p':
			mode |= FALLOC_FL_PUNCH_HOLE;
			/* fall through */
		case 'n':
			mode |= FALLOC_FL_KEEP_SIZE;
			break;


unfortunately there is nothing about it in man page and usage(). It's
described in fallocate(2) syscall man page only.

Anyway, the code is wrong, it assumes that KEEP_SIZE is possible to use
only with ZERO_RANGE. 

Fixed in git tree, man page massively modified to be readable for
humans (original text in the man page was from FS devels:-)

> Finally, please add some examples to the man page.

I think the new version of the man page is good enough without examples, but 
if someone has something nice we can add EXAMPLES section.

Thanks!

    Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

Re: fallocate: --punch option parsing error diagnostics irritating

[Pádraig Brady]

On 06/26/2014 11:10 AM, Karel Zak wrote:
> On Wed, Jun 25, 2014 at 10:16:54PM +0200, Bernhard Voelker wrote:
>>   $ ./fallocate -p -l 10000 /tmp/x
>>   fallocate: only -n mode can be used with --zero-range
>>
>> Huh? I didn't specify neither -n nor -z.
> 
> 		case 'p':
> 			mode |= FALLOC_FL_PUNCH_HOLE;
> 			/* fall through */
> 		case 'n':
> 			mode |= FALLOC_FL_KEEP_SIZE;
> 			break;
> 
> 
> unfortunately there is nothing about it in man page and usage(). It's
> described in fallocate(2) syscall man page only.
> 
> Anyway, the code is wrong, it assumes that KEEP_SIZE is possible to use
> only with ZERO_RANGE. 
> 
> Fixed in git tree, man page massively modified to be readable for
> humans (original text in the man page was from FS devels:-)
> 
>> Finally, please add some examples to the man page.
> 
> I think the new version of the man page is good enough without examples, but 
> if someone has something nice we can add EXAMPLES section.

--help could he clarified:

diff --git a/sys-utils/fallocate.c b/sys-utils/fallocate.c
index d950f9c..6192f0a 100644
--- a/sys-utils/fallocate.c
+++ b/sys-utils/fallocate.c
@@ -75,13 +75,13 @@ static void __attribute__((__noreturn__)) usage(FILE *out)
        fprintf(out,
              _(" %s [options] <filename>\n"), program_invocation_short_name);
        fputs(USAGE_OPTIONS, out);
-       fputs(_(" -c, --collapse-range collapse space in the file\n"), out);
-       fputs(_(" -d, --dig-holes      detect and dig holes\n"), out);
-       fputs(_(" -l, --length <num>   length of the (de)allocation, in bytes\n"), out);
-       fputs(_(" -n, --keep-size      don't modify the length of the file\n"), out);
-       fputs(_(" -o, --offset <num>   offset of the (de)allocation, in bytes\n"), out);
-       fputs(_(" -p, --punch-hole     punch holes in the file (implies --keep-size)\n"), out);
-       fputs(_(" -z, --zero-range     zeroes a range in the file\n"), out);
+       fputs(_(" -c, --collapse-range remove a range from the file\n"), out);
+       fputs(_(" -d, --dig-holes      detect zeroes and replace with holes\n"), out);
+       fputs(_(" -l, --length <num>   length for range operations, in bytes\n"), out);
+       fputs(_(" -n, --keep-size      maintain the apparent size of the file\n"), out);
+       fputs(_(" -o, --offset <num>   offset for range operations, in bytes\n"), out);
+       fputs(_(" -p, --punch-hole     replace a range with a hole (implies -n)\n"), out);
+       fputs(_(" -z, --zero-range     zero and ensure allocation of a range\n"), out);
        fputs(_(" -v, --verbose        verbose mode\n"), out);

        fputs(USAGE_SEPARATOR, out);

Re: fallocate: --punch option parsing error diagnostics irritating

[Karel Zak]

On Thu, Jun 26, 2014 at 11:52:10AM +0100, Pádraig Brady wrote:
> --help could he clarified:
> 
> diff --git a/sys-utils/fallocate.c b/sys-utils/fallocate.c
> index d950f9c..6192f0a 100644

 Applied, thanks.

    Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

Re: fallocate: --punch option parsing error diagnostics irritating

[Bernhard Voelker]

On 06/26/2014 01:07 PM, Karel Zak wrote:
>   Applied, thanks.

Thanks!

Now, the optind check is too late in those 2 invalid cases:

   $ ./fallocate
   fallocate: no length argument specified

   $ ./fallocate file1 file2
   fallocate: no length argument specified

The patch below fixes it.

BTW: I think fallocate(1) shouldn't open(...,O_CREAT)
with -d, -p, and maybe with -z, should it?

Have a nice day,
Berny

 From d4b880a188517e12aead021bcee01630e2893e03 Mon Sep 17 00:00:00 2001
From: Bernhard Voelker <mail@bernhard-voelker.de>
Date: Thu, 26 Jun 2014 14:09:47 +0200
Subject: [PATCH] fallocate: fix check of number of arguments

Signed-off-by: Bernhard Voelker <mail@bernhard-voelker.de>
---
  sys-utils/fallocate.c | 18 +++++++++---------
  1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/sys-utils/fallocate.c b/sys-utils/fallocate.c
index 91dee41..5abe3eb 100644
--- a/sys-utils/fallocate.c
+++ b/sys-utils/fallocate.c
@@ -339,6 +339,15 @@ int main(int argc, char **argv)
  			break;
  		}
  	}
+
+	if (optind == argc)
+		errx(EXIT_FAILURE, _("no filename specified"));
+
+	filename = argv[optind++];
+
+	if (optind != argc)
+		errx(EXIT_FAILURE, _("unexpected number of arguments"));
+
  	if (dig) {
  		/* for --dig-holes the default is analyze all file */
  		if (length == -2LL)
@@ -354,15 +363,6 @@ int main(int argc, char **argv)
  	}
  	if (offset < 0)
  		errx(EXIT_FAILURE, _("invalid offset value specified"));
-	if (optind == argc)
-		errx(EXIT_FAILURE, _("no filename specified."));
-
-	filename = argv[optind++];
-
-	if (optind != argc) {
-		warnx(_("unexpected number of arguments"));
-		usage(stderr);
-	}

  	fd = open(filename, O_RDWR|O_CREAT, 0644);
  	if (fd < 0)
-- 
1.8.4.2

Re: fallocate: --punch option parsing error diagnostics irritating

[Karel Zak]

On Thu, Jun 26, 2014 at 02:14:01PM +0200, Bernhard Voelker wrote:
> The patch below fixes it.

 Applied, thanks.

> BTW: I think fallocate(1) shouldn't open(...,O_CREAT)
> with -d, -p, and maybe with -z, should it?

Good catch, I think O_CREAT makes sense only for the default
fallocate(2) behavior when no mode is specified and new file is
allocated. All another modes work with existing data/holes. 

Fixed.

    Karel


PS. I love this on-line development and brain storming, git & OSS is
    the best :-)

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com