Re: [PATCH 3/3] mkfs.ubifs: use libubi to format UBI volume

[Artem Bityutskiy <dedekind@infradead.org>]

Hi,

On Sat, 2009-05-09 at 11:41 +0200, Corentin Chary wrote:
> libubi is now used to format directly UBI volume.
> Typing mkfs.ubifs /dev/ubi0_0 is now possible.
> dtypes should be ok as they are taken from UBIFS code.

Did you test that this works fine on UBI volume and there
are no regressions when you output to a file?

...


>  static const char *helptext =
> -"Usage: mkfs.ubifs [OPTIONS]\n"
> +"Usage: mkfs.ubifs [OPTIONS] [device]\n"
> +"Example: mkfs.ubifs ubi0:test\n"
> +"         mkfs.ubifs -r /opt/img ubi0:test\n"
> +"         mkfs.ubifs -m 512 -e 128KiB -c 100 -r /opt/img -o ubifs.img\n\n"
>  "Make a UBIFS file system image from an existing directory tree\n\n"
>  "Options:\n"
>  "-r, -d, --root=DIR       build file system from directory DIR\n"

I think some more text in the help output which says you may direct
the result to either a file or UBI volume would be nice to have.

> @@ -357,11 +362,9 @@ static int validate_options(void)
>  {
>  	int tmp;
>  
> -	if (!root)
> -		return err_msg("root directory was not specified");
>  	if (!output)
> -		return err_msg("no output file specified");
> -	if (in_path(root, output))
> +		return err_msg("no output file or UBI volume specified");
> +	if (root && in_path(root, output))
>  		return err_msg("output file cannot be in the UBIFS root "
>  			       "directory");

Why you removed

if (!root)
	return err_msg("root directory was not specified");

?

>  	if (!is_power_of_2(c->min_io_size))
> @@ -468,6 +471,34 @@ static long long get_bytes(const char *str)
>  	return bytes;
>  }
>  
> +/**
> + * open_ubi - parse UBI device name string and open the UBI device.
> + * @name: UBI volume name
> + *
> + * There are several ways to specify UBI volumes
> + * o /dev/ubiX_Y - UBI device number X, volume Y;
> + *
> + * ubi:name ubiX_Y, etc.. are not implemented right now
> + *
> + * Returns %0 in case of success and %-1 in case of faillure
> + */
> +static int open_ubi(const char *name)
> +{
> +	struct stat st;
> +
> +	if (stat(name, &st) || !S_ISCHR(st.st_mode))
> +		return -1;
> +
> +	ubi = libubi_open();
> +	if (!ubi)
> +		return -1;
> +	if (ubi_get_vol_info(ubi, name, &c->vi))
> +		return -1;
> +	if (ubi_get_dev_info1(ubi, c->vi.dev_num, &c->di))
> +		return -1;
> +	return 0;
> +}

The comments are incorrect. 'ubi_get_vol_info()' does not depend on the
name of the file at all. It looks at device's major/minor, then looks up
the information in sysfs.
 
>  /**
> - * write_leb - copy the image of a LEB to the output file.
> + * write_leb - copy the image of a LEB to the output target

You removed the "." from the end :-)

> +
>  /**
>   * write_empty_leb - copy the image of an empty LEB to the output file.

Also s/file/target/ ?

> @@ -1011,8 +1064,6 @@ static int add_inode_with_data(struct stat *st, ino_t inum, void *data,
>  	ino->mtime_nsec = 0;
>  	ino->uid        = cpu_to_le32(st->st_uid);
>  	ino->gid        = cpu_to_le32(st->st_gid);
> -	ino->uid        = cpu_to_le32(st->st_uid);
> -	ino->gid        = cpu_to_le32(st->st_gid);
>  	ino->mode       = cpu_to_le32(st->st_mode);
>  	ino->flags      = cpu_to_le32(use_flags);
>  	ino->data_len   = cpu_to_le32(data_len);

Why the 2 lines are removed?

>  /**
> + * check_volume_empty - check if the UBI volume is empty.
> + *
> + * This function checks if the UBIFS volume is empty by looking if its LEBs are
> + * mapped or not.
> + * Returns zero in case of success and a negative error code in case of
> + * failure.
> + */
> +static int check_volume_empty()
check_volume_empty(void)

Comments are untidy.

> +
> +/**
> + * erase an ubi volume
> + */
> +static int erase_volume()

Ditto.

> +
> +	// FIXME: Ask if the user really want to erase this volume
> +	dbg_msg(1, "volume is not empty, erasing ...");
> +	for (lnum = 0; lnum < c->vi.rsvd_lebs; lnum++) {
> +		err = ubi_is_mapped(out_fd, lnum);
> +		if (err == 1) {
> +			dbg_msg(3, "erasing leb %d/%d", lnum, c->vi.rsvd_lebs);
> +			err = ubi_leb_unmap(out_fd, lnum);
> +		}
> +		if (err < 0)
> +			return err;
> +	}
> +	dbg_msg(1, "done.");
> +	return 0;
> +}

Yeah, I think you should either ask the user, or just refuse
writing to it. Users may just 'ubiupdatevol -t' to erase the
volume before invocating mkfs.ubifs.

Probably it is easier for now to just remove whole
'erase_volume()' ?
 
-- 
Best regards,
Artem Bityutskiy (Битюцкий Артём)