linux-ext4 - Re: [PATCH] fiemap support for ext3

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-id: <20080423232725.GK3095@webber.adilger.int>
Date:	Wed, 23 Apr 2008 17:27:25 -0600
From:	Andreas Dilger <adilger@....com>
To:	Josef Bacik <jbacik@...hat.com>
Cc:	linux-ext4@...r.kernel.org, sandeen@...hat.com
Subject: Re: [PATCH] fiemap support for ext3

On Apr 23, 2008  15:39 -0400, Josef Bacik wrote:
> Here is my reworked fiemap patch for ext3.  The generic fiemap handler takes the
> get_block_t from the filesystem and does the fiemap that way, so then adding
> ext2 support should be a snap, as well as any other fs that wants to use this.

The generic mechanism is really good, some minor issues still exist.

> +int generic_block_fiemap(struct inode *inode, unsigned long arg,
> +			 get_block_t *get_block)
> +{
> +	/*
> +	 * if fm_start is in the middle of the current block, get the next
> +	 * block so we don't end up returning a start thats before the given
> +	 * fm_start
> +	 */
> +	start_blk = (fiemap_s->fm_start + (1 << inode->i_blkbits) - 1) >>
> +		inode->i_blkbits;

This should return the start of the current block (so the requested fm_start
is included in the returned mapping), instead of the start of the next block.

> +	/*
> +	 * we want the comparisons to be unsigned, in case somebody passes -1,
> +	 * meaning they want they want the entire file, but the result has to be
> +	 * signed so we can handle the case where we get more blocks than the
> +	 * size of the file
> +	 */
> +	length = (long)min((unsigned long)fiemap_s->fm_length,
> +			   (unsigned long)i_size_read(inode));

This might be written as:

	length = (long)min_t(unsigned long,fiemap_s->fm_len,i_size_read(inode));

Also, what about files that have blocks mapped after i_size?

> +		if (!tmp.b_blocknr) {
> +
> +			if (!hole) {

No empty line above "if (!hole)".

> +			fiemap_e.fe_offset = tmp.b_blocknr <<
> +				inode->i_blkbits;

This doesn't need to wrap.

> +			fiemap_e.fe_length = tmp.b_size;
> +
> +			if (length <= 0)
> +				fiemap_e.fe_flags |= FIEMAP_EXTENT_LAST;

Should the exit condition should be the first hole (block == 0) after i_size?

Cheers, Andreas
--
Andreas Dilger
Sr. Staff Engineer, Lustre Group
Sun Microsystems of Canada, Inc.

--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html