I see that makes sense. In that case, I'll rename the function to
errcode_t ext2fs_decode_extent(struct ext2fs_extent *dst, void *src).
I wonder if it's okay if we make this function return an error in case
the on-disk format is not sane. If we do that, we can add
ext2fs_validate_extent() later. Does that make sense?
On Wed, Dec 2, 2020 at 10:45 AM Theodore Y. Ts'o <tytso@xxxxxxx> wrote:
>
> On Fri, Nov 20, 2020 at 11:15:57AM -0800, Harshad Shirwadkar wrote:
> > This patch adds the following new APIs:
> >
> > Count the total number of blocks occupied by inode including
> > intermediate extent tree nodes.
> > extern blk64_t ext2fs_count_blocks(ext2_filsys fs, ext2_ino_t ino,
> > struct ext2_inode *inode);
> >
> > Convert ext3_extent to ext2fs_extent.
> > extern void ext2fs_convert_extent(struct ext2fs_extent *to,
> > struct ext3_extent *from);
>
> So one of the reasons why I've intentionally never exposed "struct
> ext3_extent" in the libext2fs interface is because that's an on-disk
> structure which I keep hoping we might change someday --- for example,
> to allow for 64-bit logical block numbers so we can create ext4 files
> greater than 2**32 blocks. It might be that some other future
> enhancement, such as say, reflinks (depending on how we implement
> them), or reverse pointers, might also require making changes to the
> on-disk format.
>
> The kernel code has the on-disk format and the various logical
> manipulations of the extent tree hopelessly entangled with each other,
> which means changing the kernel code to support more than one on-disk
> extent structure is going to be **hard**. But in the userspace code,
> all of the knowledge about the on-disk structure is abstracted away
> inside lib/ext2fs/extent.c.
>
> It may very well be that for fast commit, we're going to need to crack
> open that abstraction barrier a bit. But let's make sure the function
> name makes it clear that what we are doing here is converting between
> a particular on-disk encoding and the ext2fs abtract extent type.
> "ext2fs_convert_extent" doesn't exactly make this clear.
>
> It might also be that what should do is include a pointer to the fs
> and inode structures, and call this something like
> "ext2fs_{decode,encode}_extent()", and pass in the on-disk format via
> a void *. We might also want to have some kind of
> ext2fs_validate_extent() function which takes a void * and validates
> the on-disk structure to make sure it's sane.
>
> What do you think?
>
> - Ted
