[PATCH 00/10] udf: cleanups

[PATCH 00/10] udf: cleanups

[marcin.slusarz]

Hi

This patchset contains various UDF fs cleanups.

[PATCH 01/10] udf: udf_CS0toUTF8 cleanup
[PATCH 02/10] udf: fix udf_build_ustr
[PATCH 03/10] udf: udf_CS0toNLS cleanup
[PATCH 04/10] udf: constify crc
[PATCH 05/10] udf: simple cleanup of truncate.c
[PATCH 06/10] udf: truncate: create function for updating of Allocation Ext Descriptor
[PATCH 07/10] udf: replace all adds to little endians variables with le*_add_cpu
[PATCH 08/10] udf: simplify __udf_read_inode
[PATCH 09/10] udf: replace udf_*_offset macros with functions
[PATCH 10/10] udf: constify udf_bitmap_lookup array

 fs/udf/balloc.c          |   13 +---
 fs/udf/crc.c             |    4 -
 fs/udf/ialloc.c          |   12 +---
 fs/udf/inode.c           |   68 +++++++++---------------
 fs/udf/super.c           |    2 
 fs/udf/truncate.c        |  132 ++++++++++++++++++++---------------------------
 fs/udf/udfdecl.h         |   33 +++++++----
 fs/udf/unicode.c         |   58 ++++++++------------
 include/linux/udf_fs_i.h |    4 -
 9 files changed, 142 insertions(+), 184 deletions(-)

This patchset depends on udf and byteorder patches (all in -mm).

Marcin Slusarz

[PATCH 01/10] udf: udf_CS0toUTF8 cleanup

[marcin.slusarz]

- fix error handling - always zero output variable
- don't zero explicitely fields zeroed by memset
- mark "in" paramater as const
- remove outdated comment

Signed-off-by: Marcin Slusarz <marcin.slusarz@gmail.com>
Cc: Jan Kara <jack@suse.cz>
---
 fs/udf/udfdecl.h |    2 +-
 fs/udf/unicode.c |   27 ++++++++++-----------------
 2 files changed, 11 insertions(+), 18 deletions(-)

diff --git a/fs/udf/udfdecl.h b/fs/udf/udfdecl.h
index 681dc2b..0c525e8 100644
--- a/fs/udf/udfdecl.h
+++ b/fs/udf/udfdecl.h
@@ -157,7 +157,7 @@ extern int udf_get_filename(struct super_block *, uint8_t *, uint8_t *, int);
 extern int udf_put_filename(struct super_block *, const uint8_t *, uint8_t *,
 			    int);
 extern int udf_build_ustr(struct ustr *, dstring *, int);
-extern int udf_CS0toUTF8(struct ustr *, struct ustr *);
+extern int udf_CS0toUTF8(struct ustr *, const struct ustr *);
 
 /* ialloc.c */
 extern void udf_free_inode(struct inode *);
diff --git a/fs/udf/unicode.c b/fs/udf/unicode.c
index e533b11..f969617 100644
--- a/fs/udf/unicode.c
+++ b/fs/udf/unicode.c
@@ -83,9 +83,6 @@ static int udf_build_ustr_exact(struct ustr *dest, dstring *ptr, int exactsize)
  * PURPOSE
  *	Convert OSTA Compressed Unicode to the UTF-8 equivalent.
  *
- * DESCRIPTION
- *	This routine is only called by udf_filldir().
- *
  * PRE-CONDITIONS
  *	utf			Pointer to UTF-8 output buffer.
  *	ocu			Pointer to OSTA Compressed Unicode input buffer
@@ -99,43 +96,39 @@ static int udf_build_ustr_exact(struct ustr *dest, dstring *ptr, int exactsize)
  *	November 12, 1997 - Andrew E. Mileski
  *	Written, tested, and released.
  */
