Re: [PATCH 2/2 v2] debugfs: dump a sparse file as a new sparse file

[Theodore Ts'o <tytso@mit.edu>]

On Fri, Jan 04, 2013 at 12:05:05PM +0800, Zheng Liu wrote:
> >
> > I'm a bit concenred about this abstraction.  Consider what happens if
> > wanted is greater than a block size --- for example, consider if
> > wanted is 16k, and every other 1k block is uninitialized.
>
> Hi Ted,
>
> I wonder why wanted is 16k.  If a program calls ext2fs_file_read()
> function, seek will be 0 and SEEK flag won't be marked.  The behavior of
> ext2fs_file_read() is the same as before.  If ext2fs_file_read2() is
> called by dump_file(), seek won't be 0 and wanted is always equal to
> block size.  That is why I fix the hard-coded buffer length in dump_file().
> If I miss something, please let me know.

The problem is that ext2fs_file_read() is an exported function, and
there are users of this API/ABI outside of e2fsprogs.

The goal of this function is that it should look like the read system
call, and the caller might not know what the blocksize might be.  So
if the caller uses a 4k fixed size buffer, and the underlying file
system blocksize is 1k, this function needs to work properly.

So consider what happens if some program, perhaps an ext[234] FUSE
driver (there are two or three of them out there), or the e2tools
package, uses a 4k or 16k buffer --- this is legal, and they call the
existing ext2fs_file_read() library function.  In your patch,
ext2fs_file_read() will call ext2fs_file_read2(), and it will skip the
sparse blocks, and since the returned seek pointer is null, there's no
possible way for the caller of the ext2fs_file_read() would know this
had happened --- and even if there was a way, we don't ever change the
semantics/behaviour of an existing functional interface unless it's a
clear bug (and even then we need to think very carefully about the
backwards compatibility implications).

Regards,

                                                - Ted