LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH 00/10] udf: cleanups
@ 2008-01-30 21:03 marcin.slusarz
2008-01-30 21:03 ` [PATCH 01/10] udf: udf_CS0toUTF8 cleanup marcin.slusarz
` (9 more replies)
0 siblings, 10 replies; 29+ messages in thread
From: marcin.slusarz @ 2008-01-30 21:03 UTC (permalink / raw)
To: LKML; +Cc: Jan Kara, 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
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH 01/10] udf: udf_CS0toUTF8 cleanup
2008-01-30 21:03 [PATCH 00/10] udf: cleanups marcin.slusarz
@ 2008-01-30 21:03 ` marcin.slusarz
2008-01-31 9:57 ` Jan Kara
2008-01-30 21:03 ` [PATCH 02/10] udf: fix udf_build_ustr marcin.slusarz
` (8 subsequent siblings)
9 siblings, 1 reply; 29+ messages in thread
From: marcin.slusarz @ 2008-01-30 21:03 UTC (permalink / raw)
To: LKML; +Cc: Jan Kara, 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
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH 02/10] udf: fix udf_build_ustr
2008-01-30 21:03 [PATCH 00/10] udf: cleanups marcin.slusarz
2008-01-30 21:03 ` [PATCH 01/10] udf: udf_CS0toUTF8 cleanup marcin.slusarz
@ 2008-01-30 21:03 ` marcin.slusarz
2008-01-31 10:45 ` Jan Kara
2008-01-30 21:03 ` [PATCH 03/10] udf: udf_CS0toNLS cleanup marcin.slusarz
` (7 subsequent siblings)
9 siblings, 1 reply; 29+ messages in thread
From: marcin.slusarz @ 2008-01-30 21:03 UTC (permalink / raw)
To: LKML; +Cc: Jan Kara, 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
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH 03/10] udf: udf_CS0toNLS cleanup
2008-01-30 21:03 [PATCH 00/10] udf: cleanups marcin.slusarz
2008-01-30 21:03 ` [PATCH 01/10] udf: udf_CS0toUTF8 cleanup marcin.slusarz
2008-01-30 21:03 ` [PATCH 02/10] udf: fix udf_build_ustr marcin.slusarz
@ 2008-01-30 21:03 ` marcin.slusarz
2008-01-31 12:23 ` Jan Kara
2008-01-30 21:03 ` [PATCH 04/10] udf: constify crc marcin.slusarz
` (6 subsequent siblings)
9 siblings, 1 reply; 29+ messages in thread
From: marcin.slusarz @ 2008-01-30 21:03 UTC (permalink / raw)
To: LKML; +Cc: Jan Kara, 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
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH 04/10] udf: constify crc
2008-01-30 21:03 [PATCH 00/10] udf: cleanups marcin.slusarz
` (2 preceding siblings ...)
2008-01-30 21:03 ` [PATCH 03/10] udf: udf_CS0toNLS cleanup marcin.slusarz
@ 2008-01-30 21:03 ` marcin.slusarz
2008-01-31 12:58 ` Jan Kara
2008-01-30 21:03 ` [PATCH 05/10] udf: simple cleanup of truncate.c marcin.slusarz
` (5 subsequent siblings)
9 siblings, 1 reply; 29+ messages in thread
From: marcin.slusarz @ 2008-01-30 21:03 UTC (permalink / raw)
To: LKML; +Cc: Jan Kara, 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
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH 05/10] udf: simple cleanup of truncate.c
2008-01-30 21:03 [PATCH 00/10] udf: cleanups marcin.slusarz
` (3 preceding siblings ...)
2008-01-30 21:03 ` [PATCH 04/10] udf: constify crc marcin.slusarz
@ 2008-01-30 21:03 ` marcin.slusarz
2008-01-31 13:01 ` Jan Kara
2008-01-30 21:03 ` [PATCH 06/10] udf: truncate: create function for updating of Allocation Ext Descriptor marcin.slusarz
` (4 subsequent siblings)
9 siblings, 1 reply; 29+ messages in thread
From: marcin.slusarz @ 2008-01-30 21:03 UTC (permalink / raw)
To: LKML; +Cc: Jan Kara, 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
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH 06/10] udf: truncate: create function for updating of Allocation Ext Descriptor
2008-01-30 21:03 [PATCH 00/10] udf: cleanups marcin.slusarz
` (4 preceding siblings ...)
2008-01-30 21:03 ` [PATCH 05/10] udf: simple cleanup of truncate.c marcin.slusarz
@ 2008-01-30 21:03 ` marcin.slusarz
2008-01-31 17:08 ` Jan Kara
2008-01-30 21:03 ` [PATCH 07/10] udf: replace all adds to little endians variables with le*_add_cpu marcin.slusarz
` (3 subsequent siblings)
9 siblings, 1 reply; 29+ messages in thread
From: marcin.slusarz @ 2008-01-30 21:03 UTC (permalink / raw)
To: LKML; +Cc: Jan Kara, 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
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH 07/10] udf: replace all adds to little endians variables with le*_add_cpu
2008-01-30 21:03 [PATCH 00/10] udf: cleanups marcin.slusarz
` (5 preceding siblings ...)
2008-01-30 21:03 ` [PATCH 06/10] udf: truncate: create function for updating of Allocation Ext Descriptor marcin.slusarz
@ 2008-01-30 21:03 ` marcin.slusarz
2008-01-31 16:42 ` Jan Kara
2008-01-30 21:03 ` [PATCH 08/10] udf: simplify __udf_read_inode marcin.slusarz
` (2 subsequent siblings)
9 siblings, 1 reply; 29+ messages in thread
From: marcin.slusarz @ 2008-01-30 21:03 UTC (permalink / raw)
To: LKML; +Cc: Jan Kara, 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
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH 08/10] udf: simplify __udf_read_inode
2008-01-30 21:03 [PATCH 00/10] udf: cleanups marcin.slusarz
` (6 preceding siblings ...)
2008-01-30 21:03 ` [PATCH 07/10] udf: replace all adds to little endians variables with le*_add_cpu marcin.slusarz
@ 2008-01-30 21:03 ` marcin.slusarz
2008-01-31 16:46 ` Jan Kara
2008-01-30 21:03 ` [PATCH 09/10] udf: replace udf_*_offset macros with functions marcin.slusarz
2008-01-30 21:04 ` [PATCH 10/10] udf: constify udf_bitmap_lookup array marcin.slusarz
9 siblings, 1 reply; 29+ messages in thread
From: marcin.slusarz @ 2008-01-30 21:03 UTC (permalink / raw)
To: LKML; +Cc: Jan Kara, 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
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH 09/10] udf: replace udf_*_offset macros with functions
2008-01-30 21:03 [PATCH 00/10] udf: cleanups marcin.slusarz
` (7 preceding siblings ...)
2008-01-30 21:03 ` [PATCH 08/10] udf: simplify __udf_read_inode marcin.slusarz
@ 2008-01-30 21:03 ` marcin.slusarz
2008-01-31 16:48 ` Jan Kara
2008-01-30 21:04 ` [PATCH 10/10] udf: constify udf_bitmap_lookup array marcin.slusarz
9 siblings, 1 reply; 29+ messages in thread
From: marcin.slusarz @ 2008-01-30 21:03 UTC (permalink / raw)
To: LKML; +Cc: Jan Kara, 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
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH 10/10] udf: constify udf_bitmap_lookup array
2008-01-30 21:03 [PATCH 00/10] udf: cleanups marcin.slusarz
` (8 preceding siblings ...)
2008-01-30 21:03 ` [PATCH 09/10] udf: replace udf_*_offset macros with functions marcin.slusarz
@ 2008-01-30 21:04 ` marcin.slusarz
2008-01-31 16:52 ` Jan Kara
9 siblings, 1 reply; 29+ messages in thread
From: marcin.slusarz @ 2008-01-30 21:04 UTC (permalink / raw)
To: LKML; +Cc: Jan Kara, 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
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 01/10] udf: udf_CS0toUTF8 cleanup
2008-01-30 21:03 ` [PATCH 01/10] udf: udf_CS0toUTF8 cleanup marcin.slusarz
@ 2008-01-31 9:57 ` Jan Kara
0 siblings, 0 replies; 29+ messages in thread
From: Jan Kara @ 2008-01-31 9:57 UTC (permalink / raw)
To: marcin.slusarz; +Cc: LKML
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
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 02/10] udf: fix udf_build_ustr
2008-01-30 21:03 ` [PATCH 02/10] udf: fix udf_build_ustr marcin.slusarz
@ 2008-01-31 10:45 ` Jan Kara
2008-01-31 19:57 ` Marcin Slusarz
0 siblings, 1 reply; 29+ messages in thread
From: Jan Kara @ 2008-01-31 10:45 UTC (permalink / raw)
To: marcin.slusarz; +Cc: LKML
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
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 03/10] udf: udf_CS0toNLS cleanup
2008-01-30 21:03 ` [PATCH 03/10] udf: udf_CS0toNLS cleanup marcin.slusarz
@ 2008-01-31 12:23 ` Jan Kara
0 siblings, 0 replies; 29+ messages in thread
From: Jan Kara @ 2008-01-31 12:23 UTC (permalink / raw)
To: marcin.slusarz; +Cc: LKML
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
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 04/10] udf: constify crc
2008-01-30 21:03 ` [PATCH 04/10] udf: constify crc marcin.slusarz
@ 2008-01-31 12:58 ` Jan Kara
0 siblings, 0 replies; 29+ messages in thread
From: Jan Kara @ 2008-01-31 12:58 UTC (permalink / raw)
To: marcin.slusarz; +Cc: LKML
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
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 05/10] udf: simple cleanup of truncate.c
2008-01-30 21:03 ` [PATCH 05/10] udf: simple cleanup of truncate.c marcin.slusarz
@ 2008-01-31 13:01 ` Jan Kara
0 siblings, 0 replies; 29+ messages in thread
From: Jan Kara @ 2008-01-31 13:01 UTC (permalink / raw)
To: marcin.slusarz; +Cc: LKML
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
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 07/10] udf: replace all adds to little endians variables with le*_add_cpu
2008-01-30 21:03 ` [PATCH 07/10] udf: replace all adds to little endians variables with le*_add_cpu marcin.slusarz
@ 2008-01-31 16:42 ` Jan Kara
0 siblings, 0 replies; 29+ messages in thread
From: Jan Kara @ 2008-01-31 16:42 UTC (permalink / raw)
To: marcin.slusarz; +Cc: LKML
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
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 08/10] udf: simplify __udf_read_inode
2008-01-30 21:03 ` [PATCH 08/10] udf: simplify __udf_read_inode marcin.slusarz
@ 2008-01-31 16:46 ` Jan Kara
0 siblings, 0 replies; 29+ messages in thread
From: Jan Kara @ 2008-01-31 16:46 UTC (permalink / raw)
To: marcin.slusarz; +Cc: LKML
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
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 09/10] udf: replace udf_*_offset macros with functions
2008-01-30 21:03 ` [PATCH 09/10] udf: replace udf_*_offset macros with functions marcin.slusarz
@ 2008-01-31 16:48 ` Jan Kara
0 siblings, 0 replies; 29+ messages in thread
From: Jan Kara @ 2008-01-31 16:48 UTC (permalink / raw)
To: marcin.slusarz; +Cc: LKML
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
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 10/10] udf: constify udf_bitmap_lookup array
2008-01-30 21:04 ` [PATCH 10/10] udf: constify udf_bitmap_lookup array marcin.slusarz
@ 2008-01-31 16:52 ` Jan Kara
2008-02-02 21:37 ` Marcin Slusarz
0 siblings, 1 reply; 29+ messages in thread
From: Jan Kara @ 2008-01-31 16:52 UTC (permalink / raw)
To: marcin.slusarz; +Cc: LKML
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
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 06/10] udf: truncate: create function for updating of Allocation Ext Descriptor
2008-01-30 21:03 ` [PATCH 06/10] udf: truncate: create function for updating of Allocation Ext Descriptor marcin.slusarz
@ 2008-01-31 17:08 ` Jan Kara
2008-01-31 18:18 ` Marcin Slusarz
0 siblings, 1 reply; 29+ messages in thread
From: Jan Kara @ 2008-01-31 17:08 UTC (permalink / raw)
To: marcin.slusarz; +Cc: LKML
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
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 06/10] udf: truncate: create function for updating of Allocation Ext Descriptor
2008-01-31 17:08 ` Jan Kara
@ 2008-01-31 18:18 ` Marcin Slusarz
0 siblings, 0 replies; 29+ messages in thread
From: Marcin Slusarz @ 2008-01-31 18:18 UTC (permalink / raw)
To: Jan Kara; +Cc: LKML
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
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 02/10] udf: fix udf_build_ustr
2008-01-31 10:45 ` Jan Kara
@ 2008-01-31 19:57 ` Marcin Slusarz
2008-02-04 19:31 ` Jan Kara
0 siblings, 1 reply; 29+ messages in thread
From: Marcin Slusarz @ 2008-01-31 19:57 UTC (permalink / raw)
To: Jan Kara; +Cc: LKML
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
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 10/10] udf: constify udf_bitmap_lookup array
2008-01-31 16:52 ` Jan Kara
@ 2008-02-02 21:37 ` Marcin Slusarz
2008-02-04 19:15 ` Jan Kara
0 siblings, 1 reply; 29+ messages in thread
From: Marcin Slusarz @ 2008-02-02 21:37 UTC (permalink / raw)
To: Jan Kara; +Cc: LKML
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
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 10/10] udf: constify udf_bitmap_lookup array
2008-02-02 21:37 ` Marcin Slusarz
@ 2008-02-04 19:15 ` Jan Kara
0 siblings, 0 replies; 29+ messages in thread
From: Jan Kara @ 2008-02-04 19:15 UTC (permalink / raw)
To: Marcin Slusarz; +Cc: LKML
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
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 02/10] udf: fix udf_build_ustr
2008-01-31 19:57 ` Marcin Slusarz
@ 2008-02-04 19:31 ` Jan Kara
2008-02-04 21:27 ` Marcin Slusarz
0 siblings, 1 reply; 29+ messages in thread
From: Jan Kara @ 2008-02-04 19:31 UTC (permalink / raw)
To: Marcin Slusarz; +Cc: LKML
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
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 02/10] udf: fix udf_build_ustr
2008-02-04 19:31 ` Jan Kara
@ 2008-02-04 21:27 ` Marcin Slusarz
2008-02-05 15:29 ` Jan Kara
0 siblings, 1 reply; 29+ messages in thread
From: Marcin Slusarz @ 2008-02-04 21:27 UTC (permalink / raw)
To: Jan Kara; +Cc: LKML
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
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 02/10] udf: fix udf_build_ustr
2008-02-04 21:27 ` Marcin Slusarz
@ 2008-02-05 15:29 ` Jan Kara
2008-02-05 19:17 ` Marcin Slusarz
0 siblings, 1 reply; 29+ messages in thread
From: Jan Kara @ 2008-02-05 15:29 UTC (permalink / raw)
To: Marcin Slusarz; +Cc: LKML
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
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 02/10] udf: fix udf_build_ustr
2008-02-05 15:29 ` Jan Kara
@ 2008-02-05 19:17 ` Marcin Slusarz
0 siblings, 0 replies; 29+ messages in thread
From: Marcin Slusarz @ 2008-02-05 19:17 UTC (permalink / raw)
To: Jan Kara; +Cc: LKML
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
^ permalink raw reply [flat|nested] 29+ messages in thread
end of thread, other threads:[~2008-02-05 19:18 UTC | newest]
Thread overview: 29+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2008-01-30 21:03 [PATCH 00/10] udf: cleanups marcin.slusarz
2008-01-30 21:03 ` [PATCH 01/10] udf: udf_CS0toUTF8 cleanup marcin.slusarz
2008-01-31 9:57 ` Jan Kara
2008-01-30 21:03 ` [PATCH 02/10] udf: fix udf_build_ustr marcin.slusarz
2008-01-31 10:45 ` Jan Kara
2008-01-31 19:57 ` Marcin Slusarz
2008-02-04 19:31 ` Jan Kara
2008-02-04 21:27 ` Marcin Slusarz
2008-02-05 15:29 ` Jan Kara
2008-02-05 19:17 ` Marcin Slusarz
2008-01-30 21:03 ` [PATCH 03/10] udf: udf_CS0toNLS cleanup marcin.slusarz
2008-01-31 12:23 ` Jan Kara
2008-01-30 21:03 ` [PATCH 04/10] udf: constify crc marcin.slusarz
2008-01-31 12:58 ` Jan Kara
2008-01-30 21:03 ` [PATCH 05/10] udf: simple cleanup of truncate.c marcin.slusarz
2008-01-31 13:01 ` Jan Kara
2008-01-30 21:03 ` [PATCH 06/10] udf: truncate: create function for updating of Allocation Ext Descriptor marcin.slusarz
2008-01-31 17:08 ` Jan Kara
2008-01-31 18:18 ` Marcin Slusarz
2008-01-30 21:03 ` [PATCH 07/10] udf: replace all adds to little endians variables with le*_add_cpu marcin.slusarz
2008-01-31 16:42 ` Jan Kara
2008-01-30 21:03 ` [PATCH 08/10] udf: simplify __udf_read_inode marcin.slusarz
2008-01-31 16:46 ` Jan Kara
2008-01-30 21:03 ` [PATCH 09/10] udf: replace udf_*_offset macros with functions marcin.slusarz
2008-01-31 16:48 ` Jan Kara
2008-01-30 21:04 ` [PATCH 10/10] udf: constify udf_bitmap_lookup array marcin.slusarz
2008-01-31 16:52 ` Jan Kara
2008-02-02 21:37 ` Marcin Slusarz
2008-02-04 19:15 ` Jan Kara
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).