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

[Corentin Chary]

On Wed, May 13, 2009 at 10:12 AM, Artem Bityutskiy
<dedekind at infradead.org> wrote:
> 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?

I did some test with and without --root, it worked fine.
But I'll do some more test with the corrected patch.

>>  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.

Yep, and the first example is not valid anymore as this patch don't
handle "ubi:<name>" format.

>> @@ -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");
>
> ?

Because I wanted mkfs.ubifs to work without this parameter, like other mkfs.*.
I know mounting an empty volume will works, but we should also be able
to use mkfs.ubifs to format
empty volumes.


> 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.

Yep, I forget to rewrite the comments.

>>  /**
>> - * 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 :-)

Oups ....

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

Write empty leb to the target, wich can be a file or an UBI volume ..
I'll clarify that.

>> @@ -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?

Theses should have been removed in another patch, but look, they are
here twice =).

>> +static int check_volume_empty()
> check_volume_empty(void)
>
> Comments are untidy.

Oups ..

>
> 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()' ?
>
mkfs.ext{2/3/4} and mkfs.vfat don't ask for confirmation
I believe mkfs.reiserfs does.
But I won't hurt to ask.
Maybe first we can remove erase_volume(), and then, in another patch,
add erase volume with confirmation.


New patch is comming at the end of the week ...
-- 
Corentin Chary
http://xf.iksaif.net - http://uffs.org