Re: [PATCH 5/5] UBI: fastmap: more tiny TODOs

[Richard Weinberger <richard@nod.at>]

[-- Attachment #1: Type: text/plain, Size: 2880 bytes --]

Am 05.06.2012 17:11, schrieb Artem Bityutskiy:
> From: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
> 
> Signed-off-by: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
> ---
>  TODO                      |    2 ++
>  drivers/mtd/ubi/fastmap.c |    7 +++++++
>  2 files changed, 9 insertions(+)
> 
> diff --git a/TODO b/TODO
> index 17e30b6..a944159 100644
> --- a/TODO
> +++ b/TODO
> @@ -9,3 +9,5 @@ to the ubi-utils.git repository, to a separate branch at the beginning
>     test UBI + fastmap with it.
>  3. Test the autoresize feature
>  4. Test 'ubi_flush()'
> +5. Test that the same UBI image works fine on both LE and BE machines. I guess
> +   we can do this using sime kind of emulators?

Okay, I'll test the same image on x86 and ppc.

> diff --git a/drivers/mtd/ubi/fastmap.c b/drivers/mtd/ubi/fastmap.c
> index f938507..d446fc3 100644
> --- a/drivers/mtd/ubi/fastmap.c
> +++ b/drivers/mtd/ubi/fastmap.c
> @@ -455,6 +455,9 @@ out:
>   * @fm_raw: the fastmap it self as byte array
>   * @fm_size: size of the fastmap in bytes
>   */
> +/* TODO: please, make all pointers like 'fm_raw' of type void. It is indeed a
> + * pointer to data blob, it is not a pointer to a string of characters. Note,
> + * that in pointer arithmetics void * is the same as char *. */

I know that char * and void * are equivalent.
That's why I've chosen char *.
IMHO a blob is a string of chars. (Of course it's not a C sting).
Anyway, if you prefer void * I'll change it. :-)

>  static int ubi_attach_fastmap(struct ubi_device *ubi,
>  			      struct ubi_attach_info *ai,
>  			      char *fm_raw, size_t fm_size)
> @@ -503,6 +506,10 @@ static int ubi_attach_fastmap(struct ubi_device *ubi,
>  	if (fm_pos >= fm_size)
>  		goto fail_bad;
>  
> +	/* TODO: this is difficult to read. Can we please have instead an
> +	 * aggregate data structure? I did not think hard on it may be you have
> +	 * a good reason for this difficult style, but on the first glance it
> +	 * does not look like. And where are all the endiness stuff?  */

Yes, it's difficult to read but an aggregate data structure is not really doable.
Please let me explain why:

The fastmap data blob has not a fixed length and it's internal field don't have fixed positions.

1 struct ubi_fm_sb followed by
1 struct ubi_fm_hdr followed by
1 struct ubi_fm_scan_pool followed by
free_peb_count+used_peb_count struct ubi_fm_ec followed by
vol_count (
 struct ubi_fm_volhdr
 struct ubi_fm_eba
)

You see that only the first three structs have fixed positions.
So, the whole blob cannot be mapped into one aggregate data structure.
And I really don't want to play nasty games with variable-length arrays in stucts...

Why do you mean by "where are all the endiness stuff"?
The position calculation takes care about endiness.

Thanks,
//richard


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]