[Ocfs2-tools-devel] [PATCH 2/6] extent_map: Add a parameter for ocfs2_extent_map_get_blocks.

[Mark Fasheh]

On Mon, Sep 24, 2007 at 02:24:07PM -0700, Joel Becker wrote:
> On Mon, Sep 24, 2007 at 05:19:53PM +0800, Tao Ma wrote:
> > extent_map: Add a parameter for ocfs2_extent_map_get_blocks.
> >  	    So the extent flags can be returned.
> 
> 	Wouldn't it be better to have a separate "get flags" API?  That
> is, you'd ask for the extent out of the map, but then you can also ask
> for the flags based on the extent data?

I think a seperate flags API for getting 8 bits of flag information which
are core to ocfs2_extent_rec is overkill. This is exactly the type of
information you'd want when looking up an extent.


> 	Heck, you could turn your current function (flags and blocks)
> into ocfs2_extent_map_get_full(). then do:
> 
> errcode_t ocfs2_extent_map_get_blocks(..., blkno, count
> {
> 	return ocfs2_extent_map_get_full(..., blkno, count, NULL);
> }
> errcode_t ocfs2_extent_map_get_flags(..., flags)
> {
> 	return ocfs2_extent_map_get_full(..., NULL, NULL, flags);
> }

Yuck - now the lower extent map code has to deal with block and count
potentially being NULL? That doesn't make sense to me. And you're forcing
code to make two calls just to get the whole idea of what an extent looks
like.


If having the extra parameter is a big deal, then we can do this:

errcode_t ocfs2_extent_map_get_blocks_full(ocfs2_cached_inode *cinode,
					   uint64_t v_blkno, int count,
					   uint64_t *p_blkno, uint64_t *ret_count,
					   int *ret_flags)
errcode_t ocfs2_extent_map_get_blocks(ocfs2_cached_inode *cinode,
				      uint64_t v_blkno, int count,
				      uint64_t *p_blkno, uint64_t
				      *ret_count)
{
	return ocfs2_extent_map_get_blocks_full(cinode, v_blkno, count,
						p_blkno, ret_count, NULL);
}

That way code which wants the flags field can call the lower function,
others can remain unchanged.
	--Mark

--
Mark Fasheh
Senior Software Developer, Oracle
mark.fasheh at oracle.com