Re: [PATCH 1/2] [v3] filetype: Improve FAT detection

[Sascha Hauer <s.hauer@pengutronix.de>]

On Tue, Sep 18, 2012 at 11:51:35PM +0200, Franck Jullien wrote:
> We may have some disk with MBR as a first sector. In this case, the
> current FAT check returns an error. However, the FAT sector exist and
> the MBR can tell us where it is.
> 
> This patch add to file_name_detect_type function the ability to find
> the FAT boot sector on the first sector of the first partition in case
> it is not on sector 0.
> 
> It also introduce is_fat_or_mbr to check if a buffer is a FAT boot
> or MBR sector
> 
> Signed-off-by: Franck Jullien <franck.jullien@gmail.com>
> ---
>  common/filetype.c  |   63 ++++++++++++++++++++++++++++++++++++++++++----------
>  include/filetype.h |    2 +
>  2 files changed, 53 insertions(+), 12 deletions(-)
> 
> diff --git a/common/filetype.c b/common/filetype.c
> index e736d43..cb4db5c 100644
> --- a/common/filetype.c
> +++ b/common/filetype.c
> @@ -26,6 +26,8 @@
>  #include <fcntl.h>
>  #include <fs.h>
>  #include <malloc.h>
> +#include <fat.h>
> +#include <errno.h>
>  
>  static const char *filetype_str[] = {
>  	[filetype_unknown] = "unknown",
> @@ -42,6 +44,7 @@ static const char *filetype_str[] = {
>  	[filetype_sh] = "Bourne Shell",
>  	[filetype_mips_barebox] = "MIPS barebox image",
>  	[filetype_fat] = "FAT filesytem",
> +	[filetype_mbr] = "MBR sector",
>  };
>  
>  const char *file_type_to_string(enum filetype f)
> @@ -52,26 +55,42 @@ const char *file_type_to_string(enum filetype f)
>  	return NULL;
>  }
>  
> -static int is_fat(u8 *buf)
> +#define MBR_StartSector		8	/* MBR: Offset of Starting Sector in Partition Table Entry */
> +#define BS_55AA			510	/* Boot sector signature (2) */
> +#define MBR_Table		446	/* MBR: Partition table offset (2) */
> +#define BS_FilSysType32		82	/* File system type (1) */
> +#define BS_FilSysType		54	/* File system type (1) */
> +
> +enum filetype is_fat_or_mbr(unsigned char *sector, unsigned long *bootsec)

Could you add a comment describing the meaning of the bootsec argument?
Also, please make the sector argument 'const'. (I'll send a patch
making the other functions take a const argument where appropriate)

>  {
> -	if (get_unaligned_le16(&buf[510]) != 0xAA55)
> -		return 0;
> +	if(bootsec)

whitespace after 'if' please.

> +		*bootsec = 0;
> +
> +	/* Check record signature (always placed at offset 510 even if the sector size is>512) */
> +	if (get_unaligned_le16(&sector[BS_55AA]) != 0xAA55)
> +		return filetype_unknown;
> +
> +	/* Check "FAT" string */
> +	if ((get_unaligned_le32(&sector[BS_FilSysType]) & 0xFFFFFF) == 0x544146)
> +		return filetype_fat;
>  
> -	/* FAT */
> -	if ((get_unaligned_le32(&buf[54]) & 0xFFFFFF) == 0x544146)
> -		return 1;
> +	if ((get_unaligned_le32(&sector[BS_FilSysType32]) & 0xFFFFFF) == 0x544146)
> +		return filetype_fat;
>  
> -	/* FAT32 */
> -	if ((get_unaligned_le32(&buf[82]) & 0xFFFFFF) == 0x544146)
> -		return 1;
> +	if(bootsec)
> +		/* This must be an MBR, so return the starting sector of the
> +		 * first partition so we could check if there is a FAT boot
> +		 * sector there */
> +		*bootsec = get_unaligned_le16(&sector[MBR_Table + MBR_StartSector]);
>  
> -	return 0;
> +	return filetype_mbr;
>  }
>  
>  enum filetype file_detect_type(void *_buf)
>  {
>  	u32 *buf = _buf;
>  	u8 *buf8 = _buf;
> +	enum filetype type;
>  
>  	if (strncmp(buf8, "#!/bin/sh", 9) == 0)
>  		return filetype_sh;
> @@ -99,8 +118,10 @@ enum filetype file_detect_type(void *_buf)
>  		return filetype_aimage;
>  	if (strncmp(buf8 + 0x10, "barebox", 7) == 0)
>  		return filetype_mips_barebox;
> -	if (is_fat(buf8))
> -		return filetype_fat;
> +
> +	type = is_fat_or_mbr(buf8, NULL);
> +	if (type != filetype_unknown)
> +		return type;
>  
>  	return filetype_unknown;
>  }
> @@ -110,6 +131,7 @@ enum filetype file_name_detect_type(const char *filename)
>  	int fd, ret;
>  	void *buf;
>  	enum filetype type = filetype_unknown;
> +	unsigned long bootsec;
>  
>  	fd = open(filename, O_RDONLY);
>  	if (fd < 0)
> @@ -123,6 +145,23 @@ enum filetype file_name_detect_type(const char *filename)
>  
>  	type = file_detect_type(buf);
>  
> +	if (type == filetype_mbr) {
> +		/* Get the first partition start sector
> +		 * and check for FAT in it */
> +		is_fat_or_mbr((u8 *)buf, &bootsec);

Unnecessary cast.

> +		ret = lseek(fd, (bootsec) * 512, SEEK_SET);
> +		if (ret < 0) {
> +			type = filetype_unknown;

It's still an mbr in this case.

> +			goto err_out;
> +		}
> +		ret = read(fd, buf, 512);
> +		if (ret < 0) {
> +			type = filetype_unknown;

ditto.

Sascha


-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox