Re: [PATCH] Add support to check for FALLOC_FL_COLLAPSE_RANGE and FALLOC_FL_ZERO_RANGE crap modes

[Hugo Mills <hugo-lkml@carfax.org.uk>]

[-- Attachment #1: Type: text/plain, Size: 2316 bytes --]

On Thu, Jul 31, 2014 at 09:53:15PM -0400, Nick Krause wrote:
> On Thu, Jul 31, 2014 at 3:09 PM, Hugo Mills <hugo@carfax.org.uk> wrote:
> > On Thu, Jul 31, 2014 at 01:53:33PM -0400, Nicholas Krause wrote:
> >> This adds checks for the stated modes as if they are crap we will return error
> >> not supported.
> >
> >    You've just enabled two options, but you haven't actually
> > implemented the code behind it. I would tell you *NOT* to do anything
> > else on this work until you can answer the question: What happens if
> > you apply this patch, create a large file called "foo.txt", and then a
> > userspace program executes the following code?
> >
> > int fd = open("foo.txt", O_RDWR);
> > fallocate(fd, FALLOCATE_FL_COLLAPSE_RANGE, 50, 50);
> >
> >    Try it on a btrfs filesystem, both with and without your patch.
> > Also try it on an ext4 filesystem.
> >
> >    Once you've done all of that, reply to this mail and tell me what
> > the problem is with this patch. You need to make two answers: what are
> > the technical problems with the patch? What errors have you made in
> > the development process?
> >
> >    *Only* if you can answer those questions sensibly, should you write
> > any more patches, of any kind.
[snip]

> Calls are there in btrfs , therefore will either kernel panic or
> cause an oops.

   That's a guess. I can tell it's a guess, because I've actually read
(some of) the rest of that function, so I've got a good idea of what I
think it will do -- and panic or oops is not the answer. Try again.
You can answer this question two ways: by test (see my suggestion
above), or by reading and understanding the code. Either will work in
this case, but doing neither is not an option for someone who wants to
change the function.

> Need to test this patch as this is very easy to catch bug.

   So why didn't you? It's your patch, testing it is your job --
*before* it gets out into the outside world.

   Hugo.

-- 
=== Hugo Mills: hugo@... carfax.org.uk | darksatanic.net | lug.org.uk ===
  PGP key: 65E74AC0 from wwwkeys.eu.pgp.net or http://www.carfax.org.uk
  --- But people have always eaten people,  / what else is there to ---  
         eat?  / If the Juju had meant us not to eat people / he         
                     wouldn't have made us of meat.                      

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 811 bytes --]