Patchwork [5/5] UBI: fastmap: more tiny TODOs

login
register
mail settings
Submitter Artem Bityutskiy
Date June 5, 2012, 3:11 p.m.
Message ID <1338909119-5188-6-git-send-email-dedekind1@gmail.com>
Download mbox | patch
Permalink /patch/163096/
State New
Headers show

Comments

Artem Bityutskiy - June 5, 2012, 3:11 p.m.
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(+)
Richard Weinberger - June 6, 2012, 9:30 p.m.
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

Patch

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?
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 *. */
 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?  */
 	fmhdr = (struct ubi_fm_hdr *)(fm_raw + fm_pos);
 	fm_pos += sizeof(*fmhdr);
 	if (fm_pos >= fm_size)