[U-Boot-Users] ROMFS

[Grant Likely]

On 6/20/07, Michal Simek <monstr at seznam.cz> wrote:
> ROMFS: Add support for ROMFS filesystem based on MTD system.
>
> signed-off-by: Michal Simek <monstr at monstr.eu>
>
> Patch is in attachement.

Please send your patches inline.  It is harder to comment on patches
that are attachments because I cannot hit 'reply' and start typing.
Patches that don't get reviewed are harder to get merged.
(git-send-email is your friend here)

I needed to cut and paste to write this email, but here are some comments.

Also, it's looking more and more like there needs to be an abstraction
between filesystems and block devices.  (More of a general comment
than a comment on your patch)  Overriding cmd_jffs2 to do more and
more filesystems just smells wrong.

Otherwise, it's a good looking driver.

Cheers,
g.

>
> Best regards,
> Michal Simek

> diff --git a/common/cmd_jffs2.c b/common/cmd_jffs2.c
> index 7fd1fa3..92bd898 100644
> --- a/common/cmd_jffs2.c
> +++ b/common/cmd_jffs2.c
> @@ -85,7 +85,7 @@
>   */
>
>  /*
> - * JFFS2/CRAMFS support
> + * JFFS2/CRAMFS/ROMFS support

Hmmm, perhaps the filename cmd_jffs2.c needs to be changed.

>   */
>  #include <common.h>
>  #include <command.h>
> @@ -175,6 +175,11 @@ extern int cramfs_load (char *loadoffset, struct part_info *info, char *filename
>  extern int cramfs_ls (struct part_info *info, char *filename);
>  extern int cramfs_info (struct part_info *info);
>
> +extern int romfs_check (struct part_info *info);
> +extern int romfs_load (char *loadoffset, struct part_info *info, char *filename);
> +extern int romfs_ls (struct part_info *info, char *filename);
> +extern int romfs_info (struct part_info *info);
> +
>  static struct part_info* jffs2_part_info(struct mtd_device *dev, unsigned int part_num);
>
>  /* command line only routines */
> @@ -1874,14 +1879,22 @@ int do_jffs2_fsload(cmd_tbl_t *cmdtp, int flag, int argc, char *argv[])
>
>  	if ((part = jffs2_part_info(current_dev, current_partnum))){
>
> -		/* check partition type for cramfs */
> -		fsname = (cramfs_check(part) ? "CRAMFS" : "JFFS2");
> +		/* check partition type for JFFS2, cramfs, romfs  */
> +		if (cramfs_check(part)) {
> +                 	fsname = "CRAMFS";
> +		} else if (romfs_check(part)) {
> +			fsname = "ROMFS";
> +  		} else {
> +            		fsname = "JFFS2";
> +    		}
>  		printf("### %s loading '%s' to 0x%lx\n", fsname, filename, offset);
>
>  		if (cramfs_check(part)) {
>  			size = cramfs_load ((char *) offset, part, filename);
> +		} else if (romfs_check(part)){
> +			size = romfs_load ((char *) offset, part, filename);
>  		} else {
> -			/* if this is not cramfs assume jffs2 */
> +			/* if this is not cramfs or romfs assume jffs2 */
>  			size = jffs2_1pass_load((char *)offset, part, filename);
>  		}
>
> @@ -1928,8 +1941,10 @@ int do_jffs2_ls(cmd_tbl_t *cmdtp, int flag, int argc, char *argv[])
>  		/* check partition type for cramfs */
>  		if (cramfs_check(part)) {
>  			ret = cramfs_ls (part, filename);
> +		} else if (romfs_check(part)) {
> +			ret = romfs_ls (part, filename);
>  		} else {
> -			/* if this is not cramfs assume jffs2 */
> +			/* if this is not cramfs or romfs assume jffs2 */
>  			ret = jffs2_1pass_ls(part, filename);
>  		}
>
> @@ -1951,7 +1966,6 @@ int do_jffs2_ls(cmd_tbl_t *cmdtp, int flag, int argc, char *argv[])
>  int do_jffs2_fsinfo(cmd_tbl_t *cmdtp, int flag, int argc, char *argv[])
>  {
>  	struct part_info *part;
> -	char *fsname;
>  	int ret;
>
>  	/* make sure we are in sync with env variables */
> @@ -1961,13 +1975,18 @@ int do_jffs2_fsinfo(cmd_tbl_t *cmdtp, int flag, int argc, char *argv[])
>  	if ((part = jffs2_part_info(current_dev, current_partnum))){
>
>  		/* check partition type for cramfs */
> -		fsname = (cramfs_check(part) ? "CRAMFS" : "JFFS2");
> -		printf("### filesystem type is %s\n", fsname);
> +		puts("### filesystem type is ");
>
>  		if (cramfs_check(part)) {
> +			puts("CRAMFS\n");
>  			ret = cramfs_info (part);
> +			
> +		} else if (romfs_check(part)) {
> +			puts("ROMFS\n");
> +			ret = romfs_info (part);
>  		} else {
> -			/* if this is not cramfs assume jffs2 */
> +			/* if this is not cramfs or romfs assume jffs2 */
> +			puts("JFFS2\n");
>  			ret = jffs2_1pass_info(part);
>  		}
>
> diff --git a/fs/Makefile b/fs/Makefile
> index 273d90e..118ae78 100644
> --- a/fs/Makefile
> +++ b/fs/Makefile
> @@ -22,7 +22,7 @@
>  #
>  #
>
> -SUBDIRS	:= jffs2 cramfs fdos fat reiserfs ext2
> +SUBDIRS	:= romfs jffs2 cramfs fdos fat reiserfs ext2
>
>  $(obj).depend all:
>  	@for dir in $(SUBDIRS) ; do \
> diff --git a/fs/romfs/romfs.c b/fs/romfs/romfs.c
> index e69de29..1a4b931 100644
> --- a/fs/romfs/romfs.c
> +++ b/fs/romfs/romfs.c
> @@ -0,0 +1,270 @@
> +/*
> + * (C) Copyright 2007 Michal Simek
> + *
> + * Michal SIMEK <monstr at monstr.eu>
> + *
> + * See file CREDITS for list of people who contributed to this
> + * project.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2 of
> + * the License, or (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
> + * MA 02111-1307 USA
> + */
> +
> +#include <common.h>
> +#include <malloc.h>
> +#include <command.h>
> +
> +#if (CONFIG_COMMANDS & CFG_CMD_JFFS2)

I believe Jon's patches have caused CONFIG_COMMANDS to go away.
Also, shouldn't it be gated by a new config value "CONFIG_CMD_ROMFS"?

> +static int romfs_list_inode (struct part_info *info, unsigned long offset)
> +{
> +	struct romfs_inode *inode =
> +			(struct romfs_inode *)(PART_OFFSET (info) + offset);
> +	struct romfs_inode *hardlink = NULL;
> +	char str[3], *data;
> +
> +/*	mapping		spec.info means
> + 0	hard link	link destination [file header]
> + 1	directory	first file's header
> + 2	regular file	unused, must be zero [MBZ]
> + 3	symbolic link	unused, MBZ (file data is the link content)
> + 4	block device	16/16 bits major/minor number
> + 5	char device		- " -
> + 6	socket		unused, MBZ
> + 7	fifo		unused, MBZ */
> +	switch (inode->next & 0x7) {
> +	case 0:
> +		str[0] = 'h';
> +		break;
> +	case 1:
> +		str[0] = 'd';
> +		break;
> +	case 2:
> +		str[0] = 'f';
> +		break;
> +	case 3:
> +		str[0] = 'l';
> +		break;
> +	case 4:
> +		str[0] = 'b';
> +		break;
> +	case 5:
> +		str[0] = 'c';
> +		break;
> +	case 6:
> +		str[0] = 's';
> +		break;
> +	case 7:
> +		str[0] = 'p';
> +		break;
> +	default:
> +		str[0] = '?';
> +	}

This can be done in a less verbose way by using an offset into a table
or string.

> +
> +	if (inode->next & 0x8) {
> +		str[1] = 'x';
> +	} else {
> +		str[1] = '-';
> +	}

Maybe?
str[1] = inode->next & 0x8 ? 'x' : '-';