-int udf_CS0toUTF8(struct ustr *utf_o, struct ustr *ocu_i)
+int udf_CS0toUTF8(struct ustr *utf_o, const struct ustr *ocu_i)
 {
-	uint8_t *ocu;
-	uint32_t c;
+	const uint8_t *ocu;
 	uint8_t cmp_id, ocu_len;
 	int i;
 
-	ocu = ocu_i->u_name;
-
 	ocu_len = ocu_i->u_len;
-	cmp_id = ocu_i->u_cmpID;
-	utf_o->u_len = 0;
-
 	if (ocu_len == 0) {
 		memset(utf_o, 0, sizeof(struct ustr));
-		utf_o->u_cmpID = 0;
-		utf_o->u_len = 0;
 		return 0;
 	}
 
-	if ((cmp_id != 8) && (cmp_id != 16)) {
+	cmp_id = ocu_i->u_cmpID;
+	if (cmp_id != 8 && cmp_id != 16) {
+		memset(utf_o, 0, sizeof(struct ustr));
 		printk(KERN_ERR "udf: unknown compression code (%d) stri=%s\n",
 		       cmp_id, ocu_i->u_name);
 		return 0;
 	}
 
+	ocu = ocu_i->u_name;
+	utf_o->u_len = 0;
 	for (i = 0; (i < ocu_len) && (utf_o->u_len <= (UDF_NAME_LEN - 3));) {
 
 		/* Expand OSTA compressed Unicode to Unicode */
-		c = ocu[i++];
+		uint32_t c = ocu[i++];
 		if (cmp_id == 16)
 			c = (c << 8) | ocu[i++];
 
 		/* Compress Unicode to UTF-8 */
-		if (c < 0x80U) {
+		if (c < 0x80U)
 			utf_o->u_name[utf_o->u_len++] = (uint8_t)c;
-		} else if (c < 0x800U) {
+		else if (c < 0x800U) {
 			utf_o->u_name[utf_o->u_len++] =
 						(uint8_t)(0xc0 | (c >> 6));
 			utf_o->u_name[utf_o->u_len++] =
-- 
1.5.3.7

Re: [PATCH 01/10] udf: udf_CS0toUTF8 cleanup

[Jan Kara]

On Wed 30-01-08 22:03:51, marcin.slusarz@gmail.com wrote:
> - fix error handling - always zero output variable
> - don't zero explicitely fields zeroed by memset
> - mark "in" paramater as const
> - remove outdated comment
> 
> Signed-off-by: Marcin Slusarz <marcin.slusarz@gmail.com>
> Cc: Jan Kara <jack@suse.cz>
  Acked-by: Jan Kara <jack@suse.cz>

							Honza
> ---
>  fs/udf/udfdecl.h |    2 +-
>  fs/udf/unicode.c |   27 ++++++++++-----------------
>  2 files changed, 11 insertions(+), 18 deletions(-)
> 
> diff --git a/fs/udf/udfdecl.h b/fs/udf/udfdecl.h
> index 681dc2b..0c525e8 100644
> --- a/fs/udf/udfdecl.h
> +++ b/fs/udf/udfdecl.h
> @@ -157,7 +157,7 @@ extern int udf_get_filename(struct super_block *, uint8_t *, uint8_t *, int);
>  extern int udf_put_filename(struct super_block *, const uint8_t *, uint8_t *,
>  			    int);
>  extern int udf_build_ustr(struct ustr *, dstring *, int);
> -extern int udf_CS0toUTF8(struct ustr *, struct ustr *);
> +extern int udf_CS0toUTF8(struct ustr *, const struct ustr *);
>  
>  /* ialloc.c */
>  extern void udf_free_inode(struct inode *);
> diff --git a/fs/udf/unicode.c b/fs/udf/unicode.c
> index e533b11..f969617 100644
> --- a/fs/udf/unicode.c
> +++ b/fs/udf/unicode.c
> @@ -83,9 +83,6 @@ static int udf_build_ustr_exact(struct ustr *dest, dstring *ptr, int exactsize)
>   * PURPOSE
>   *	Convert OSTA Compressed Unicode to the UTF-8 equivalent.
>   *
> - * DESCRIPTION
> - *	This routine is only called by udf_filldir().
> - *
>   * PRE-CONDITIONS
>   *	utf			Pointer to UTF-8 output buffer.
>   *	ocu			Pointer to OSTA Compressed Unicode input buffer
> @@ -99,43 +96,39 @@ static int udf_build_ustr_exact(struct ustr *dest, dstring *ptr, int exactsize)
>   *	November 12, 1997 - Andrew E. Mileski
>   *	Written, tested, and released.
>   */
> -int udf_CS0toUTF8(struct ustr *utf_o, struct ustr *ocu_i)
> +int udf_CS0toUTF8(struct ustr *utf_o, const struct ustr *ocu_i)
>  {
> -	uint8_t *ocu;
> -	uint32_t c;
> +	const uint8_t *ocu;
>  	uint8_t cmp_id, ocu_len;
>  	int i;
>  
> -	ocu = ocu_i->u_name;
> -
>  	ocu_len = ocu_i->u_len;
> -	cmp_id = ocu_i->u_cmpID;
> -	utf_o->u_len = 0;
> -
>  	if (ocu_len == 0) {
>  		memset(utf_o, 0, sizeof(struct ustr));
> -		utf_o->u_cmpID = 0;
> -		utf_o->u_len = 0;
>  		return 0;
>  	}
>  
> -	if ((cmp_id != 8) && (cmp_id != 16)) {
> +	cmp_id = ocu_i->u_cmpID;
> +	if (cmp_id != 8 && cmp_id != 16) {
> +		memset(utf_o, 0, sizeof(struct ustr));
>  		printk(KERN_ERR "udf: unknown compression code (%d) stri=%s\n",
>  		       cmp_id, ocu_i->u_name);
>  		return 0;
>  	}
>  
> +	ocu = ocu_i->u_name;
> +	utf_o->u_len = 0;
>  	for (i = 0; (i < ocu_len) && (utf_o->u_len <= (UDF_NAME_LEN - 3));) {
>  
>  		/* Expand OSTA compressed Unicode to Unicode */
> -		c = ocu[i++];
> +		uint32_t c = ocu[i++];
>  		if (cmp_id == 16)
>  			c = (c << 8) | ocu[i++];
>  
>  		/* Compress Unicode to UTF-8 */
> -		if (c < 0x80U) {
> +		if (c < 0x80U)
>  			utf_o->u_name[utf_o->u_len++] = (uint8_t)c;
> -		} else if (c < 0x800U) {
> +		else if (c < 0x800U) {
>  			utf_o->u_name[utf_o->u_len++] =
>  						(uint8_t)(0xc0 | (c >> 6));
>  			utf_o->u_name[utf_o->u_len++] =
> -- 
> 1.5.3.7
> 
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

[PATCH 02/10] udf: fix udf_build_ustr

[marcin.slusarz]

udf_build_ustr was completely broken when
size >= UDF_NAME_LEN - 1 or size < 2

nobody noticed because all callers set size
to acceptable values (constants)

Signed-off-by: Marcin Slusarz <marcin.slusarz@gmail.com>
Cc: Jan Kara <jack@suse.cz>
---
 fs/udf/unicode.c |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/fs/udf/unicode.c b/fs/udf/unicode.c
index f969617..f4e54e5 100644
--- a/fs/udf/unicode.c
+++ b/fs/udf/unicode.c
@@ -47,16 +47,16 @@ static int udf_char_to_ustr(struct ustr *dest, const uint8_t *src, int strlen)
  */
 int udf_build_ustr(struct ustr *dest, dstring *ptr, int size)
 {
-	int usesize;
+	u8 usesize;
 
-	if ((!dest) || (!ptr) || (!size))
+	if (!dest || !ptr || size < 2)
 		return -1;
 
-	memset(dest, 0, sizeof(struct ustr));
-	usesize = (size > UDF_NAME_LEN) ? UDF_NAME_LEN : size;
+	usesize = min_t(size_t, size - 2, sizeof(dest->u_name));
 	dest->u_cmpID = ptr[0];
-	dest->u_len = ptr[size - 1];
-	memcpy(dest->u_name, ptr + 1, usesize - 1);
+	dest->u_len = usesize;
+	memcpy(dest->u_name, ptr + 1, usesize);
+	memset(dest->u_name + usesize, 0, sizeof(dest->u_name) - usesize);
 
 	return 0;
 }
-- 
1.5.3.7

Re: [PATCH 02/10] udf: fix udf_build_ustr

[Jan Kara]

On Wed 30-01-08 22:03:52, marcin.slusarz@gmail.com wrote:
> udf_build_ustr was completely broken when
> size >= UDF_NAME_LEN - 1 or size < 2
> 
> nobody noticed because all callers set size
> to acceptable values (constants)
> 
> Signed-off-by: Marcin Slusarz <marcin.slusarz@gmail.com>
> Cc: Jan Kara <jack@suse.cz>
> ---
>  fs/udf/unicode.c |   12 ++++++------
>  1 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/udf/unicode.c b/fs/udf/unicode.c
> index f969617..f4e54e5 100644
> --- a/fs/udf/unicode.c
> +++ b/fs/udf/unicode.c
> @@ -47,16 +47,16 @@ static int udf_char_to_ustr(struct ustr *dest, const uint8_t *src, int strlen)
>   */
>  int udf_build_ustr(struct ustr *dest, dstring *ptr, int size)
>  {
> -	int usesize;
> +	u8 usesize;
  What is the purpose for this? Why not just leave int there? Arithmetics
is usually best done in ints if that's possible...
>  
> -	if ((!dest) || (!ptr) || (!size))
> +	if (!dest || !ptr || size < 2)
>  		return -1;
>  
> -	memset(dest, 0, sizeof(struct ustr));
> -	usesize = (size > UDF_NAME_LEN) ? UDF_NAME_LEN : size;
> +	usesize = min_t(size_t, size - 2, sizeof(dest->u_name));
>  	dest->u_cmpID = ptr[0];
> -	dest->u_len = ptr[size - 1];
> -	memcpy(dest->u_name, ptr + 1, usesize - 1);
> +	dest->u_len = usesize;
> +	memcpy(dest->u_name, ptr + 1, usesize);
> +	memset(dest->u_name + usesize, 0, sizeof(dest->u_name) - usesize);
  Hmm, after parsing what the standard says (ugh), I don't think the code is
wrong (at least I think you made it incorrect). The caller of
udf_char_to_ustr() specifies length of the field (not length of the
string). Now, in the last character of the field is stored the number of
characters in the string and in the first character of the field is stored
encoding of the string. So the original code seems correct.

								Honza
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

Re: [PATCH 02/10] udf: fix udf_build_ustr

[Marcin Slusarz]

On Thu, Jan 31, 2008 at 11:45:32AM +0100, Jan Kara wrote:
> On Wed 30-01-08 22:03:52, marcin.slusarz@gmail.com wrote:
> > udf_build_ustr was completely broken when
> > size >= UDF_NAME_LEN - 1 or size < 2
> > 
> > nobody noticed because all callers set size
> > to acceptable values (constants)
> > 
> > Signed-off-by: Marcin Slusarz <marcin.slusarz@gmail.com>
> > Cc: Jan Kara <jack@suse.cz>
> > ---
> >  fs/udf/unicode.c |   12 ++++++------
> >  1 files changed, 6 insertions(+), 6 deletions(-)
> > 
> > diff --git a/fs/udf/unicode.c b/fs/udf/unicode.c
> > index f969617..f4e54e5 100644
> > --- a/fs/udf/unicode.c
> > +++ b/fs/udf/unicode.c
> > @@ -47,16 +47,16 @@ static int udf_char_to_ustr(struct ustr *dest, const uint8_t *src, int strlen)
> >   */
> >  int udf_build_ustr(struct ustr *dest, dstring *ptr, int size)
> >  {
> > -	int usesize;
> > +	u8 usesize;
>   What is the purpose for this? Why not just leave int there? Arithmetics
> is usually best done in ints if that's possible...
I made it to stress that length of string fits in one byte.
(struct ustr->u_len is uint8_t)

> > -	if ((!dest) || (!ptr) || (!size))
> > +	if (!dest || !ptr || size < 2)
> >  		return -1;
> >  
> > -	memset(dest, 0, sizeof(struct ustr));
> > -	usesize = (size > UDF_NAME_LEN) ? UDF_NAME_LEN : size;
> > +	usesize = min_t(size_t, size - 2, sizeof(dest->u_name));
> >  	dest->u_cmpID = ptr[0];
> > -	dest->u_len = ptr[size - 1];
> > -	memcpy(dest->u_name, ptr + 1, usesize - 1);
> > +	dest->u_len = usesize;
> > +	memcpy(dest->u_name, ptr + 1, usesize);
> > +	memset(dest->u_name + usesize, 0, sizeof(dest->u_name) - usesize);
>   Hmm, after parsing what the standard says (ugh), I don't think the code is
> wrong (at least I think you made it incorrect). The caller of
> udf_char_to_ustr() specifies length of the field (not length of the
> string). Now, in the last character of the field is stored the number of
> characters in the string and in the first character of the field is stored
> encoding of the string. So the original code seems correct.
You are right. I broke length calculation.

But observe that:
- when size == 1:
	dest->u_len = ptr[1 - 1], but at ptr[0] there's cmpID,
	so we create string with wrong length
- when size > 1 and size < UDF_NAME_LEN:
	we set u_len correctly, but memcpy copies one needless byte
- when size == UDF_NAME_LEN - 1:
	memcpy overwrites u_len - with correct value, but...
- when size >= UDF_NAME_LEN:
	we copy UDF_NAME_LEN - 1 bytes, but dest->u_name is array
	of UDF_NAME_LEN - 2 bytes, so we are overwriting u_len with
	character from input string

So if I didn't mess up someting, correct change would look like this:
---
udf: fix udf_build_ustr

udf_build_ustr was broken when
size >= UDF_NAME_LEN or size < 2

nobody noticed because all callers set size
to acceptable values (constants whitin range)

Signed-off-by: Marcin Slusarz <marcin.slusarz@gmail.com>
Cc: Jan Kara <jack@suse.cz>
---
 fs/udf/unicode.c |   13 +++++++------
 1 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/fs/udf/unicode.c b/fs/udf/unicode.c
index 335fc56..83ae9fc 100644
--- a/fs/udf/unicode.c
+++ b/fs/udf/unicode.c
@@ -47,16 +47,17 @@ static int udf_char_to_ustr(struct ustr *dest, const uint8_t *src, int strlen)
  */
 int udf_build_ustr(struct ustr *dest, dstring *ptr, int size)
 {
-	int usesize;
+	u8 usesize;
 
-	if ((!dest) || (!ptr) || (!size))
+	if (!dest || !ptr || size < 2)
 		return -1;
 
-	memset(dest, 0, sizeof(struct ustr));
-	usesize = (size > UDF_NAME_LEN) ? UDF_NAME_LEN : size;
+	usesize = min_t(size_t, ptr[size - 1], sizeof(dest->u_name));
+	usesize = min_t(size_t, usesize, size - 2);
 	dest->u_cmpID = ptr[0];
-	dest->u_len = ptr[size - 1];
-	memcpy(dest->u_name, ptr + 1, usesize - 1);
+	dest->u_len = usesize;
+	memcpy(dest->u_name, ptr + 1, usesize);
+	memset(dest->u_name + usesize, 0, sizeof(dest->u_name) - usesize);
 
 	return 0;
 }
-- 
1.5.3.7

Re: [PATCH 02/10] udf: fix udf_build_ustr

[Jan Kara]

On Thu 31-01-08 20:57:47, Marcin Slusarz wrote:
> On Thu, Jan 31, 2008 at 11:45:32AM +0100, Jan Kara wrote:
> > On Wed 30-01-08 22:03:52, marcin.slusarz@gmail.com wrote:
> > > udf_build_ustr was completely broken when
> > > size >= UDF_NAME_LEN - 1 or size < 2
> > > 
> > > nobody noticed because all callers set size
> > > to acceptable values (constants)
> > > 
> > > Signed-off-by: Marcin Slusarz <marcin.slusarz@gmail.com>
> > > Cc: Jan Kara <jack@suse.cz>
> > > ---
> > >  fs/udf/unicode.c |   12 ++++++------
> > >  1 files changed, 6 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/fs/udf/unicode.c b/fs/udf/unicode.c
> > > index f969617..f4e54e5 100644
> > > --- a/fs/udf/unicode.c
> > > +++ b/fs/udf/unicode.c
> > > @@ -47,16 +47,16 @@ static int udf_char_to_ustr(struct ustr *dest, const uint8_t *src, int strlen)
> > >   */
> > >  int udf_build_ustr(struct ustr *dest, dstring *ptr, int size)
> > >  {
> > > -	int usesize;
> > > +	u8 usesize;
> >   What is the purpose for this? Why not just leave int there? Arithmetics
> > is usually best done in ints if that's possible...
> I made it to stress that length of string fits in one byte.
> (struct ustr->u_len is uint8_t)
  I see. I don't think this is really worthwhile, please keep int there.

> > > -	if ((!dest) || (!ptr) || (!size))
> > > +	if (!dest || !ptr || size < 2)
> > >  		return -1;
> > >  
> > > -	memset(dest, 0, sizeof(struct ustr));
> > > -	usesize = (size > UDF_NAME_LEN) ? UDF_NAME_LEN : size;
> > > +	usesize = min_t(size_t, size - 2, sizeof(dest->u_name));
> > >  	dest->u_cmpID = ptr[0];
> > > -	dest->u_len = ptr[size - 1];
> > > -	memcpy(dest->u_name, ptr + 1, usesize - 1);
> > > +	dest->u_len = usesize;
> > > +	memcpy(dest->u_name, ptr + 1, usesize);
> > > +	memset(dest->u_name + usesize, 0, sizeof(dest->u_name) - usesize);
> >   Hmm, after parsing what the standard says (ugh), I don't think the code is
> > wrong (at least I think you made it incorrect). The caller of
> > udf_char_to_ustr() specifies length of the field (not length of the
> > string). Now, in the last character of the field is stored the number of
> > characters in the string and in the first character of the field is stored
> > encoding of the string. So the original code seems correct.
> You are right. I broke length calculation.
> 
> But observe that:
> - when size == 1:
> 	dest->u_len = ptr[1 - 1], but at ptr[0] there's cmpID,
> 	so we create string with wrong length
  Yes, but that never happens. This function should always be used for
fixed-length strings whose maximum length is defined in the standard so if
someone calls it with size == 1, it is a bug. So you can just do
BUG_ON(size < 2).

> - when size > 1 and size < UDF_NAME_LEN:
> 	we set u_len correctly, but memcpy copies one needless byte
> - when size == UDF_NAME_LEN - 1:
> 	memcpy overwrites u_len - with correct value, but...
  Yes, you're right.

> - when size >= UDF_NAME_LEN:
> 	we copy UDF_NAME_LEN - 1 bytes, but dest->u_name is array
> 	of UDF_NAME_LEN - 2 bytes, so we are overwriting u_len with
> 	character from input string
  Again, this should not happen because UDF_NAME_LEN is large enough but
you are right it's better to clean this.

> So if I didn't mess up someting, correct change would look like this:
> ---
> udf: fix udf_build_ustr
> 
> udf_build_ustr was broken when
> size >= UDF_NAME_LEN or size < 2
> 
> nobody noticed because all callers set size
> to acceptable values (constants whitin range)
> 
> Signed-off-by: Marcin Slusarz <marcin.slusarz@gmail.com>
> Cc: Jan Kara <jack@suse.cz>
> ---
>  fs/udf/unicode.c |   13 +++++++------
>  1 files changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/udf/unicode.c b/fs/udf/unicode.c
> index 335fc56..83ae9fc 100644
> --- a/fs/udf/unicode.c
> +++ b/fs/udf/unicode.c
> @@ -47,16 +47,17 @@ static int udf_char_to_ustr(struct ustr *dest, const uint8_t *src, int strlen)
>   */
>  int udf_build_ustr(struct ustr *dest, dstring *ptr, int size)
>  {
> -	int usesize;
> +	u8 usesize;
  Just use int here..

>  
> -	if ((!dest) || (!ptr) || (!size))
> +	if (!dest || !ptr || size < 2)
>  		return -1;
>  
> -	memset(dest, 0, sizeof(struct ustr));
> -	usesize = (size > UDF_NAME_LEN) ? UDF_NAME_LEN : size;
> +	usesize = min_t(size_t, ptr[size - 1], sizeof(dest->u_name));
> +	usesize = min_t(size_t, usesize, size - 2);
  And here use just min() in both cases so that it's easier to read.

>  	dest->u_cmpID = ptr[0];
> -	dest->u_len = ptr[size - 1];
> -	memcpy(dest->u_name, ptr + 1, usesize - 1);
> +	dest->u_len = usesize;
> +	memcpy(dest->u_name, ptr + 1, usesize);
> +	memset(dest->u_name + usesize, 0, sizeof(dest->u_name) - usesize);
>  
>  	return 0;
>  }
  Otherwise it looks fine. Thanks for the cleanups.

										Honza

-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

Re: [PATCH 02/10] udf: fix udf_build_ustr

[Marcin Slusarz]

On Mon, Feb 04, 2008 at 08:31:07PM +0100, Jan Kara wrote:
> On Thu 31-01-08 20:57:47, Marcin Slusarz wrote:
> > On Thu, Jan 31, 2008 at 11:45:32AM +0100, Jan Kara wrote:
> > > On Wed 30-01-08 22:03:52, marcin.slusarz@gmail.com wrote:
> > > > udf_build_ustr was completely broken when
> > > > size >= UDF_NAME_LEN - 1 or size < 2
> > > > 
> > > > nobody noticed because all callers set size
> > > > to acceptable values (constants)
> > > > 
> > > > Signed-off-by: Marcin Slusarz <marcin.slusarz@gmail.com>
> > > > Cc: Jan Kara <jack@suse.cz>
> > > > ---
> > > >  fs/udf/unicode.c |   12 ++++++------
> > > >  1 files changed, 6 insertions(+), 6 deletions(-)
> > > > 
> > > > diff --git a/fs/udf/unicode.c b/fs/udf/unicode.c
> > > > index f969617..f4e54e5 100644
> > > > --- a/fs/udf/unicode.c
> > > > +++ b/fs/udf/unicode.c
> > > > @@ -47,16 +47,16 @@ static int udf_char_to_ustr(struct ustr *dest, const uint8_t *src, int strlen)
> > > >   */
> > > >  int udf_build_ustr(struct ustr *dest, dstring *ptr, int size)
> > > >  {
> > > > -	int usesize;
> > > > +	u8 usesize;
> > >   What is the purpose for this? Why not just leave int there? Arithmetics
> > > is usually best done in ints if that's possible...
> > I made it to stress that length of string fits in one byte.
> > (struct ustr->u_len is uint8_t)
>   I see. I don't think this is really worthwhile, please keep int there.
> 
> > > > -	if ((!dest) || (!ptr) || (!size))
> > > > +	if (!dest || !ptr || size < 2)
> > > >  		return -1;
> > > >  
> > > > -	memset(dest, 0, sizeof(struct ustr));
> > > > -	usesize = (size > UDF_NAME_LEN) ? UDF_NAME_LEN : size;
> > > > +	usesize = min_t(size_t, size - 2, sizeof(dest->u_name));
> > > >  	dest->u_cmpID = ptr[0];
> > > > -	dest->u_len = ptr[size - 1];
> > > > -	memcpy(dest->u_name, ptr + 1, usesize - 1);
> > > > +	dest->u_len = usesize;
> > > > +	memcpy(dest->u_name, ptr + 1, usesize);
> > > > +	memset(dest->u_name + usesize, 0, sizeof(dest->u_name) - usesize);
> > >   Hmm, after parsing what the standard says (ugh), I don't think the code is
> > > wrong (at least I think you made it incorrect). The caller of
> > > udf_char_to_ustr() specifies length of the field (not length of the
> > > string). Now, in the last character of the field is stored the number of
> > > characters in the string and in the first character of the field is stored
> > > encoding of the string. So the original code seems correct.
> > You are right. I broke length calculation.
> > 
> > But observe that:
> > - when size == 1:
> > 	dest->u_len = ptr[1 - 1], but at ptr[0] there's cmpID,
> > 	so we create string with wrong length
>   Yes, but that never happens. This function should always be used for
> fixed-length strings whose maximum length is defined in the standard so if
> someone calls it with size == 1, it is a bug. So you can just do
> BUG_ON(size < 2).
> 
> > - when size > 1 and size < UDF_NAME_LEN:
> > 	we set u_len correctly, but memcpy copies one needless byte
> > - when size == UDF_NAME_LEN - 1:
> > 	memcpy overwrites u_len - with correct value, but...
>   Yes, you're right.
> 
> > - when size >= UDF_NAME_LEN:
> > 	we copy UDF_NAME_LEN - 1 bytes, but dest->u_name is array
> > 	of UDF_NAME_LEN - 2 bytes, so we are overwriting u_len with
> > 	character from input string
>   Again, this should not happen because UDF_NAME_LEN is large enough but
> you are right it's better to clean this.
> 
> > So if I didn't mess up someting, correct change would look like this:
> > ---
> > udf: fix udf_build_ustr
> > 
> > udf_build_ustr was broken when
> > size >= UDF_NAME_LEN or size < 2
> > 
> > nobody noticed because all callers set size
> > to acceptable values (constants whitin range)
> > 
> > Signed-off-by: Marcin Slusarz <marcin.slusarz@gmail.com>
> > Cc: Jan Kara <jack@suse.cz>
> > ---
> >  fs/udf/unicode.c |   13 +++++++------
> >  1 files changed, 7 insertions(+), 6 deletions(-)
> > 
> > diff --git a/fs/udf/unicode.c b/fs/udf/unicode.c
> > index 335fc56..83ae9fc 100644
> > --- a/fs/udf/unicode.c
> > +++ b/fs/udf/unicode.c
> > @@ -47,16 +47,17 @@ static int udf_char_to_ustr(struct ustr *dest, const uint8_t *src, int strlen)
> >   */
> >  int udf_build_ustr(struct ustr *dest, dstring *ptr, int size)
> >  {
> > -	int usesize;
> > +	u8 usesize;
>   Just use int here..
Ok
 
> >  
> > -	if ((!dest) || (!ptr) || (!size))
> > +	if (!dest || !ptr || size < 2)
> >  		return -1;
> >  
> > -	memset(dest, 0, sizeof(struct ustr));
> > -	usesize = (size > UDF_NAME_LEN) ? UDF_NAME_LEN : size;
> > +	usesize = min_t(size_t, ptr[size - 1], sizeof(dest->u_name));
> > +	usesize = min_t(size_t, usesize, size - 2);
>   And here use just min() in both cases so that it's easier to read.
I used min_t because gcc and sparse warned about different types.
 
> >  	dest->u_cmpID = ptr[0];
> > -	dest->u_len = ptr[size - 1];
> > -	memcpy(dest->u_name, ptr + 1, usesize - 1);
> > +	dest->u_len = usesize;
> > +	memcpy(dest->u_name, ptr + 1, usesize);
> > +	memset(dest->u_name + usesize, 0, sizeof(dest->u_name) - usesize);
> >  
> >  	return 0;
> >  }
>   Otherwise it looks fine. Thanks for the cleanups.

Updated patch:
---
udf: fix udf_build_ustr

udf_build_ustr was broken:

- size == 1:
    dest->u_len = ptr[1 - 1], but at ptr[0] there's cmpID,
    so we created string with wrong length
    it should not happen, so we BUG() it
- size > 1 and size < UDF_NAME_LEN:
    we set u_len correctly, but memcpy copied one needless byte
- size == UDF_NAME_LEN - 1:
    memcpy overwrited u_len - with correct value, but...
- size >= UDF_NAME_LEN:
    we copied UDF_NAME_LEN - 1 bytes, but dest->u_name is array
    of UDF_NAME_LEN - 2 bytes, so we were overwriting u_len with
    character from input string

nobody noticed because all callers set size
to acceptable values (constants whitin range)

Signed-off-by: Marcin Slusarz <marcin.slusarz@gmail.com>
Cc: Jan Kara <jack@suse.cz>
---
 fs/udf/unicode.c |   12 +++++++-----
 1 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/fs/udf/unicode.c b/fs/udf/unicode.c
index 335fc56..442d38a 100644
--- a/fs/udf/unicode.c
+++ b/fs/udf/unicode.c
@@ -49,14 +49,16 @@ int udf_build_ustr(struct ustr *dest, dstring *ptr, int size)
 {
 	int usesize;
 
-	if ((!dest) || (!ptr) || (!size))
+	if (!dest || !ptr || !size)
 		return -1;
+	BUG_ON(size < 2);
 
-	memset(dest, 0, sizeof(struct ustr));
-	usesize = (size > UDF_NAME_LEN) ? UDF_NAME_LEN : size;
+	usesize = min_t(size_t, ptr[size - 1], sizeof(dest->u_name));
+	usesize = min(usesize, size - 2);
 	dest->u_cmpID = ptr[0];
-	dest->u_len = ptr[size - 1];
-	memcpy(dest->u_name, ptr + 1, usesize - 1);
+	dest->u_len = usesize;
+	memcpy(dest->u_name, ptr + 1, usesize);
+	memset(dest->u_name + usesize, 0, sizeof(dest->u_name) - usesize);
 
 	return 0;
 }
-- 
1.5.3.7

Re: [PATCH 02/10] udf: fix udf_build_ustr

[Jan Kara]

On Mon 04-02-08 22:27:39, Marcin Slusarz wrote:
> On Mon, Feb 04, 2008 at 08:31:07PM +0100, Jan Kara wrote:
> > On Thu 31-01-08 20:57:47, Marcin Slusarz wrote:
> > > On Thu, Jan 31, 2008 at 11:45:32AM +0100, Jan Kara wrote:
> > > > On Wed 30-01-08 22:03:52, marcin.slusarz@gmail.com wrote:
> > > > > udf_build_ustr was completely broken when
> > > > > size >= UDF_NAME_LEN - 1 or size < 2
> > > > > 
> > > > > nobody noticed because all callers set size
> > > > > to acceptable values (constants)
> > > > > 
> > > > > Signed-off-by: Marcin Slusarz <marcin.slusarz@gmail.com>
> > > > > Cc: Jan Kara <jack@suse.cz>
> > > > > ---
> > > > >  fs/udf/unicode.c |   12 ++++++------
> > > > >  1 files changed, 6 insertions(+), 6 deletions(-)
> > > > > 
> > > > > diff --git a/fs/udf/unicode.c b/fs/udf/unicode.c
> > > > > index f969617..f4e54e5 100644
> > > > > --- a/fs/udf/unicode.c
> > > > > +++ b/fs/udf/unicode.c
> > > > > @@ -47,16 +47,16 @@ static int udf_char_to_ustr(struct ustr *dest, const uint8_t *src, int strlen)
> > > > >   */
> > > > >  int udf_build_ustr(struct ustr *dest, dstring *ptr, int size)
> > > > >  {
> > > > > -	int usesize;
> > > > > +	u8 usesize;
> > > >   What is the purpose for this? Why not just leave int there? Arithmetics
> > > > is usually best done in ints if that's possible...
> > > I made it to stress that length of string fits in one byte.
> > > (struct ustr->u_len is uint8_t)
> >   I see. I don't think this is really worthwhile, please keep int there.
> > 
> > > > > -	if ((!dest) || (!ptr) || (!size))
> > > > > +	if (!dest || !ptr || size < 2)
> > > > >  		return -1;
> > > > >  
> > > > > -	memset(dest, 0, sizeof(struct ustr));
> > > > > -	usesize = (size > UDF_NAME_LEN) ? UDF_NAME_LEN : size;
> > > > > +	usesize = min_t(size_t, size - 2, sizeof(dest->u_name));
> > > > >  	dest->u_cmpID = ptr[0];
> > > > > -	dest->u_len = ptr[size - 1];
> > > > > -	memcpy(dest->u_name, ptr + 1, usesize - 1);
> > > > > +	dest->u_len = usesize;
> > > > > +	memcpy(dest->u_name, ptr + 1, usesize);
> > > > > +	memset(dest->u_name + usesize, 0, sizeof(dest->u_name) - usesize);
> > > >   Hmm, after parsing what the standard says (ugh), I don't think the code is
> > > > wrong (at least I think you made it incorrect). The caller of
> > > > udf_char_to_ustr() specifies length of the field (not length of the
> > > > string). Now, in the last character of the field is stored the number of
> > > > characters in the string and in the first character of the field is stored
> > > > encoding of the string. So the original code seems correct.
> > > You are right. I broke length calculation.
> > > 
> > > But observe that:
> > > - when size == 1:
> > > 	dest->u_len = ptr[1 - 1], but at ptr[0] there's cmpID,
> > > 	so we create string with wrong length
> >   Yes, but that never happens. This function should always be used for
> > fixed-length strings whose maximum length is defined in the standard so if
> > someone calls it with size == 1, it is a bug. So you can just do
> > BUG_ON(size < 2).
> > 
> > > - when size > 1 and size < UDF_NAME_LEN:
> > > 	we set u_len correctly, but memcpy copies one needless byte
> > > - when size == UDF_NAME_LEN - 1:
> > > 	memcpy overwrites u_len - with correct value, but...
> >   Yes, you're right.
> > 
> > > - when size >= UDF_NAME_LEN:
> > > 	we copy UDF_NAME_LEN - 1 bytes, but dest->u_name is array
> > > 	of UDF_NAME_LEN - 2 bytes, so we are overwriting u_len with
> > > 	character from input string
> >   Again, this should not happen because UDF_NAME_LEN is large enough but
> > you are right it's better to clean this.
> > 
> > > So if I didn't mess up someting, correct change would look like this:
> > > ---
> > > udf: fix udf_build_ustr
> > > 
> > > udf_build_ustr was broken when
> > > size >= UDF_NAME_LEN or size < 2
> > > 
> > > nobody noticed because all callers set size
> > > to acceptable values (constants whitin range)
> > > 
> > > Signed-off-by: Marcin Slusarz <marcin.slusarz@gmail.com>
> > > Cc: Jan Kara <jack@suse.cz>
> > > ---
> > >  fs/udf/unicode.c |   13 +++++++------
> > >  1 files changed, 7 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/fs/udf/unicode.c b/fs/udf/unicode.c
> > > index 335fc56..83ae9fc 100644
> > > --- a/fs/udf/unicode.c
> > > +++ b/fs/udf/unicode.c
> > > @@ -47,16 +47,17 @@ static int udf_char_to_ustr(struct ustr *dest, const uint8_t *src, int strlen)
> > >   */
> > >  int udf_build_ustr(struct ustr *dest, dstring *ptr, int size)
> > >  {
> > > -	int usesize;
> > > +	u8 usesize;
> >   Just use int here..
> Ok
>  
> > >  
> > > -	if ((!dest) || (!ptr) || (!size))
> > > +	if (!dest || !ptr || size < 2)
> > >  		return -1;
> > >  
> > > -	memset(dest, 0, sizeof(struct ustr));
> > > -	usesize = (size > UDF_NAME_LEN) ? UDF_NAME_LEN : size;
> > > +	usesize = min_t(size_t, ptr[size - 1], sizeof(dest->u_name));
> > > +	usesize = min_t(size_t, usesize, size - 2);
> >   And here use just min() in both cases so that it's easier to read.
> I used min_t because gcc and sparse warned about different types.
>  
> > >  	dest->u_cmpID = ptr[0];
> > > -	dest->u_len = ptr[size - 1];
> > > -	memcpy(dest->u_name, ptr + 1, usesize - 1);
> > > +	dest->u_len = usesize;
> > > +	memcpy(dest->u_name, ptr + 1, usesize);
> > > +	memset(dest->u_name + usesize, 0, sizeof(dest->u_name) - usesize);
> > >  
> > >  	return 0;
> > >  }
> >   Otherwise it looks fine. Thanks for the cleanups.
> 
> Updated patch:
> ---
> udf: fix udf_build_ustr
> 
> udf_build_ustr was broken:
> 
> - size == 1:
>     dest->u_len = ptr[1 - 1], but at ptr[0] there's cmpID,
>     so we created string with wrong length
>     it should not happen, so we BUG() it
> - size > 1 and size < UDF_NAME_LEN:
>     we set u_len correctly, but memcpy copied one needless byte
> - size == UDF_NAME_LEN - 1:
>     memcpy overwrited u_len - with correct value, but...
> - size >= UDF_NAME_LEN:
>     we copied UDF_NAME_LEN - 1 bytes, but dest->u_name is array
>     of UDF_NAME_LEN - 2 bytes, so we were overwriting u_len with
>     character from input string
> 
> nobody noticed because all callers set size
> to acceptable values (constants whitin range)
> 
> Signed-off-by: Marcin Slusarz <marcin.slusarz@gmail.com>
> Cc: Jan Kara <jack@suse.cz>
> ---
>  fs/udf/unicode.c |   12 +++++++-----
>  1 files changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/udf/unicode.c b/fs/udf/unicode.c
> index 335fc56..442d38a 100644
> --- a/fs/udf/unicode.c
> +++ b/fs/udf/unicode.c
> @@ -49,14 +49,16 @@ int udf_build_ustr(struct ustr *dest, dstring *ptr, int size)
>  {
>  	int usesize;
>  
> -	if ((!dest) || (!ptr) || (!size))
> +	if (!dest || !ptr || !size)
>  		return -1;
> +	BUG_ON(size < 2);
>  
> -	memset(dest, 0, sizeof(struct ustr));
> -	usesize = (size > UDF_NAME_LEN) ? UDF_NAME_LEN : size;
> +	usesize = min_t(size_t, ptr[size - 1], sizeof(dest->u_name));
> +	usesize = min(usesize, size - 2);
>  	dest->u_cmpID = ptr[0];
> -	dest->u_len = ptr[size - 1];
> -	memcpy(dest->u_name, ptr + 1, usesize - 1);
> +	dest->u_len = usesize;
> +	memcpy(dest->u_name, ptr + 1, usesize);
> +	memset(dest->u_name + usesize, 0, sizeof(dest->u_name) - usesize);
>  
>  	return 0;
>  }
  OK, fine, thanks. Acked-by: Jan Kara <jack@suse.cz>

									Honza

PS: I'm working on getting access to kernel.org so that I can run UDF git
tree there so we try merging these patches with the new git when I set that
up :).
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

Re: [PATCH 02/10] udf: fix udf_build_ustr

[Marcin Slusarz]

On Tue, Feb 05, 2008 at 04:29:23PM +0100, Jan Kara wrote:
> PS: I'm working on getting access to kernel.org so that I can run UDF git
> tree there so we try merging these patches with the new git when I set that
> up :).
Good to hear that :)

Marcin

[PATCH 03/10] udf: udf_CS0toNLS cleanup

[marcin.slusarz]

- fix error handling - always zero output variable
- don't zero explicitely fields zeroed by memset
- mark "in" paramater as const

Signed-off-by: Marcin Slusarz <marcin.slusarz@gmail.com>
Cc: Jan Kara <jack@suse.cz>
---
 fs/udf/unicode.c |   19 ++++++++-----------
 1 files changed, 8 insertions(+), 11 deletions(-)

diff --git a/fs/udf/unicode.c b/fs/udf/unicode.c
index f4e54e5..d068d33 100644
--- a/fs/udf/unicode.c
+++ b/fs/udf/unicode.c
@@ -248,35 +248,32 @@ error_out:
 }
 
 static int udf_CS0toNLS(struct nls_table *nls, struct ustr *utf_o,
-			struct ustr *ocu_i)
+			const struct ustr *ocu_i)
 {
-	uint8_t *ocu;
-	uint32_t c;
+	const uint8_t *ocu;
 	uint8_t cmp_id, ocu_len;
 	int i;
 
-	ocu = ocu_i->u_name;
 
 	ocu_len = ocu_i->u_len;
-	cmp_id = ocu_i->u_cmpID;
-	utf_o->u_len = 0;
-
 	if (ocu_len == 0) {
 		memset(utf_o, 0, sizeof(struct ustr));
-		utf_o->u_cmpID = 0;
-		utf_o->u_len = 0;
 		return 0;
 	}
 
-	if ((cmp_id != 8) && (cmp_id != 16)) {
+	cmp_id = ocu_i->u_cmpID;
+	if (cmp_id != 8 && cmp_id != 16) {
+		memset(utf_o, 0, sizeof(struct ustr));
 		printk(KERN_ERR "udf: unknown compression code (%d) stri=%s\n",
 		       cmp_id, ocu_i->u_name);
 		return 0;
 	}
 
+	ocu = ocu_i->u_name;
+	utf_o->u_len = 0;
 	for (i = 0; (i < ocu_len) && (utf_o->u_len <= (UDF_NAME_LEN - 3));) {
 		/* Expand OSTA compressed Unicode to Unicode */
-		c = ocu[i++];
+		uint32_t c = ocu[i++];
 		if (cmp_id == 16)
 			c = (c << 8) | ocu[i++];
 
-- 
1.5.3.7

Re: [PATCH 03/10] udf: udf_CS0toNLS cleanup

[Jan Kara]

On Wed 30-01-08 22:03:53, marcin.slusarz@gmail.com wrote:
> - fix error handling - always zero output variable
> - don't zero explicitely fields zeroed by memset
> - mark "in" paramater as const
> 
> Signed-off-by: Marcin Slusarz <marcin.slusarz@gmail.com>
> Cc: Jan Kara <jack@suse.cz>
  Acked-by: Jan Kara <jack@suse.cz>

								Honza
> ---
>  fs/udf/unicode.c |   19 ++++++++-----------
>  1 files changed, 8 insertions(+), 11 deletions(-)
> 
> diff --git a/fs/udf/unicode.c b/fs/udf/unicode.c
> index f4e54e5..d068d33 100644
> --- a/fs/udf/unicode.c
> +++ b/fs/udf/unicode.c
> @@ -248,35 +248,32 @@ error_out:
>  }
>  
>  static int udf_CS0toNLS(struct nls_table *nls, struct ustr *utf_o,
> -			struct ustr *ocu_i)
> +			const struct ustr *ocu_i)
>  {
> -	uint8_t *ocu;
> -	uint32_t c;
> +	const uint8_t *ocu;
>  	uint8_t cmp_id, ocu_len;
>  	int i;
>  
> -	ocu = ocu_i->u_name;
>  
>  	ocu_len = ocu_i->u_len;
> -	cmp_id = ocu_i->u_cmpID;
> -	utf_o->u_len = 0;
> -
>  	if (ocu_len == 0) {
>  		memset(utf_o, 0, sizeof(struct ustr));
> -		utf_o->u_cmpID = 0;
> -		utf_o->u_len = 0;
>  		return 0;
>  	}
>  
> -	if ((cmp_id != 8) && (cmp_id != 16)) {
> +	cmp_id = ocu_i->u_cmpID;
> +	if (cmp_id != 8 && cmp_id != 16) {
> +		memset(utf_o, 0, sizeof(struct ustr));
>  		printk(KERN_ERR "udf: unknown compression code (%d) stri=%s\n",
>  		       cmp_id, ocu_i->u_name);
>  		return 0;
>  	}
>  
> +	ocu = ocu_i->u_name;
> +	utf_o->u_len = 0;
>  	for (i = 0; (i < ocu_len) && (utf_o->u_len <= (UDF_NAME_LEN - 3));) {
>  		/* Expand OSTA compressed Unicode to Unicode */
> -		c = ocu[i++];
> +		uint32_t c = ocu[i++];
>  		if (cmp_id == 16)
>  			c = (c << 8) | ocu[i++];
>  
> -- 
> 1.5.3.7
> 
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

[PATCH 04/10] udf: constify crc

[marcin.slusarz]

- constify internal crc table
- mark udf_crc "in" parameter as const

Signed-off-by: Marcin Slusarz <marcin.slusarz@gmail.com>
Cc: Jan Kara <jack@suse.cz>
---
 fs/udf/crc.c     |    4 ++--
 fs/udf/udfdecl.h |    2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/udf/crc.c b/fs/udf/crc.c
index b166129..f178c63 100644
--- a/fs/udf/crc.c
+++ b/fs/udf/crc.c
@@ -23,7 +23,7 @@
 
 #include "udfdecl.h"
 
-static uint16_t crc_table[256] = {
+static const uint16_t crc_table[256] = {
 	0x0000U, 0x1021U, 0x2042U, 0x3063U, 0x4084U, 0x50a5U, 0x60c6U, 0x70e7U,
 	0x8108U, 0x9129U, 0xa14aU, 0xb16bU, 0xc18cU, 0xd1adU, 0xe1ceU, 0xf1efU,
 	0x1231U, 0x0210U, 0x3273U, 0x2252U, 0x52b5U, 0x4294U, 0x72f7U, 0x62d6U,
@@ -79,7 +79,7 @@ static uint16_t crc_table[256] = {
  *	July 21, 1997 - Andrew E. Mileski
  *	Adapted from OSTA-UDF(tm) 1.50 standard.
  */
-uint16_t udf_crc(uint8_t *data, uint32_t size, uint16_t crc)
+uint16_t udf_crc(const uint8_t *data, uint32_t size, uint16_t crc)
 {
 	while (size--)
 		crc = crc_table[(crc >> 8 ^ *(data++)) & 0xffU] ^ (crc << 8);
diff --git a/fs/udf/udfdecl.h b/fs/udf/udfdecl.h
index 0c525e8..c6c457b 100644
--- a/fs/udf/udfdecl.h
+++ b/fs/udf/udfdecl.h
@@ -192,7 +192,7 @@ extern long_ad *udf_get_filelongad(uint8_t *, int, uint32_t *, int);
 extern short_ad *udf_get_fileshortad(uint8_t *, int, uint32_t *, int);
 
 /* crc.c */
-extern uint16_t udf_crc(uint8_t *, uint32_t, uint16_t);
+extern uint16_t udf_crc(const uint8_t *, uint32_t, uint16_t);
 
 /* udftime.c */
 extern time_t *udf_stamp_to_time(time_t *, long *, kernel_timestamp);
-- 
1.5.3.7

Re: [PATCH 04/10] udf: constify crc

[Jan Kara]

On Wed 30-01-08 22:03:54, marcin.slusarz@gmail.com wrote:
> - constify internal crc table
> - mark udf_crc "in" parameter as const
  Acked-by: Jan Kara <jack@suse.cz>

								Honza

> 
> Signed-off-by: Marcin Slusarz <marcin.slusarz@gmail.com>
> Cc: Jan Kara <jack@suse.cz>
> ---
>  fs/udf/crc.c     |    4 ++--
>  fs/udf/udfdecl.h |    2 +-
>  2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/udf/crc.c b/fs/udf/crc.c
> index b166129..f178c63 100644
> --- a/fs/udf/crc.c
> +++ b/fs/udf/crc.c
> @@ -23,7 +23,7 @@
>  
>  #include "udfdecl.h"
>  
> -static uint16_t crc_table[256] = {
> +static const uint16_t crc_table[256] = {
>  	0x0000U, 0x1021U, 0x2042U, 0x3063U, 0x4084U, 0x50a5U, 0x60c6U, 0x70e7U,
>  	0x8108U, 0x9129U, 0xa14aU, 0xb16bU, 0xc18cU, 0xd1adU, 0xe1ceU, 0xf1efU,
>  	0x1231U, 0x0210U, 0x3273U, 0x2252U, 0x52b5U, 0x4294U, 0x72f7U, 0x62d6U,
> @@ -79,7 +79,7 @@ static uint16_t crc_table[256] = {
>   *	July 21, 1997 - Andrew E. Mileski
>   *	Adapted from OSTA-UDF(tm) 1.50 standard.
>   */
> -uint16_t udf_crc(uint8_t *data, uint32_t size, uint16_t crc)
> +uint16_t udf_crc(const uint8_t *data, uint32_t size, uint16_t crc)
>  {
>  	while (size--)
>  		crc = crc_table[(crc >> 8 ^ *(data++)) & 0xffU] ^ (crc << 8);
> diff --git a/fs/udf/udfdecl.h b/fs/udf/udfdecl.h
> index 0c525e8..c6c457b 100644
> --- a/fs/udf/udfdecl.h
> +++ b/fs/udf/udfdecl.h
> @@ -192,7 +192,7 @@ extern long_ad *udf_get_filelongad(uint8_t *, int, uint32_t *, int);
>  extern short_ad *udf_get_fileshortad(uint8_t *, int, uint32_t *, int);
>  
>  /* crc.c */
> -extern uint16_t udf_crc(uint8_t *, uint32_t, uint16_t);
> +extern uint16_t udf_crc(const uint8_t *, uint32_t, uint16_t);
>  
>  /* udftime.c */
>  extern time_t *udf_stamp_to_time(time_t *, long *, kernel_timestamp);
> -- 
> 1.5.3.7
> 
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

[PATCH 05/10] udf: simple cleanup of truncate.c

[marcin.slusarz]

- remove one indentation level by little code reorganization
- convert "if (smth) BUG();" to "BUG_ON(smth);"

Signed-off-by: Marcin Slusarz <marcin.slusarz@gmail.com>
Cc: Jan Kara <jack@suse.cz>
---
 fs/udf/truncate.c |   76 +++++++++++++++++++++++-----------------------------
 1 files changed, 34 insertions(+), 42 deletions(-)

diff --git a/fs/udf/truncate.c b/fs/udf/truncate.c
index fe61be1..f64f827 100644
--- a/fs/udf/truncate.c
+++ b/fs/udf/truncate.c
@@ -224,34 +224,29 @@ void udf_truncate_extents(struct inode *inode)
 				if (indirect_ext_len) {
 					/* We managed to free all extents in the
 					 * indirect extent - free it too */
-					if (!epos.bh)
-						BUG();
+					BUG_ON(!epos.bh);
 					udf_free_blocks(sb, inode, epos.block,
 							0, indirect_ext_len);
+				} else if (!epos.bh) {
+					iinfo->i_lenAlloc = lenalloc;
+					mark_inode_dirty(inode);
 				} else {
-					if (!epos.bh) {
-						iinfo->i_lenAlloc =
-								lenalloc;
-						mark_inode_dirty(inode);
-					} else {
-						struct allocExtDesc *aed =
-							(struct allocExtDesc *)
-							(epos.bh->b_data);
-						int len =
-						    sizeof(struct allocExtDesc);
+					struct allocExtDesc *aed =
+						(struct allocExtDesc *)
+						(epos.bh->b_data);
+					int len = sizeof(struct allocExtDesc);
 
-						aed->lengthAllocDescs =
-						    cpu_to_le32(lenalloc);
-						if (!UDF_QUERY_FLAG(sb,
-							UDF_FLAG_STRICT) ||
-						    sbi->s_udfrev >= 0x0201)
-							len += lenalloc;
+					aed->lengthAllocDescs =
+						cpu_to_le32(lenalloc);
+					if (!UDF_QUERY_FLAG(sb,
+						UDF_FLAG_STRICT) ||
+						sbi->s_udfrev >= 0x0201)
+						len += lenalloc;
 
-						udf_update_tag(epos.bh->b_data,
-								len);
-						mark_buffer_dirty_inode(
-								epos.bh, inode);
-					}
+					udf_update_tag(epos.bh->b_data,
+							len);
+					mark_buffer_dirty_inode(
+							epos.bh, inode);
 				}
 				brelse(epos.bh);
 				epos.offset = sizeof(struct allocExtDesc);
@@ -272,28 +267,25 @@ void udf_truncate_extents(struct inode *inode)
 		}
 
 		if (indirect_ext_len) {
-			if (!epos.bh)
-				BUG();
+			BUG_ON(!epos.bh);
 			udf_free_blocks(sb, inode, epos.block, 0,
 					indirect_ext_len);
+		} else if (!epos.bh) {
+			iinfo->i_lenAlloc = lenalloc;
+			mark_inode_dirty(inode);
 		} else {
-			if (!epos.bh) {
-				iinfo->i_lenAlloc = lenalloc;
-				mark_inode_dirty(inode);
-			} else {
-				struct allocExtDesc *aed =
-				    (struct allocExtDesc *)(epos.bh->b_data);
-				aed->lengthAllocDescs = cpu_to_le32(lenalloc);
-				if (!UDF_QUERY_FLAG(sb, UDF_FLAG_STRICT) ||
-				    sbi->s_udfrev >= 0x0201)
-					udf_update_tag(epos.bh->b_data,
-						lenalloc +
-						sizeof(struct allocExtDesc));
-				else
-					udf_update_tag(epos.bh->b_data,
-						sizeof(struct allocExtDesc));
-				mark_buffer_dirty_inode(epos.bh, inode);
-			}
+			struct allocExtDesc *aed =
+				(struct allocExtDesc *)(epos.bh->b_data);
+			aed->lengthAllocDescs = cpu_to_le32(lenalloc);
+			if (!UDF_QUERY_FLAG(sb, UDF_FLAG_STRICT) ||
+				sbi->s_udfrev >= 0x0201)
+				udf_update_tag(epos.bh->b_data,
+					lenalloc +
+					sizeof(struct allocExtDesc));
+			else
+				udf_update_tag(epos.bh->b_data,
+					sizeof(struct allocExtDesc));
+			mark_buffer_dirty_inode(epos.bh, inode);
 		}
 	} else if (inode->i_size) {
 		if (byte_offset) {
-- 
1.5.3.7

Re: [PATCH 05/10] udf: simple cleanup of truncate.c

[Jan Kara]

On Wed 30-01-08 22:03:55, marcin.slusarz@gmail.com wrote:
> - remove one indentation level by little code reorganization
> - convert "if (smth) BUG();" to "BUG_ON(smth);"
> 
> Signed-off-by: Marcin Slusarz <marcin.slusarz@gmail.com>
> Cc: Jan Kara <jack@suse.cz>
  Acked-by: Jan Kara <jack@suse.cz>

									Honza

> ---
>  fs/udf/truncate.c |   76 +++++++++++++++++++++++-----------------------------
>  1 files changed, 34 insertions(+), 42 deletions(-)
> 
> diff --git a/fs/udf/truncate.c b/fs/udf/truncate.c
> index fe61be1..f64f827 100644
> --- a/fs/udf/truncate.c
> +++ b/fs/udf/truncate.c
> @@ -224,34 +224,29 @@ void udf_truncate_extents(struct inode *inode)
>  				if (indirect_ext_len) {
>  					/* We managed to free all extents in the
>  					 * indirect extent - free it too */
> -					if (!epos.bh)
> -						BUG();
> +					BUG_ON(!epos.bh);
>  					udf_free_blocks(sb, inode, epos.block,
>  							0, indirect_ext_len);
> +				} else if (!epos.bh) {
> +					iinfo->i_lenAlloc = lenalloc;
> +					mark_inode_dirty(inode);
>  				} else {
> -					if (!epos.bh) {
> -						iinfo->i_lenAlloc =
> -								lenalloc;
> -						mark_inode_dirty(inode);
> -					} else {
> -						struct allocExtDesc *aed =
> -							(struct allocExtDesc *)
> -							(epos.bh->b_data);
> -						int len =
> -						    sizeof(struct allocExtDesc);
> +					struct allocExtDesc *aed =
> +						(struct allocExtDesc *)
> +						(epos.bh->b_data);
> +					int len = sizeof(struct allocExtDesc);
>  
> -						aed->lengthAllocDescs =
> -						    cpu_to_le32(lenalloc);
> -						if (!UDF_QUERY_FLAG(sb,
> -							UDF_FLAG_STRICT) ||
> -						    sbi->s_udfrev >= 0x0201)
> -							len += lenalloc;
> +					aed->lengthAllocDescs =
> +						cpu_to_le32(lenalloc);
> +					if (!UDF_QUERY_FLAG(sb,
> +						UDF_FLAG_STRICT) ||
> +						sbi->s_udfrev >= 0x0201)
> +						len += lenalloc;
>  
> -						udf_update_tag(epos.bh->b_data,
> -								len);
> -						mark_buffer_dirty_inode(
> -								epos.bh, inode);
> -					}
> +					udf_update_tag(epos.bh->b_data,
> +							len);
> +					mark_buffer_dirty_inode(
> +							epos.bh, inode);
>  				}
>  				brelse(epos.bh);
>  				epos.offset = sizeof(struct allocExtDesc);
> @@ -272,28 +267,25 @@ void udf_truncate_extents(struct inode *inode)
>  		}
>  
>  		if (indirect_ext_len) {
> -			if (!epos.bh)
> -				BUG();
> +			BUG_ON(!epos.bh);
>  			udf_free_blocks(sb, inode, epos.block, 0,
>  					indirect_ext_len);
> +		} else if (!epos.bh) {
> +			iinfo->i_lenAlloc = lenalloc;
> +			mark_inode_dirty(inode);
>  		} else {
> -			if (!epos.bh) {
> -				iinfo->i_lenAlloc = lenalloc;
> -				mark_inode_dirty(inode);
> -			} else {
> -				struct allocExtDesc *aed =
> -				    (struct allocExtDesc *)(epos.bh->b_data);
> -				aed->lengthAllocDescs = cpu_to_le32(lenalloc);
> -				if (!UDF_QUERY_FLAG(sb, UDF_FLAG_STRICT) ||
> -				    sbi->s_udfrev >= 0x0201)
> -					udf_update_tag(epos.bh->b_data,
> -						lenalloc +
> -						sizeof(struct allocExtDesc));
> -				else
> -					udf_update_tag(epos.bh->b_data,
> -						sizeof(struct allocExtDesc));
> -				mark_buffer_dirty_inode(epos.bh, inode);
> -			}
> +			struct allocExtDesc *aed =
> +				(struct allocExtDesc *)(epos.bh->b_data);
> +			aed->lengthAllocDescs = cpu_to_le32(lenalloc);
> +			if (!UDF_QUERY_FLAG(sb, UDF_FLAG_STRICT) ||
> +				sbi->s_udfrev >= 0x0201)
> +				udf_update_tag(epos.bh->b_data,
> +					lenalloc +
> +					sizeof(struct allocExtDesc));
> +			else
> +				udf_update_tag(epos.bh->b_data,
> +					sizeof(struct allocExtDesc));
> +			mark_buffer_dirty_inode(epos.bh, inode);
>  		}
>  	} else if (inode->i_size) {
>  		if (byte_offset) {
> -- 
> 1.5.3.7
> 
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

[PATCH 06/10] udf: truncate: create function for updating of Allocation Ext Descriptor

[marcin.slusarz]

Signed-off-by: Marcin Slusarz <marcin.slusarz@gmail.com>
Cc: Jan Kara <jack@suse.cz>
---
 fs/udf/truncate.c |   56 +++++++++++++++++++++-------------------------------
 1 files changed, 23 insertions(+), 33 deletions(-)

diff --git a/fs/udf/truncate.c b/fs/udf/truncate.c
index f64f827..eb98616 100644
--- a/fs/udf/truncate.c
+++ b/fs/udf/truncate.c
@@ -180,6 +180,24 @@ void udf_discard_prealloc(struct inode *inode)
 	brelse(epos.bh);
 }
 
+static void udf_update_alloc_ext_desc(struct inode *inode,
+				      struct extent_position *epos,
+				      u32 lenalloc)
+{
+	struct super_block *sb = inode->i_sb;
+	struct udf_sb_info *sbi = UDF_SB(sb);
+
+	struct allocExtDesc *aed = (struct allocExtDesc *) (epos->bh->b_data);
+	int len = sizeof(struct allocExtDesc);
+
+	aed->lengthAllocDescs =	cpu_to_le32(lenalloc);
+	if (!UDF_QUERY_FLAG(sb, UDF_FLAG_STRICT) || sbi->s_udfrev >= 0x0201)
+		len += lenalloc;
+
+	udf_update_tag(epos->bh->b_data, len);
+	mark_buffer_dirty_inode(epos->bh, inode);
+}
+
 void udf_truncate_extents(struct inode *inode)
 {
 	struct extent_position epos;
@@ -187,7 +205,6 @@ void udf_truncate_extents(struct inode *inode)
 	uint32_t elen, nelen = 0, indirect_ext_len = 0, lenalloc;
 	int8_t etype;
 	struct super_block *sb = inode->i_sb;
-	struct udf_sb_info *sbi = UDF_SB(sb);
 	sector_t first_block = inode->i_size >> sb->s_blocksize_bits, offset;
 	loff_t byte_offset;
 	int adsize;
@@ -230,24 +247,9 @@ void udf_truncate_extents(struct inode *inode)
 				} else if (!epos.bh) {
 					iinfo->i_lenAlloc = lenalloc;
 					mark_inode_dirty(inode);
-				} else {
-					struct allocExtDesc *aed =
-						(struct allocExtDesc *)
-						(epos.bh->b_data);
-					int len = sizeof(struct allocExtDesc);
-
-					aed->lengthAllocDescs =
-						cpu_to_le32(lenalloc);
-					if (!UDF_QUERY_FLAG(sb,
-						UDF_FLAG_STRICT) ||
-						sbi->s_udfrev >= 0x0201)
-						len += lenalloc;
-
-					udf_update_tag(epos.bh->b_data,
-							len);
-					mark_buffer_dirty_inode(
-							epos.bh, inode);
-				}
+				} else
+					udf_update_alloc_ext_desc(inode,
+							&epos, lenalloc);
 				brelse(epos.bh);
 				epos.offset = sizeof(struct allocExtDesc);
 				epos.block = eloc;
@@ -273,20 +275,8 @@ void udf_truncate_extents(struct inode *inode)
 		} else if (!epos.bh) {
 			iinfo->i_lenAlloc = lenalloc;
 			mark_inode_dirty(inode);
-		} else {
-			struct allocExtDesc *aed =
-				(struct allocExtDesc *)(epos.bh->b_data);
-			aed->lengthAllocDescs = cpu_to_le32(lenalloc);
-			if (!UDF_QUERY_FLAG(sb, UDF_FLAG_STRICT) ||
-				sbi->s_udfrev >= 0x0201)
-				udf_update_tag(epos.bh->b_data,
-					lenalloc +
-					sizeof(struct allocExtDesc));
-			else
-				udf_update_tag(epos.bh->b_data,
-					sizeof(struct allocExtDesc));
-			mark_buffer_dirty_inode(epos.bh, inode);
-		}
+		} else
+			udf_update_alloc_ext_desc(inode, &epos, lenalloc);
 	} else if (inode->i_size) {
 		if (byte_offset) {
 			kernel_long_ad extent;
-- 
1.5.3.7

Re: [PATCH 06/10] udf: truncate: create function for updating of Allocation Ext Descriptor

[Jan Kara]

On Wed 30-01-08 22:03:56, marcin.slusarz@gmail.com wrote:
> Signed-off-by: Marcin Slusarz <marcin.slusarz@gmail.com>
> Cc: Jan Kara <jack@suse.cz>
  There are at least a few other places in inode.c which do exactly what
you do in udf_update_alloc_ext_desc() so these should be converted as well.
Moreover I think that we could move all the work with allocation extent
descriptors into some helper functions but I have to think a bit (and also
read the standard) how to do that best.

								Honza

> ---
>  fs/udf/truncate.c |   56 +++++++++++++++++++++-------------------------------
>  1 files changed, 23 insertions(+), 33 deletions(-)
> 
> diff --git a/fs/udf/truncate.c b/fs/udf/truncate.c
> index f64f827..eb98616 100644
> --- a/fs/udf/truncate.c
> +++ b/fs/udf/truncate.c
> @@ -180,6 +180,24 @@ void udf_discard_prealloc(struct inode *inode)
>  	brelse(epos.bh);
>  }
>  
> +static void udf_update_alloc_ext_desc(struct inode *inode,
> +				      struct extent_position *epos,
> +				      u32 lenalloc)
> +{
> +	struct super_block *sb = inode->i_sb;
> +	struct udf_sb_info *sbi = UDF_SB(sb);
> +
> +	struct allocExtDesc *aed = (struct allocExtDesc *) (epos->bh->b_data);
> +	int len = sizeof(struct allocExtDesc);
> +
> +	aed->lengthAllocDescs =	cpu_to_le32(lenalloc);
> +	if (!UDF_QUERY_FLAG(sb, UDF_FLAG_STRICT) || sbi->s_udfrev >= 0x0201)
> +		len += lenalloc;
> +
> +	udf_update_tag(epos->bh->b_data, len);
> +	mark_buffer_dirty_inode(epos->bh, inode);
> +}
> +
>  void udf_truncate_extents(struct inode *inode)
>  {
>  	struct extent_position epos;
> @@ -187,7 +205,6 @@ void udf_truncate_extents(struct inode *inode)
>  	uint32_t elen, nelen = 0, indirect_ext_len = 0, lenalloc;
>  	int8_t etype;
>  	struct super_block *sb = inode->i_sb;
> -	struct udf_sb_info *sbi = UDF_SB(sb);
>  	sector_t first_block = inode->i_size >> sb->s_blocksize_bits, offset;
>  	loff_t byte_offset;
>  	int adsize;
> @@ -230,24 +247,9 @@ void udf_truncate_extents(struct inode *inode)
>  				} else if (!epos.bh) {
>  					iinfo->i_lenAlloc = lenalloc;
>  					mark_inode_dirty(inode);
> -				} else {
> -					struct allocExtDesc *aed =
> -						(struct allocExtDesc *)
> -						(epos.bh->b_data);
> -					int len = sizeof(struct allocExtDesc);
> -
> -					aed->lengthAllocDescs =
> -						cpu_to_le32(lenalloc);
> -					if (!UDF_QUERY_FLAG(sb,
> -						UDF_FLAG_STRICT) ||
> -						sbi->s_udfrev >= 0x0201)
> -						len += lenalloc;
> -
> -					udf_update_tag(epos.bh->b_data,
> -							len);
> -					mark_buffer_dirty_inode(
> -							epos.bh, inode);
> -				}
> +				} else
> +					udf_update_alloc_ext_desc(inode,
> +							&epos, lenalloc);
>  				brelse(epos.bh);
>  				epos.offset = sizeof(struct allocExtDesc);
>  				epos.block = eloc;
> @@ -273,20 +275,8 @@ void udf_truncate_extents(struct inode *inode)
>  		} else if (!epos.bh) {
>  			iinfo->i_lenAlloc = lenalloc;
>  			mark_inode_dirty(inode);
> -		} else {
> -			struct allocExtDesc *aed =
> -				(struct allocExtDesc *)(epos.bh->b_data);
> -			aed->lengthAllocDescs = cpu_to_le32(lenalloc);
> -			if (!UDF_QUERY_FLAG(sb, UDF_FLAG_STRICT) ||
> -				sbi->s_udfrev >= 0x0201)
> -				udf_update_tag(epos.bh->b_data,
> -					lenalloc +
> -					sizeof(struct allocExtDesc));
> -			else
> -				udf_update_tag(epos.bh->b_data,
> -					sizeof(struct allocExtDesc));
> -			mark_buffer_dirty_inode(epos.bh, inode);
> -		}
> +		} else
> +			udf_update_alloc_ext_desc(inode, &epos, lenalloc);
>  	} else if (inode->i_size) {
>  		if (byte_offset) {
>  			kernel_long_ad extent;
> -- 
> 1.5.3.7
> 
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

Re: [PATCH 06/10] udf: truncate: create function for updating of Allocation Ext Descriptor

[Marcin Slusarz]

On Thu, Jan 31, 2008 at 06:08:58PM +0100, Jan Kara wrote:
> On Wed 30-01-08 22:03:56, marcin.slusarz@gmail.com wrote:
> > Signed-off-by: Marcin Slusarz <marcin.slusarz@gmail.com>
> > Cc: Jan Kara <jack@suse.cz>
>   There are at least a few other places in inode.c which do exactly what
> you do in udf_update_alloc_ext_desc() so these should be converted as well.

I noticed that too, but it's not clear to me how to do it properly.

Marcin

[PATCH 07/10] udf: replace all adds to little endians variables with le*_add_cpu

[marcin.slusarz]

replace all:
	little_endian_variable = cpu_to_leX(leX_to_cpu(little_endian_variable) +
	                                    expression_in_cpu_byteorder);
with:
	leX_add_cpu(&little_endian_variable, expression_in_cpu_byteorder);
sparse didn't generate any new warning with this patch

Signed-off-by: Marcin Slusarz <marcin.slusarz@gmail.com
Cc: Jan Kara <jack@suse.cz>
---
 fs/udf/balloc.c |   13 ++++---------
 fs/udf/ialloc.c |   12 ++++--------
 fs/udf/inode.c  |   16 ++++------------
 3 files changed, 12 insertions(+), 29 deletions(-)

diff --git a/fs/udf/balloc.c b/fs/udf/balloc.c
index d721a1a..a95fcfe 100644
--- a/fs/udf/balloc.c
+++ b/fs/udf/balloc.c
@@ -149,8 +149,7 @@ static bool udf_add_free_space(struct udf_sb_info *sbi,
 		return false;
 
 	lvid = (struct logicalVolIntegrityDesc *)sbi->s_lvid_bh->b_data;
-	lvid->freeSpaceTable[partition] = cpu_to_le32(le32_to_cpu(
-					lvid->freeSpaceTable[partition]) + cnt);
+	le32_add_cpu(&lvid->freeSpaceTable[partition], cnt);
 	return true;
 }
 
@@ -589,10 +588,8 @@ static void udf_table_free_blocks(struct super_block *sb,
 					sptr = oepos.bh->b_data + epos.offset;
 					aed = (struct allocExtDesc *)
 						oepos.bh->b_data;
-					aed->lengthAllocDescs =
-						cpu_to_le32(le32_to_cpu(
-							aed->lengthAllocDescs) +
-								adsize);
+					le32_add_cpu(&aed->lengthAllocDescs,
+							adsize);
 				} else {
 					sptr = iinfo->i_ext.i_data +
 								epos.offset;
@@ -645,9 +642,7 @@ static void udf_table_free_blocks(struct super_block *sb,
 				mark_inode_dirty(table);
 			} else {
 				aed = (struct allocExtDesc *)epos.bh->b_data;
-				aed->lengthAllocDescs =
-					cpu_to_le32(le32_to_cpu(
-					    aed->lengthAllocDescs) + adsize);
+				le32_add_cpu(&aed->lengthAllocDescs, adsize);
 				udf_update_tag(epos.bh->b_data, epos.offset);
 				mark_buffer_dirty(epos.bh);
 			}
diff --git a/fs/udf/ialloc.c b/fs/udf/ialloc.c
index 8436031..93b40cc 100644
--- a/fs/udf/ialloc.c
+++ b/fs/udf/ialloc.c
@@ -47,11 +47,9 @@ void udf_free_inode(struct inode *inode)
 		struct logicalVolIntegrityDescImpUse *lvidiu =
 							udf_sb_lvidiu(sbi);
 		if (S_ISDIR(inode->i_mode))
-			lvidiu->numDirs =
-				cpu_to_le32(le32_to_cpu(lvidiu->numDirs) - 1);
+			le32_add_cpu(&lvidiu->numDirs, -1);
 		else
-			lvidiu->numFiles =
-				cpu_to_le32(le32_to_cpu(lvidiu->numFiles) - 1);
+			le32_add_cpu(&lvidiu->numFiles, -1);
 
 		mark_buffer_dirty(sbi->s_lvid_bh);
 	}
@@ -105,11 +103,9 @@ struct inode *udf_new_inode(struct inode *dir, int mode, int *err)
 		lvhd = (struct logicalVolHeaderDesc *)
 				(lvid->logicalVolContentsUse);
 		if (S_ISDIR(mode))
-			lvidiu->numDirs =
-				cpu_to_le32(le32_to_cpu(lvidiu->numDirs) + 1);
+			le32_add_cpu(&lvidiu->numDirs, 1);
 		else
-			lvidiu->numFiles =
-				cpu_to_le32(le32_to_cpu(lvidiu->numFiles) + 1);
+			le32_add_cpu(&lvidiu->numFiles, 1);
 		iinfo->i_unique = uniqueID = le64_to_cpu(lvhd->uniqueID);
 		if (!(++uniqueID & 0x00000000FFFFFFFFUL))
 			uniqueID += 16;
diff --git a/fs/udf/inode.c b/fs/udf/inode.c
index 3483274..68db2ea 100644
--- a/fs/udf/inode.c
+++ b/fs/udf/inode.c
@@ -1778,9 +1778,7 @@ int8_t udf_add_aext(struct inode *inode, struct extent_position *epos,
 
 			if (epos->bh) {
 				aed = (struct allocExtDesc *)epos->bh->b_data;
-				aed->lengthAllocDescs =
-					cpu_to_le32(le32_to_cpu(
-					aed->lengthAllocDescs) + adsize);
+				le32_add_cpu(&aed->lengthAllocDescs, adsize);
 			} else {
 				iinfo->i_lenAlloc += adsize;
 				mark_inode_dirty(inode);
@@ -1830,9 +1828,7 @@ int8_t udf_add_aext(struct inode *inode, struct extent_position *epos,
 		mark_inode_dirty(inode);
 	} else {
 		aed = (struct allocExtDesc *)epos->bh->b_data;
-		aed->lengthAllocDescs =
-			cpu_to_le32(le32_to_cpu(aed->lengthAllocDescs) +
-				    adsize);
+		le32_add_cpu(&aed->lengthAllocDescs, adsize);
 		if (!UDF_QUERY_FLAG(inode->i_sb, UDF_FLAG_STRICT) ||
 				UDF_SB(inode->i_sb)->s_udfrev >= 0x0201)
 			udf_update_tag(epos->bh->b_data,
@@ -2046,9 +2042,7 @@ int8_t udf_delete_aext(struct inode *inode, struct extent_position epos,
 			mark_inode_dirty(inode);
 		} else {
 			aed = (struct allocExtDesc *)oepos.bh->b_data;
-			aed->lengthAllocDescs =
-				cpu_to_le32(le32_to_cpu(aed->lengthAllocDescs) -
-					    (2 * adsize));
+			le32_add_cpu(&aed->lengthAllocDescs, -(2 * adsize));
 			if (!UDF_QUERY_FLAG(inode->i_sb, UDF_FLAG_STRICT) ||
 			    UDF_SB(inode->i_sb)->s_udfrev >= 0x0201)
 				udf_update_tag(oepos.bh->b_data,
@@ -2065,9 +2059,7 @@ int8_t udf_delete_aext(struct inode *inode, struct extent_position epos,
 			mark_inode_dirty(inode);
 		} else {
 			aed = (struct allocExtDesc *)oepos.bh->b_data;
-			aed->lengthAllocDescs =
-				cpu_to_le32(le32_to_cpu(aed->lengthAllocDescs) -
-					    adsize);
+			le32_add_cpu(&aed->lengthAllocDescs, -adsize);
 			if (!UDF_QUERY_FLAG(inode->i_sb, UDF_FLAG_STRICT) ||
 			    UDF_SB(inode->i_sb)->s_udfrev >= 0x0201)
 				udf_update_tag(oepos.bh->b_data,
-- 
1.5.3.7

Re: [PATCH 07/10] udf: replace all adds to little endians variables with le*_add_cpu

[Jan Kara]

On Wed 30-01-08 22:03:57, marcin.slusarz@gmail.com wrote:
> replace all:
> 	little_endian_variable = cpu_to_leX(leX_to_cpu(little_endian_variable) +
> 	                                    expression_in_cpu_byteorder);
> with:
> 	leX_add_cpu(&little_endian_variable, expression_in_cpu_byteorder);
> sparse didn't generate any new warning with this patch
> 
> Signed-off-by: Marcin Slusarz <marcin.slusarz@gmail.com
> Cc: Jan Kara <jack@suse.cz>
  Acked-by: Jan Kara <jack@suse.cz>

									Honza
> ---
>  fs/udf/balloc.c |   13 ++++---------
>  fs/udf/ialloc.c |   12 ++++--------
>  fs/udf/inode.c  |   16 ++++------------
>  3 files changed, 12 insertions(+), 29 deletions(-)
> 
> diff --git a/fs/udf/balloc.c b/fs/udf/balloc.c
> index d721a1a..a95fcfe 100644
> --- a/fs/udf/balloc.c
> +++ b/fs/udf/balloc.c
> @@ -149,8 +149,7 @@ static bool udf_add_free_space(struct udf_sb_info *sbi,
>  		return false;
>  
>  	lvid = (struct logicalVolIntegrityDesc *)sbi->s_lvid_bh->b_data;
> -	lvid->freeSpaceTable[partition] = cpu_to_le32(le32_to_cpu(
> -					lvid->freeSpaceTable[partition]) + cnt);
> +	le32_add_cpu(&lvid->freeSpaceTable[partition], cnt);
>  	return true;
>  }
>  
> @@ -589,10 +588,8 @@ static void udf_table_free_blocks(struct super_block *sb,
>  					sptr = oepos.bh->b_data + epos.offset;
>  					aed = (struct allocExtDesc *)
>  						oepos.bh->b_data;
> -					aed->lengthAllocDescs =
> -						cpu_to_le32(le32_to_cpu(
> -							aed->lengthAllocDescs) +
> -								adsize);
> +					le32_add_cpu(&aed->lengthAllocDescs,
> +							adsize);
>  				} else {
>  					sptr = iinfo->i_ext.i_data +
>  								epos.offset;
> @@ -645,9 +642,7 @@ static void udf_table_free_blocks(struct super_block *sb,
>  				mark_inode_dirty(table);
>  			} else {
>  				aed = (struct allocExtDesc *)epos.bh->b_data;
> -				aed->lengthAllocDescs =
> -					cpu_to_le32(le32_to_cpu(
> -					    aed->lengthAllocDescs) + adsize);
> +				le32_add_cpu(&aed->lengthAllocDescs, adsize);
>  				udf_update_tag(epos.bh->b_data, epos.offset);
>  				mark_buffer_dirty(epos.bh);
>  			}
> diff --git a/fs/udf/ialloc.c b/fs/udf/ialloc.c
> index 8436031..93b40cc 100644
> --- a/fs/udf/ialloc.c
> +++ b/fs/udf/ialloc.c
> @@ -47,11 +47,9 @@ void udf_free_inode(struct inode *inode)
>  		struct logicalVolIntegrityDescImpUse *lvidiu =
>  							udf_sb_lvidiu(sbi);
>  		if (S_ISDIR(inode->i_mode))
> -			lvidiu->numDirs =
> -				cpu_to_le32(le32_to_cpu(lvidiu->numDirs) - 1);
> +			le32_add_cpu(&lvidiu->numDirs, -1);
>  		else
> -			lvidiu->numFiles =
> -				cpu_to_le32(le32_to_cpu(lvidiu->numFiles) - 1);
> +			le32_add_cpu(&lvidiu->numFiles, -1);
>  
>  		mark_buffer_dirty(sbi->s_lvid_bh);
>  	}
> @@ -105,11 +103,9 @@ struct inode *udf_new_inode(struct inode *dir, int mode, int *err)
>  		lvhd = (struct logicalVolHeaderDesc *)
>  				(lvid->logicalVolContentsUse);
>  		if (S_ISDIR(mode))
> -			lvidiu->numDirs =
> -				cpu_to_le32(le32_to_cpu(lvidiu->numDirs) + 1);
> +			le32_add_cpu(&lvidiu->numDirs, 1);
>  		else
> -			lvidiu->numFiles =
> -				cpu_to_le32(le32_to_cpu(lvidiu->numFiles) + 1);
> +			le32_add_cpu(&lvidiu->numFiles, 1);
>  		iinfo->i_unique = uniqueID = le64_to_cpu(lvhd->uniqueID);
>  		if (!(++uniqueID & 0x00000000FFFFFFFFUL))
>  			uniqueID += 16;
> diff --git a/fs/udf/inode.c b/fs/udf/inode.c
> index 3483274..68db2ea 100644
> --- a/fs/udf/inode.c
> +++ b/fs/udf/inode.c
> @@ -1778,9 +1778,7 @@ int8_t udf_add_aext(struct inode *inode, struct extent_position *epos,
>  
>  			if (epos->bh) {
>  				aed = (struct allocExtDesc *)epos->bh->b_data;
> -				aed->lengthAllocDescs =
> -					cpu_to_le32(le32_to_cpu(
> -					aed->lengthAllocDescs) + adsize);
> +				le32_add_cpu(&aed->lengthAllocDescs, adsize);
>  			} else {
>  				iinfo->i_lenAlloc += adsize;
>  				mark_inode_dirty(inode);
> @@ -1830,9 +1828,7 @@ int8_t udf_add_aext(struct inode *inode, struct extent_position *epos,
>  		mark_inode_dirty(inode);
>  	} else {
>  		aed = (struct allocExtDesc *)epos->bh->b_data;
> -		aed->lengthAllocDescs =
> -			cpu_to_le32(le32_to_cpu(aed->lengthAllocDescs) +
> -				    adsize);
> +		le32_add_cpu(&aed->lengthAllocDescs, adsize);
>  		if (!UDF_QUERY_FLAG(inode->i_sb, UDF_FLAG_STRICT) ||
>  				UDF_SB(inode->i_sb)->s_udfrev >= 0x0201)
>  			udf_update_tag(epos->bh->b_data,
> @@ -2046,9 +2042,7 @@ int8_t udf_delete_aext(struct inode *inode, struct extent_position epos,
>  			mark_inode_dirty(inode);
>  		} else {
>  			aed = (struct allocExtDesc *)oepos.bh->b_data;
> -			aed->lengthAllocDescs =
> -				cpu_to_le32(le32_to_cpu(aed->lengthAllocDescs) -
> -					    (2 * adsize));
> +			le32_add_cpu(&aed->lengthAllocDescs, -(2 * adsize));
>  			if (!UDF_QUERY_FLAG(inode->i_sb, UDF_FLAG_STRICT) ||
>  			    UDF_SB(inode->i_sb)->s_udfrev >= 0x0201)
>  				udf_update_tag(oepos.bh->b_data,
> @@ -2065,9 +2059,7 @@ int8_t udf_delete_aext(struct inode *inode, struct extent_position epos,
>  			mark_inode_dirty(inode);
>  		} else {
>  			aed = (struct allocExtDesc *)oepos.bh->b_data;
> -			aed->lengthAllocDescs =
> -				cpu_to_le32(le32_to_cpu(aed->lengthAllocDescs) -
> -					    adsize);
> +			le32_add_cpu(&aed->lengthAllocDescs, -adsize);
>  			if (!UDF_QUERY_FLAG(inode->i_sb, UDF_FLAG_STRICT) ||
>  			    UDF_SB(inode->i_sb)->s_udfrev >= 0x0201)
>  				udf_update_tag(oepos.bh->b_data,
> -- 
> 1.5.3.7
> 
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

[PATCH 08/10] udf: simplify __udf_read_inode

[marcin.slusarz]

- move all brelse(ibh) after main if, because it's called
  on every path except one where ibh is null
- move variables to the most inner blocks

Signed-off-by: Marcin Slusarz <marcin.slusarz@gmail.com>
Cc: Jan Kara <jack@suse.cz>
---
 fs/udf/inode.c |   52 +++++++++++++++++++++++-----------------------------
 1 files changed, 23 insertions(+), 29 deletions(-)

diff --git a/fs/udf/inode.c b/fs/udf/inode.c
index 68db2ea..c2d0477 100644
--- a/fs/udf/inode.c
+++ b/fs/udf/inode.c
@@ -1116,42 +1116,36 @@ static void __udf_read_inode(struct inode *inode)
 	fe = (struct fileEntry *)bh->b_data;
 
 	if (fe->icbTag.strategyType == cpu_to_le16(4096)) {
-		struct buffer_head *ibh = NULL, *nbh = NULL;
-		struct indirectEntry *ie;
+		struct buffer_head *ibh;
 
 		ibh = udf_read_ptagged(inode->i_sb, iinfo->i_location, 1,
 					&ident);
-		if (ident == TAG_IDENT_IE) {
-			if (ibh) {
-				kernel_lb_addr loc;
-				ie = (struct indirectEntry *)ibh->b_data;
-
-				loc = lelb_to_cpu(ie->indirectICB.extLocation);
-
-				if (ie->indirectICB.extLength &&
-				    (nbh = udf_read_ptagged(inode->i_sb, loc, 0,
-							    &ident))) {
-					if (ident == TAG_IDENT_FE ||
-					    ident == TAG_IDENT_EFE) {
-						memcpy(&iinfo->i_location,
-						       &loc,
-						       sizeof(kernel_lb_addr));
-						brelse(bh);
-						brelse(ibh);
-						brelse(nbh);
-						__udf_read_inode(inode);
-						return;
-					} else {
-						brelse(nbh);
-						brelse(ibh);
-					}
-				} else {
+		if (ident == TAG_IDENT_IE && ibh) {
+			struct buffer_head *nbh = NULL;
+			kernel_lb_addr loc;
+			struct indirectEntry *ie;
+
+			ie = (struct indirectEntry *)ibh->b_data;
+			loc = lelb_to_cpu(ie->indirectICB.extLocation);
+
+			if (ie->indirectICB.extLength &&
+				(nbh = udf_read_ptagged(inode->i_sb, loc, 0,
+							&ident))) {
+				if (ident == TAG_IDENT_FE ||
+					ident == TAG_IDENT_EFE) {
+					memcpy(&iinfo->i_location,
+						&loc,
+						sizeof(kernel_lb_addr));
+					brelse(bh);
 					brelse(ibh);
+					brelse(nbh);
+					__udf_read_inode(inode);
+					return;
 				}
+				brelse(nbh);
 			}
-		} else {
-			brelse(ibh);
 		}
+		brelse(ibh);
 	} else if (fe->icbTag.strategyType != cpu_to_le16(4)) {
 		printk(KERN_ERR "udf: unsupported strategy type: %d\n",
 		       le16_to_cpu(fe->icbTag.strategyType));
-- 
1.5.3.7

Re: [PATCH 08/10] udf: simplify __udf_read_inode

[Jan Kara]

On Wed 30-01-08 22:03:58, marcin.slusarz@gmail.com wrote:
> - move all brelse(ibh) after main if, because it's called
>   on every path except one where ibh is null
> - move variables to the most inner blocks
> 
> Signed-off-by: Marcin Slusarz <marcin.slusarz@gmail.com>
> Cc: Jan Kara <jack@suse.cz>
  Acked-by: Jan Kara <jack@suse.cz>

									Honza

> ---
>  fs/udf/inode.c |   52 +++++++++++++++++++++++-----------------------------
>  1 files changed, 23 insertions(+), 29 deletions(-)
> 
> diff --git a/fs/udf/inode.c b/fs/udf/inode.c
> index 68db2ea..c2d0477 100644
> --- a/fs/udf/inode.c
> +++ b/fs/udf/inode.c
> @@ -1116,42 +1116,36 @@ static void __udf_read_inode(struct inode *inode)
>  	fe = (struct fileEntry *)bh->b_data;
>  
>  	if (fe->icbTag.strategyType == cpu_to_le16(4096)) {
> -		struct buffer_head *ibh = NULL, *nbh = NULL;
> -		struct indirectEntry *ie;
> +		struct buffer_head *ibh;
>  
>  		ibh = udf_read_ptagged(inode->i_sb, iinfo->i_location, 1,
>  					&ident);
> -		if (ident == TAG_IDENT_IE) {
> -			if (ibh) {
> -				kernel_lb_addr loc;
> -				ie = (struct indirectEntry *)ibh->b_data;
> -
> -				loc = lelb_to_cpu(ie->indirectICB.extLocation);
> -
> -				if (ie->indirectICB.extLength &&
> -				    (nbh = udf_read_ptagged(inode->i_sb, loc, 0,
> -							    &ident))) {
> -					if (ident == TAG_IDENT_FE ||
> -					    ident == TAG_IDENT_EFE) {
> -						memcpy(&iinfo->i_location,
> -						       &loc,
> -						       sizeof(kernel_lb_addr));
> -						brelse(bh);
> -						brelse(ibh);
> -						brelse(nbh);
> -						__udf_read_inode(inode);
> -						return;
> -					} else {
> -						brelse(nbh);
> -						brelse(ibh);
> -					}
> -				} else {
> +		if (ident == TAG_IDENT_IE && ibh) {
> +			struct buffer_head *nbh = NULL;
> +			kernel_lb_addr loc;
> +			struct indirectEntry *ie;
> +
> +			ie = (struct indirectEntry *)ibh->b_data;
> +			loc = lelb_to_cpu(ie->indirectICB.extLocation);
> +
> +			if (ie->indirectICB.extLength &&
> +				(nbh = udf_read_ptagged(inode->i_sb, loc, 0,
> +							&ident))) {
> +				if (ident == TAG_IDENT_FE ||
> +					ident == TAG_IDENT_EFE) {
> +					memcpy(&iinfo->i_location,
> +						&loc,
> +						sizeof(kernel_lb_addr));
> +					brelse(bh);
>  					brelse(ibh);
> +					brelse(nbh);
> +					__udf_read_inode(inode);
> +					return;
>  				}
> +				brelse(nbh);
>  			}
> -		} else {
> -			brelse(ibh);
>  		}
> +		brelse(ibh);
>  	} else if (fe->icbTag.strategyType != cpu_to_le16(4)) {
>  		printk(KERN_ERR "udf: unsupported strategy type: %d\n",
>  		       le16_to_cpu(fe->icbTag.strategyType));
> -- 
> 1.5.3.7
> 
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

[PATCH 09/10] udf: replace udf_*_offset macros with functions

[marcin.slusarz]

- translate udf_file_entry_alloc_offset macro into function
- translate udf_ext0_offset macro into function
- add comment about crypticly named fields in struct udf_inode_info

Signed-off-by: Marcin Slusarz <marcin.slusarz@gmail.com>
Cc: Jan Kara <jack@suse.cz>
---
 fs/udf/udfdecl.h         |   29 +++++++++++++++++++----------
 include/linux/udf_fs_i.h |    4 ++--
 2 files changed, 21 insertions(+), 12 deletions(-)

diff --git a/fs/udf/udfdecl.h b/fs/udf/udfdecl.h
index c6c457b..375be1b 100644
--- a/fs/udf/udfdecl.h
+++ b/fs/udf/udfdecl.h
@@ -12,6 +12,7 @@
 #include <linux/buffer_head.h>
 
 #include "udfend.h"
+#include "udf_i.h"
 
 #define udf_fixed_to_variable(x) ( ( ( (x) >> 5 ) * 39 ) + ( (x) & 0x0000001F ) )
 #define udf_variable_to_fixed(x) ( ( ( (x) / 39 ) << 5 ) + ( (x) % 39 ) )
@@ -23,16 +24,24 @@
 #define UDF_NAME_LEN		256
 #define UDF_PATH_LEN		1023
 
-#define udf_file_entry_alloc_offset(inode)\
-	(UDF_I(inode)->i_use ?\
-		sizeof(struct unallocSpaceEntry) :\
-		((UDF_I(inode)->i_efe ?\
-			sizeof(struct extendedFileEntry) :\
-			sizeof(struct fileEntry)) + UDF_I(inode)->i_lenEAttr))
-
-#define udf_ext0_offset(inode)\
-	(UDF_I(inode)->i_alloc_type == ICBTAG_FLAG_AD_IN_ICB ?\
-		udf_file_entry_alloc_offset(inode) : 0)
+static inline size_t udf_file_entry_alloc_offset(struct inode *inode)
+{
+	struct udf_inode_info *iinfo = UDF_I(inode);
+	if (iinfo->i_use)
+		return sizeof(struct unallocSpaceEntry);
+	else if (iinfo->i_efe)
+		return sizeof(struct extendedFileEntry) + iinfo->i_lenEAttr;
+	else
+		return sizeof(struct fileEntry) + iinfo->i_lenEAttr;
+}
+
+static inline size_t udf_ext0_offset(struct inode *inode)
+{
+	if (UDF_I(inode)->i_alloc_type == ICBTAG_FLAG_AD_IN_ICB)
+		return udf_file_entry_alloc_offset(inode);
+	else
+		return 0;
+}
 
 #define udf_get_lb_pblock(sb,loc,offset) udf_get_pblock((sb), (loc).logicalBlockNum, (loc).partitionReferenceNum, (offset))
 
diff --git a/include/linux/udf_fs_i.h b/include/linux/udf_fs_i.h
index ffaf056..6281a52 100644
--- a/include/linux/udf_fs_i.h
+++ b/include/linux/udf_fs_i.h
@@ -27,8 +27,8 @@ struct udf_inode_info
 	__u32			i_next_alloc_block;
 	__u32			i_next_alloc_goal;
 	unsigned		i_alloc_type : 3;
-	unsigned		i_efe : 1;
-	unsigned		i_use : 1;
+	unsigned		i_efe : 1; /* extendedFileEntry */
+	unsigned		i_use : 1; /* unallocSpaceEntry */
 	unsigned		i_strat4096 : 1;
 	unsigned		reserved : 26;
 	union
-- 
1.5.3.7

Re: [PATCH 09/10] udf: replace udf_*_offset macros with functions

[Jan Kara]

On Wed 30-01-08 22:03:59, marcin.slusarz@gmail.com wrote:
> - translate udf_file_entry_alloc_offset macro into function
> - translate udf_ext0_offset macro into function
> - add comment about crypticly named fields in struct udf_inode_info
> 
> Signed-off-by: Marcin Slusarz <marcin.slusarz@gmail.com>
> Cc: Jan Kara <jack@suse.cz>
  Acked-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/udf/udfdecl.h         |   29 +++++++++++++++++++----------
>  include/linux/udf_fs_i.h |    4 ++--
>  2 files changed, 21 insertions(+), 12 deletions(-)
> 
> diff --git a/fs/udf/udfdecl.h b/fs/udf/udfdecl.h
> index c6c457b..375be1b 100644
> --- a/fs/udf/udfdecl.h
> +++ b/fs/udf/udfdecl.h
> @@ -12,6 +12,7 @@
>  #include <linux/buffer_head.h>
>  
>  #include "udfend.h"
> +#include "udf_i.h"
>  
>  #define udf_fixed_to_variable(x) ( ( ( (x) >> 5 ) * 39 ) + ( (x) & 0x0000001F ) )
>  #define udf_variable_to_fixed(x) ( ( ( (x) / 39 ) << 5 ) + ( (x) % 39 ) )
> @@ -23,16 +24,24 @@
>  #define UDF_NAME_LEN		256
>  #define UDF_PATH_LEN		1023
>  
> -#define udf_file_entry_alloc_offset(inode)\
> -	(UDF_I(inode)->i_use ?\
> -		sizeof(struct unallocSpaceEntry) :\
> -		((UDF_I(inode)->i_efe ?\
> -			sizeof(struct extendedFileEntry) :\
> -			sizeof(struct fileEntry)) + UDF_I(inode)->i_lenEAttr))
> -
> -#define udf_ext0_offset(inode)\
> -	(UDF_I(inode)->i_alloc_type == ICBTAG_FLAG_AD_IN_ICB ?\
> -		udf_file_entry_alloc_offset(inode) : 0)
> +static inline size_t udf_file_entry_alloc_offset(struct inode *inode)
> +{
> +	struct udf_inode_info *iinfo = UDF_I(inode);
> +	if (iinfo->i_use)
> +		return sizeof(struct unallocSpaceEntry);
> +	else if (iinfo->i_efe)
> +		return sizeof(struct extendedFileEntry) + iinfo->i_lenEAttr;
> +	else
> +		return sizeof(struct fileEntry) + iinfo->i_lenEAttr;
> +}
> +
> +static inline size_t udf_ext0_offset(struct inode *inode)
> +{
> +	if (UDF_I(inode)->i_alloc_type == ICBTAG_FLAG_AD_IN_ICB)
> +		return udf_file_entry_alloc_offset(inode);
> +	else
> +		return 0;
> +}
>  
>  #define udf_get_lb_pblock(sb,loc,offset) udf_get_pblock((sb), (loc).logicalBlockNum, (loc).partitionReferenceNum, (offset))
>  
> diff --git a/include/linux/udf_fs_i.h b/include/linux/udf_fs_i.h
> index ffaf056..6281a52 100644
> --- a/include/linux/udf_fs_i.h
> +++ b/include/linux/udf_fs_i.h
> @@ -27,8 +27,8 @@ struct udf_inode_info
>  	__u32			i_next_alloc_block;
>  	__u32			i_next_alloc_goal;
>  	unsigned		i_alloc_type : 3;
> -	unsigned		i_efe : 1;
> -	unsigned		i_use : 1;
> +	unsigned		i_efe : 1; /* extendedFileEntry */
> +	unsigned		i_use : 1; /* unallocSpaceEntry */
>  	unsigned		i_strat4096 : 1;
>  	unsigned		reserved : 26;
>  	union
> -- 
> 1.5.3.7
> 
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

[PATCH 10/10] udf: constify udf_bitmap_lookup array

[marcin.slusarz]

udf_bitmap_lookup never changes, so constify it

Signed-off-by: Marcin Slusarz <marcin.slusarz@gmail.com>
Cc: Jan Kara <jack@suse.cz>
---
 fs/udf/super.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/fs/udf/super.c b/fs/udf/super.c
index 3afe764..6bb2a5b 100644
--- a/fs/udf/super.c
+++ b/fs/udf/super.c
@@ -1969,7 +1969,7 @@ static int udf_statfs(struct dentry *dentry, struct kstatfs *buf)
 	return 0;
 }
 
-static unsigned char udf_bitmap_lookup[16] = {
+static const unsigned char udf_bitmap_lookup[16] = {
 	0, 1, 1, 2, 1, 2, 2, 3, 1, 2, 2, 3, 2, 3, 3, 4
 };
 
-- 
1.5.3.7

Re: [PATCH 10/10] udf: constify udf_bitmap_lookup array

[Jan Kara]

On Wed 30-01-08 22:04:00, marcin.slusarz@gmail.com wrote:
> udf_bitmap_lookup never changes, so constify it
> 
> Signed-off-by: Marcin Slusarz <marcin.slusarz@gmail.com>
> Cc: Jan Kara <jack@suse.cz>
  Hmm, rather than doing this, could you change the function to use
standard functions for counting set bits? I guess bitmap_weight() in
include/linux/bitmap.h should be what we need... Thanks.

								Honza

> ---
>  fs/udf/super.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/fs/udf/super.c b/fs/udf/super.c
> index 3afe764..6bb2a5b 100644
> --- a/fs/udf/super.c
> +++ b/fs/udf/super.c
> @@ -1969,7 +1969,7 @@ static int udf_statfs(struct dentry *dentry, struct kstatfs *buf)
>  	return 0;
>  }
>  
> -static unsigned char udf_bitmap_lookup[16] = {
> +static const unsigned char udf_bitmap_lookup[16] = {
>  	0, 1, 1, 2, 1, 2, 2, 3, 1, 2, 2, 3, 2, 3, 3, 4
>  };
>  
> -- 
> 1.5.3.7
> 
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

Re: [PATCH 10/10] udf: constify udf_bitmap_lookup array

[Marcin Slusarz]

On Thu, Jan 31, 2008 at 05:52:44PM +0100, Jan Kara wrote:
> On Wed 30-01-08 22:04:00, marcin.slusarz@gmail.com wrote:
> > udf_bitmap_lookup never changes, so constify it
> > 
> > Signed-off-by: Marcin Slusarz <marcin.slusarz@gmail.com>
> > Cc: Jan Kara <jack@suse.cz>
>   Hmm, rather than doing this, could you change the function to use
> standard functions for counting set bits? I guess bitmap_weight() in
> include/linux/bitmap.h should be what we need... Thanks.

---
udf: convert udf_count_free_bitmap to use bitmap_weight

replace handwritten bits counting with bitmap_weight

Signed-off-by: Marcin Slusarz <marcin.slusarz@gmail.com>
Cc: Jan Kara <jack@suse.cz>
---
 fs/udf/super.c |   17 +++++------------
 1 files changed, 5 insertions(+), 12 deletions(-)

diff --git a/fs/udf/super.c b/fs/udf/super.c
index 3afe764..0dcee12 100644
--- a/fs/udf/super.c
+++ b/fs/udf/super.c
@@ -53,6 +53,7 @@
 #include <linux/vfs.h>
 #include <linux/vmalloc.h>
 #include <linux/errno.h>
+#include <linux/bitmap.h>
 #include <asm/byteorder.h>
 
 #include <linux/udf_fs.h>
@@ -1969,10 +1970,6 @@ static int udf_statfs(struct dentry *dentry, struct kstatfs *buf)
 	return 0;
 }
 
-static unsigned char udf_bitmap_lookup[16] = {
-	0, 1, 1, 2, 1, 2, 2, 3, 1, 2, 2, 3, 2, 3, 3, 4
-};
-
 static unsigned int udf_count_free_bitmap(struct super_block *sb,
 					  struct udf_bitmap *bitmap)
 {
@@ -1982,7 +1979,6 @@ static unsigned int udf_count_free_bitmap(struct super_block *sb,
 	int block = 0, newblock;
 	kernel_lb_addr loc;
 	uint32_t bytes;
-	uint8_t value;
 	uint8_t *ptr;
 	uint16_t ident;
 	struct spaceBitmapDesc *bm;
@@ -2008,13 +2004,10 @@ static unsigned int udf_count_free_bitmap(struct super_block *sb,
 	ptr = (uint8_t *)bh->b_data;
 
 	while (bytes > 0) {
-		while ((bytes > 0) && (index < sb->s_blocksize)) {
-			value = ptr[index];
-			accum += udf_bitmap_lookup[value & 0x0f];
-			accum += udf_bitmap_lookup[value >> 4];
-			index++;
-			bytes--;
-		}
+		u32 cur_bytes = min_t(u32, bytes, sb->s_blocksize - index);
+		accum += bitmap_weight((const unsigned long *)(ptr + index),
+					cur_bytes * 8);
+		bytes -= cur_bytes;
 		if (bytes) {
 			brelse(bh);
 			newblock = udf_get_lb_pblock(sb, loc, ++block);
-- 
1.5.3.7

Re: [PATCH 10/10] udf: constify udf_bitmap_lookup array

[Jan Kara]

On Sat 02-02-08 22:37:07, Marcin Slusarz wrote:
> On Thu, Jan 31, 2008 at 05:52:44PM +0100, Jan Kara wrote:
> > On Wed 30-01-08 22:04:00, marcin.slusarz@gmail.com wrote:
> > > udf_bitmap_lookup never changes, so constify it
> > > 
> > > Signed-off-by: Marcin Slusarz <marcin.slusarz@gmail.com>
> > > Cc: Jan Kara <jack@suse.cz>
> >   Hmm, rather than doing this, could you change the function to use
> > standard functions for counting set bits? I guess bitmap_weight() in
> > include/linux/bitmap.h should be what we need... Thanks.
> 
> ---
> udf: convert udf_count_free_bitmap to use bitmap_weight
> 
> replace handwritten bits counting with bitmap_weight
> 
> Signed-off-by: Marcin Slusarz <marcin.slusarz@gmail.com>
> Cc: Jan Kara <jack@suse.cz>
  Acked-by: Jan Kara <jack@suse.cz>

								Honza
> ---
>  fs/udf/super.c |   17 +++++------------
>  1 files changed, 5 insertions(+), 12 deletions(-)
> 
> diff --git a/fs/udf/super.c b/fs/udf/super.c
> index 3afe764..0dcee12 100644
> --- a/fs/udf/super.c
> +++ b/fs/udf/super.c
> @@ -53,6 +53,7 @@
>  #include <linux/vfs.h>
>  #include <linux/vmalloc.h>
>  #include <linux/errno.h>
> +#include <linux/bitmap.h>
>  #include <asm/byteorder.h>
>  
>  #include <linux/udf_fs.h>
> @@ -1969,10 +1970,6 @@ static int udf_statfs(struct dentry *dentry, struct kstatfs *buf)
>  	return 0;
>  }
>  
> -static unsigned char udf_bitmap_lookup[16] = {
> -	0, 1, 1, 2, 1, 2, 2, 3, 1, 2, 2, 3, 2, 3, 3, 4
> -};
> -
>  static unsigned int udf_count_free_bitmap(struct super_block *sb,
>  					  struct udf_bitmap *bitmap)
>  {
> @@ -1982,7 +1979,6 @@ static unsigned int udf_count_free_bitmap(struct super_block *sb,
>  	int block = 0, newblock;
>  	kernel_lb_addr loc;
>  	uint32_t bytes;
> -	uint8_t value;
>  	uint8_t *ptr;
>  	uint16_t ident;
>  	struct spaceBitmapDesc *bm;
> @@ -2008,13 +2004,10 @@ static unsigned int udf_count_free_bitmap(struct super_block *sb,
>  	ptr = (uint8_t *)bh->b_data;
>  
>  	while (bytes > 0) {
> -		while ((bytes > 0) && (index < sb->s_blocksize)) {
> -			value = ptr[index];
> -			accum += udf_bitmap_lookup[value & 0x0f];
> -			accum += udf_bitmap_lookup[value >> 4];
> -			index++;
> -			bytes--;
> -		}
> +		u32 cur_bytes = min_t(u32, bytes, sb->s_blocksize - index);
> +		accum += bitmap_weight((const unsigned long *)(ptr + index),
> +					cur_bytes * 8);
> +		bytes -= cur_bytes;
>  		if (bytes) {
>  			brelse(bh);
>  			newblock = udf_get_lb_pblock(sb, loc, ++block);
> -- 
> 1.5.3.7
> 
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR