[PATCH] UDF: check for allocated memory for inode data

[PATCH] UDF: check for allocated memory for inode data

[Cyrill Gorcunov]

This patch adds cheking for granted memory while
filling up inode data to prevent possible NULL
pointer usage. If there is not enough memory to
fill inode data we just mark it as "bad".

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>

---

Please check the patch, maybe just marking inode as
"bad" is not a good solution.

 fs/udf/inode.c |   27 ++++++++++++++++++++++++---
 1 files changed, 24 insertions(+), 3 deletions(-)

diff --git a/fs/udf/inode.c b/fs/udf/inode.c
index c846155..91cddae 100644
--- a/fs/udf/inode.c
+++ b/fs/udf/inode.c
@@ -1144,6 +1144,13 @@ static void udf_fill_inode(struct inode *inode, struct buffer_head *bh)
 		UDF_I_EFE(inode) = 1;
 		UDF_I_USE(inode) = 0;
 		UDF_I_DATA(inode) = kmalloc(inode->i_sb->s_blocksize - sizeof(struct extendedFileEntry), GFP_KERNEL);
+		if (!UDF_I_DATA(inode))
+		{
+			printk(KERN_ERR "udf: udf_fill_inode(ino %ld) no free memory\n",
+			       inode->i_ino);
+			make_bad_inode(inode);
+			return;
+		}
 		memcpy(UDF_I_DATA(inode), bh->b_data + sizeof(struct extendedFileEntry), inode->i_sb->s_blocksize - sizeof(struct extendedFileEntry));
 	}
 	else if (le16_to_cpu(fe->descTag.tagIdent) == TAG_IDENT_FE)
@@ -1151,6 +1158,13 @@ static void udf_fill_inode(struct inode *inode, struct buffer_head *bh)
 		UDF_I_EFE(inode) = 0;
 		UDF_I_USE(inode) = 0;
 		UDF_I_DATA(inode) = kmalloc(inode->i_sb->s_blocksize - sizeof(struct fileEntry), GFP_KERNEL);
+		if (!UDF_I_DATA(inode))
+		{
+			printk(KERN_ERR "udf: udf_fill_inode(ino %ld) no free memory\n",
+			       inode->i_ino);
+			make_bad_inode(inode);
+			return;
+		}
 		memcpy(UDF_I_DATA(inode), bh->b_data + sizeof(struct fileEntry), inode->i_sb->s_blocksize - sizeof(struct fileEntry));
 	}
 	else if (le16_to_cpu(fe->descTag.tagIdent) == TAG_IDENT_USE)
@@ -1161,6 +1175,13 @@ static void udf_fill_inode(struct inode *inode, struct buffer_head *bh)
 			le32_to_cpu(
 				((struct unallocSpaceEntry *)bh->b_data)->lengthAllocDescs);
 		UDF_I_DATA(inode) = kmalloc(inode->i_sb->s_blocksize - sizeof(struct unallocSpaceEntry), GFP_KERNEL);
+		if (!UDF_I_DATA(inode))
+		{
+			printk(KERN_ERR "udf: udf_fill_inode(ino %ld) no free memory\n",
+			       inode->i_ino);
+			make_bad_inode(inode);
+			return;
+		}
 		memcpy(UDF_I_DATA(inode), bh->b_data + sizeof(struct unallocSpaceEntry), inode->i_sb->s_blocksize - sizeof(struct unallocSpaceEntry));
 		return;
 	}
@@ -1178,7 +1199,7 @@ static void udf_fill_inode(struct inode *inode, struct buffer_head *bh)
 	inode->i_nlink = le16_to_cpu(fe->fileLinkCount);
 	if (!inode->i_nlink)
 		inode->i_nlink = 1;
-	
+
 	inode->i_size = le64_to_cpu(fe->informationLength);
 	UDF_I_LENEXTENTS(inode) = inode->i_size;
 
@@ -1230,7 +1251,7 @@ static void udf_fill_inode(struct inode *inode, struct buffer_head *bh)
 	}
 	else
 	{
-		inode->i_blocks = le64_to_cpu(efe->logicalBlocksRecorded) << 
+		inode->i_blocks = le64_to_cpu(efe->logicalBlocksRecorded) <<
 			(inode->i_sb->s_blocksize_bits - 9);
 
 		if ( udf_stamp_to_time(&convtime, &convtime_usec,
@@ -2059,7 +2080,7 @@ int8_t udf_delete_aext(struct inode *inode, struct extent_position epos,
 			mark_buffer_dirty_inode(oepos.bh, inode);
 		}
 	}
-	
+
 	brelse(epos.bh);
 	brelse(oepos.bh);
 	return (elen >> 30);

Re: [PATCH] UDF: check for allocated memory for inode data

[Andrew Morton]

On Thu, 10 May 2007 18:00:00 +0400
Cyrill Gorcunov <gorcunov@gmail.com> wrote:

> This patch adds cheking for granted memory while
> filling up inode data to prevent possible NULL
> pointer usage. If there is not enough memory to
> fill inode data we just mark it as "bad".
> 
> Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
> 
> Please check the patch, maybe just marking inode as
> "bad" is not a good solution.
> 

yes, make_bad_inode() is appropriate here.

> 
> diff --git a/fs/udf/inode.c b/fs/udf/inode.c
> index c846155..91cddae 100644
> --- a/fs/udf/inode.c
> +++ b/fs/udf/inode.c
> @@ -1144,6 +1144,13 @@ static void udf_fill_inode(struct inode *inode, struct buffer_head *bh)
>  		UDF_I_EFE(inode) = 1;
>  		UDF_I_USE(inode) = 0;
>  		UDF_I_DATA(inode) = kmalloc(inode->i_sb->s_blocksize - sizeof(struct extendedFileEntry), GFP_KERNEL);
> +		if (!UDF_I_DATA(inode))
> +		{
> +			printk(KERN_ERR "udf: udf_fill_inode(ino %ld) no free memory\n",
> +			       inode->i_ino);
> +			make_bad_inode(inode);
> +			return;
> +		}

But please let's not add three copies of identical code.  Do something like:

static int udf_check_inode(struct inode *inode)
{
	if (!UDF_I_DATA(inode)) {
		printk(KERN_ERR "udf: udf_fill_inode(ino %ld) no free memory\n",
			inode->i_ino);
		make_bad_inode(inode);
		return -1;
	}
	return 0;
}


	if (udf_check_inode(inode))
		return;

In fact you can also do the kmalloc in that helper function too:

static int udf_alloc_i_data(struct inode *inode, size_t size)
{
	UDF_I_DATA(inode) = kmalloc(...);
	...
}

Re: [PATCH] UDF: check for allocated memory for inode data

[Cyrill Gorcunov]

[Andrew Morton - Thu, May 10, 2007 at 03:46:40PM -0700]

[...snip...] 

| But please let's not add three copies of identical code.  Do something like:

[...snip...]

Thanks for comments, Andrew. Let me rewrite the patch...

		Cyrill

Re: [PATCH] UDF: check for allocated memory for inode data

[Christoph Hellwig]

On Thu, May 10, 2007 at 03:46:40PM -0700, Andrew Morton wrote:
> On Thu, 10 May 2007 18:00:00 +0400
> Cyrill Gorcunov <gorcunov@gmail.com> wrote:
> 
> > This patch adds cheking for granted memory while
> > filling up inode data to prevent possible NULL
> > pointer usage. If there is not enough memory to
> > fill inode data we just mark it as "bad".
> > 
> > Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
> > 
> > Please check the patch, maybe just marking inode as
> > "bad" is not a good solution.
> > 
> 
> yes, make_bad_inode() is appropriate here.
> 
> > 
> > diff --git a/fs/udf/inode.c b/fs/udf/inode.c
> > index c846155..91cddae 100644
> > --- a/fs/udf/inode.c
> > +++ b/fs/udf/inode.c
> > @@ -1144,6 +1144,13 @@ static void udf_fill_inode(struct inode *inode, struct buffer_head *bh)
> >  		UDF_I_EFE(inode) = 1;
> >  		UDF_I_USE(inode) = 0;
> >  		UDF_I_DATA(inode) = kmalloc(inode->i_sb->s_blocksize - sizeof(struct extendedFileEntry), GFP_KERNEL);
> > +		if (!UDF_I_DATA(inode))
> > +		{
> > +			printk(KERN_ERR "udf: udf_fill_inode(ino %ld) no free memory\n",
> > +			       inode->i_ino);
> > +			make_bad_inode(inode);
> > +			return;
> > +		}
> 
> But please let's not add three copies of identical code.  Do something like:
> 
> static int udf_check_inode(struct inode *inode)
> {
> 	if (!UDF_I_DATA(inode)) {
> 		printk(KERN_ERR "udf: udf_fill_inode(ino %ld) no free memory\n",
> 			inode->i_ino);
> 		make_bad_inode(inode);
> 		return -1;
> 	}
> 	return 0;
> }
> 
> 
> 	if (udf_check_inode(inode))
> 		return;
> 
> In fact you can also do the kmalloc in that helper function too:
> 
> static int udf_alloc_i_data(struct inode *inode, size_t size)
> {
> 	UDF_I_DATA(inode) = kmalloc(...);
> 	...
> }

And please get rid of the UDF_I_* macro for everything you touch, just
put a

	struct udf_inode_info *uip = UDF_I(inode);

at the beginning of the function and use the fields directly.

Re: [PATCH] UDF: check for allocated memory for inode data

[Cyrill Gorcunov]

[Christoph Hellwig - Fri, May 11, 2007 at 08:29:39AM +0100]
| On Thu, May 10, 2007 at 03:46:40PM -0700, Andrew Morton wrote:
| > On Thu, 10 May 2007 18:00:00 +0400
| > Cyrill Gorcunov <gorcunov@gmail.com> wrote:
| > 
| > > This patch adds cheking for granted memory while
| > > filling up inode data to prevent possible NULL
| > > pointer usage. If there is not enough memory to
| > > fill inode data we just mark it as "bad".
| > > 
| > > Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
| > > 
| > > Please check the patch, maybe just marking inode as
| > > "bad" is not a good solution.
| > > 
| > 
| > yes, make_bad_inode() is appropriate here.
| > 
| > > 
| > > diff --git a/fs/udf/inode.c b/fs/udf/inode.c
| > > index c846155..91cddae 100644
| > > --- a/fs/udf/inode.c
| > > +++ b/fs/udf/inode.c
| > > @@ -1144,6 +1144,13 @@ static void udf_fill_inode(struct inode *inode, struct buffer_head *bh)
| > >  		UDF_I_EFE(inode) = 1;
| > >  		UDF_I_USE(inode) = 0;
| > >  		UDF_I_DATA(inode) = kmalloc(inode->i_sb->s_blocksize - sizeof(struct extendedFileEntry), GFP_KERNEL);
| > > +		if (!UDF_I_DATA(inode))
| > > +		{
| > > +			printk(KERN_ERR "udf: udf_fill_inode(ino %ld) no free memory\n",
| > > +			       inode->i_ino);
| > > +			make_bad_inode(inode);
| > > +			return;
| > > +		}
| > 
| > But please let's not add three copies of identical code.  Do something like:
| > 
| > static int udf_check_inode(struct inode *inode)
| > {
| > 	if (!UDF_I_DATA(inode)) {
| > 		printk(KERN_ERR "udf: udf_fill_inode(ino %ld) no free memory\n",
| > 			inode->i_ino);
| > 		make_bad_inode(inode);
| > 		return -1;
| > 	}
| > 	return 0;
| > }
| > 
| > 
| > 	if (udf_check_inode(inode))
| > 		return;
| > 
| > In fact you can also do the kmalloc in that helper function too:
| > 
| > static int udf_alloc_i_data(struct inode *inode, size_t size)
| > {
| > 	UDF_I_DATA(inode) = kmalloc(...);
| > 	...
| > }
| 
| And please get rid of the UDF_I_* macro for everything you touch, just
| put a
| 
| 	struct udf_inode_info *uip = UDF_I(inode);
| 
| at the beginning of the function and use the fields directly.
| 

OK, I've already sent v.2 of the patch, so v.3 will become soon ;)

		Cyrill

Re: [PATCH] UDF: check for allocated memory for inode data

[Cyrill Gorcunov]

[Christoph Hellwig - Fri, May 11, 2007 at 08:29:39AM +0100]
[..snip..] 
| And please get rid of the UDF_I_* macro for everything you touch, just
| put a
| 
| 	struct udf_inode_info *uip = UDF_I(inode);
| 
| at the beginning of the function and use the fields directly.
| 

Christoph, I think there should be a separated patch to remove
UDF_I_* macro. I'll make it ;) 

Andrew, may be I should produce a series of patch:
	- 1st to check the memory
	- 2nd to get rid of UDF_I_* macro
?

		Cyrill

Re: [PATCH] UDF: check for allocated memory for inode data

[Cyrill Gorcunov]

[Christoph Hellwig - Fri, May 11, 2007 at 08:29:39AM +0100]

...

| And please get rid of the UDF_I_* macro for everything you touch, just
| put a
| 
| 	struct udf_inode_info *uip = UDF_I(inode);
| 
| at the beginning of the function and use the fields directly.
| 

Actually to properly remove UDF_I* and UDF_SB_* macroses in the
whole UDF subsystem - is _lot_ of work. I'm going to make it but
not now (too busy).

		Cyrill

Re: [PATCH] UDF: check for allocated memory for inode data

[Christoph Hellwig]

On Fri, May 11, 2007 at 01:01:27PM +0400, Cyrill Gorcunov wrote:
> [Christoph Hellwig - Fri, May 11, 2007 at 08:29:39AM +0100]
> 
> ...
> 
> | And please get rid of the UDF_I_* macro for everything you touch, just
> | put a
> | 
> | 	struct udf_inode_info *uip = UDF_I(inode);
> | 
> | at the beginning of the function and use the fields directly.
> | 
> 
> Actually to properly remove UDF_I* and UDF_SB_* macroses in the
> whole UDF subsystem - is _lot_ of work. I'm going to make it but
> not now (too busy).

Doing it completely is a lot of work, yes.  I was more thinking of
converting a piece of code once you do major changes.  But if you
want to convert all the code as a separate patch I'm more than happy
aswell.

Re: [PATCH] UDF: check for allocated memory for inode data

[Cyrill Gorcunov]

[Christoph Hellwig - Fri, May 11, 2007 at 11:39:56AM +0100]
| On Fri, May 11, 2007 at 01:01:27PM +0400, Cyrill Gorcunov wrote:
| > [Christoph Hellwig - Fri, May 11, 2007 at 08:29:39AM +0100]
| > 
| > ...
| > 
| > | And please get rid of the UDF_I_* macro for everything you touch, just
| > | put a
| > | 
| > | 	struct udf_inode_info *uip = UDF_I(inode);
| > | 
| > | at the beginning of the function and use the fields directly.
| > | 
| > 
| > Actually to properly remove UDF_I* and UDF_SB_* macroses in the
| > whole UDF subsystem - is _lot_ of work. I'm going to make it but
| > not now (too busy).
| 
| Doing it completely is a lot of work, yes.  I was more thinking of
| converting a piece of code once you do major changes.  But if you
| want to convert all the code as a separate patch I'm more than happy
| aswell.
| 

Christoph, my only argue against getting rid of UDF_I_* macro in
my patch is UDF coding style, I don't want to damage it. I think
we may leave it as is (including my patch). So just review the patch
I sent (second version) and Ack it then so Andrew could include it
into mm tree. Meantime I'm rewritting the whole UDF subsystem to
get rid of that macroses (it will be a long way ;)

		Cyrill

P.S.
But if you still insist on getting rid of UDF_I_ macroses from my patch
just let me now :)

Re: [PATCH] UDF: check for allocated memory for inode data

[Cyrill Gorcunov]

Christoph,

you know I've got a question (may be it's stupid) - what a
sense to discard UDF_I_* macroses? I mean as I see they
don't slow down execution of the code...

		Cyrill

Re: [PATCH] UDF: check for allocated memory for inode data

[Pekka Enberg]

Hi Cyrill

On 5/12/07, Cyrill Gorcunov <gorcunov@gmail.com> wrote:
> you know I've got a question (may be it's stupid) - what a
> sense to discard UDF_I_* macroses? I mean as I see they
> don't slow down execution of the code...

They make the code harder to read and maintain.

Re: [PATCH] UDF: check for allocated memory for inode data

[Cyrill Gorcunov]

[Pekka Enberg - Sat, May 12, 2007 at 01:15:14PM +0300]
| Hi Cyrill
| 
| On 5/12/07, Cyrill Gorcunov <gorcunov@gmail.com> wrote:
| >you know I've got a question (may be it's stupid) - what a
| >sense to discard UDF_I_* macroses? I mean as I see they
| >don't slow down execution of the code...
| 
| They make the code harder to read and maintain.
| 

Hi Pekka,

thanks for eplanation. Btw, maybe I also should
convert the UDF coding style to Linux kernel coding
style?

		Cyrill

P.S.
It's strange that I haven't got any comments from
Ben Fennema :(

Re: [PATCH] UDF: check for allocated memory for inode data

[Christoph Hellwig]

On Fri, May 11, 2007 at 03:09:20PM +0400, Cyrill Gorcunov wrote:
> | > | And please get rid of the UDF_I_* macro for everything you touch, just
> | > | put a
> | > | 
> | > | 	struct udf_inode_info *uip = UDF_I(inode);
> | > | 
> | > | at the beginning of the function and use the fields directly.
> | > | 
> | > 
> | > Actually to properly remove UDF_I* and UDF_SB_* macroses in the
> | > whole UDF subsystem - is _lot_ of work. I'm going to make it but
> | > not now (too busy).
> | 
> | Doing it completely is a lot of work, yes.  I was more thinking of
> | converting a piece of code once you do major changes.  But if you
> | want to convert all the code as a separate patch I'm more than happy
> | aswell.
> | 
> 
> Christoph, my only argue against getting rid of UDF_I_* macro in
> my patch is UDF coding style, I don't want to damage it. I think
> we may leave it as is (including my patch). So just review the patch
> I sent (second version) and Ack it then so Andrew could include it
> into mm tree. Meantime I'm rewritting the whole UDF subsystem to
> get rid of that macroses (it will be a long way ;)

The UDF style is horrible and very unlike other kernel code.  Given
that udf has been pretty much unmtained for a while there should be
nothing in the way of fixing it.

Anyway, the patch is technically correct so you'll get my ACK (not
that you should need it).

Re: [PATCH] UDF: check for allocated memory for inode data

[Cyrill Gorcunov]

[Christoph Hellwig - Sun, May 13, 2007 at 10:01:26PM +0100]
| On Fri, May 11, 2007 at 03:09:20PM +0400, Cyrill Gorcunov wrote:
| > | > | And please get rid of the UDF_I_* macro for everything you touch, just
| > | > | put a
| > | > | 
| > | > | 	struct udf_inode_info *uip = UDF_I(inode);
| > | > | 
| > | > | at the beginning of the function and use the fields directly.
| > | > | 
| > | > 
| > | > Actually to properly remove UDF_I* and UDF_SB_* macroses in the
| > | > whole UDF subsystem - is _lot_ of work. I'm going to make it but
| > | > not now (too busy).
| > | 
| > | Doing it completely is a lot of work, yes.  I was more thinking of
| > | converting a piece of code once you do major changes.  But if you
| > | want to convert all the code as a separate patch I'm more than happy
| > | aswell.
| > | 
| > 
| > Christoph, my only argue against getting rid of UDF_I_* macro in
| > my patch is UDF coding style, I don't want to damage it. I think
| > we may leave it as is (including my patch). So just review the patch
| > I sent (second version) and Ack it then so Andrew could include it
| > into mm tree. Meantime I'm rewritting the whole UDF subsystem to
| > get rid of that macroses (it will be a long way ;)
| 
| The UDF style is horrible and very unlike other kernel code.  Given
| that udf has been pretty much unmtained for a while there should be
| nothing in the way of fixing it.
| 
| Anyway, the patch is technically correct so you'll get my ACK (not
| that you should need it).
| 

Hi Christoph,

you know I've read UDF sources. As I understand all UDF_I_ macroses
could be converted without breaking UDF state but... as you exactly
mentoined it's style is horrible and I'm thinking about rewritting the
whole UDF system. Unfortunelly I'm not _mature_ kernel developer (I'm kernel
newbie) and it could take a long time for this (I think something like
~ 3 month or more ;). Actually I'm ready to spend my free time for
this. So how do you think could it be reasonable?

		Cyrill

Re: [PATCH] UDF: check for allocated memory for inode data

[Jan Kara]

> [Christoph Hellwig - Sun, May 13, 2007 at 10:01:26PM +0100]
> | On Fri, May 11, 2007 at 03:09:20PM +0400, Cyrill Gorcunov wrote:
> | > | > | And please get rid of the UDF_I_* macro for everything you touch, just
> | > | > | put a
> | > | > | 
> | > | > | 	struct udf_inode_info *uip = UDF_I(inode);
> | > | > | 
> | > | > | at the beginning of the function and use the fields directly.
> | > | > | 
> | > | > 
> | > | > Actually to properly remove UDF_I* and UDF_SB_* macroses in the
> | > | > whole UDF subsystem - is _lot_ of work. I'm going to make it but
> | > | > not now (too busy).
> | > | 
> | > | Doing it completely is a lot of work, yes.  I was more thinking of
> | > | converting a piece of code once you do major changes.  But if you
> | > | want to convert all the code as a separate patch I'm more than happy
> | > | aswell.
> | > | 
> | > 
> | > Christoph, my only argue against getting rid of UDF_I_* macro in
> | > my patch is UDF coding style, I don't want to damage it. I think
> | > we may leave it as is (including my patch). So just review the patch
> | > I sent (second version) and Ack it then so Andrew could include it
> | > into mm tree. Meantime I'm rewritting the whole UDF subsystem to
> | > get rid of that macroses (it will be a long way ;)
> | 
> | The UDF style is horrible and very unlike other kernel code.  Given
> | that udf has been pretty much unmtained for a while there should be
> | nothing in the way of fixing it.
> | 
> | Anyway, the patch is technically correct so you'll get my ACK (not
> | that you should need it).
> | 
> 
> you know I've read UDF sources. As I understand all UDF_I_ macroses
> could be converted without breaking UDF state but... as you exactly
> mentoined it's style is horrible and I'm thinking about rewritting the
> whole UDF system. Unfortunelly I'm not _mature_ kernel developer (I'm kernel
> newbie) and it could take a long time for this (I think something like
> ~ 3 month or more ;). Actually I'm ready to spend my free time for
> this. So how do you think could it be reasonable?
  I've spent some time hunting bugs in UDF recently so I'll warn you a
bit :). Definitely rewriting that ... code would be a good thing to do
(reading that code I had urges to do it several times). The hard thing
is that there is no reasonable spec you could use - there are two
documents which define how UDF should look like but they are really hard
to read (they have like hundred pages each and one does not make sence
without the other). And reading the code and learning how the filesystem is
supposed to work isn't too helpful either. Just a friendly warning ;)

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

Re: [PATCH] UDF: check for allocated memory for inode data

[Cyrill Gorcunov]

[Jan Kara - Wed, May 16, 2007 at 07:38:52PM +0200]
| > [Christoph Hellwig - Sun, May 13, 2007 at 10:01:26PM +0100]
| > | On Fri, May 11, 2007 at 03:09:20PM +0400, Cyrill Gorcunov wrote:
| > | > | > | And please get rid of the UDF_I_* macro for everything you touch, just
| > | > | > | put a
| > | > | > | 
| > | > | > | 	struct udf_inode_info *uip = UDF_I(inode);
| > | > | > | 
| > | > | > | at the beginning of the function and use the fields directly.
| > | > | > | 
| > | > | > 
| > | > | > Actually to properly remove UDF_I* and UDF_SB_* macroses in the
| > | > | > whole UDF subsystem - is _lot_ of work. I'm going to make it but
| > | > | > not now (too busy).
| > | > | 
| > | > | Doing it completely is a lot of work, yes.  I was more thinking of
| > | > | converting a piece of code once you do major changes.  But if you
| > | > | want to convert all the code as a separate patch I'm more than happy
| > | > | aswell.
| > | > | 
| > | > 
| > | > Christoph, my only argue against getting rid of UDF_I_* macro in
| > | > my patch is UDF coding style, I don't want to damage it. I think
| > | > we may leave it as is (including my patch). So just review the patch
| > | > I sent (second version) and Ack it then so Andrew could include it
| > | > into mm tree. Meantime I'm rewritting the whole UDF subsystem to
| > | > get rid of that macroses (it will be a long way ;)
| > | 
| > | The UDF style is horrible and very unlike other kernel code.  Given
| > | that udf has been pretty much unmtained for a while there should be
| > | nothing in the way of fixing it.
| > | 
| > | Anyway, the patch is technically correct so you'll get my ACK (not
| > | that you should need it).
| > | 
| > 
| > you know I've read UDF sources. As I understand all UDF_I_ macroses
| > could be converted without breaking UDF state but... as you exactly
| > mentoined it's style is horrible and I'm thinking about rewritting the
| > whole UDF system. Unfortunelly I'm not _mature_ kernel developer (I'm kernel
| > newbie) and it could take a long time for this (I think something like
| > ~ 3 month or more ;). Actually I'm ready to spend my free time for
| > this. So how do you think could it be reasonable?
|   I've spent some time hunting bugs in UDF recently so I'll warn you a
| bit :). Definitely rewriting that ... code would be a good thing to do
| (reading that code I had urges to do it several times). The hard thing
| is that there is no reasonable spec you could use - there are two
| documents which define how UDF should look like but they are really hard
| to read (they have like hundred pages each and one does not make sence
| without the other). And reading the code and learning how the filesystem is
| supposed to work isn't too helpful either. Just a friendly warning ;)
| 
| 								Honza
| -- 
| Jan Kara <jack@suse.cz>
| SuSE CR Labs
| 

I've that documants even printed ;) Actually they are _very-very_ big
indeed. I don't know may be just try to bring this code into Linux
codying style?

		Cyrill

Re: [PATCH] UDF: check for allocated memory for inode data

[Christoph Hellwig]

On Wed, May 16, 2007 at 09:52:57PM +0400, Cyrill Gorcunov wrote:
> I've that documants even printed ;) Actually they are _very-very_ big
> indeed. I don't know may be just try to bring this code into Linux
> codying style?

That's probably a good step.  And while converting things to a proper
style you'll surely find various bugs and thinkgs you could improve.
Write them down somewhere and work on fixing them.  And make sure every
actualy fix or behaviour change is a single patch separated from the
cleanups.

Re: [PATCH] UDF: check for allocated memory for inode data

[Cyrill Gorcunov]

[Christoph Hellwig - Wed, May 16, 2007 at 06:56:00PM +0100]
| On Wed, May 16, 2007 at 09:52:57PM +0400, Cyrill Gorcunov wrote:
| > I've that documants even printed ;) Actually they are _very-very_ big
| > indeed. I don't know may be just try to bring this code into Linux
| > codying style?
| 
| That's probably a good step.  And while converting things to a proper
| style you'll surely find various bugs and thinkgs you could improve.
| Write them down somewhere and work on fixing them.  And make sure every
| actualy fix or behaviour change is a single patch separated from the
| cleanups.
| 

Hi Christoph,

I almost have completed UDF style conversation (the only thing to do
is to check all I've changed). And I've been striked down by the simple
question: the conversion I've done is over 2.6.22-rc1 but meantime in -mm
tree two my patches already exist so should I take them into account while
converting UDF style?

		Cyrill

Re: [PATCH] UDF: check for allocated memory for inode data

[Jan Kara]

On Sun 20-05-07 16:20:10, Cyrill Gorcunov wrote:
> [Christoph Hellwig - Wed, May 16, 2007 at 06:56:00PM +0100]
> | On Wed, May 16, 2007 at 09:52:57PM +0400, Cyrill Gorcunov wrote:
> | > I've that documants even printed ;) Actually they are _very-very_ big
> | > indeed. I don't know may be just try to bring this code into Linux
> | > codying style?
> | 
> | That's probably a good step.  And while converting things to a proper
> | style you'll surely find various bugs and thinkgs you could improve.
> | Write them down somewhere and work on fixing them.  And make sure every
> | actualy fix or behaviour change is a single patch separated from the
> | cleanups.
> | 
> 
> Hi Christoph,
> 
> I almost have completed UDF style conversation (the only thing to do
> is to check all I've changed). And I've been striked down by the simple
> question: the conversion I've done is over 2.6.22-rc1 but meantime in -mm
> tree two my patches already exist so should I take them into account while
> converting UDF style?
  I guess Andrew would be most grateful if you counted with these two
patches...

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

Re: [PATCH] UDF: check for allocated memory for inode data

[Christoph Hellwig]

On Sun, May 20, 2007 at 04:20:10PM +0400, Cyrill Gorcunov wrote:
> I almost have completed UDF style conversation (the only thing to do
> is to check all I've changed). And I've been striked down by the simple
> question: the conversion I've done is over 2.6.22-rc1 but meantime in -mm
> tree two my patches already exist so should I take them into account while
> converting UDF style?

It's probably best if you do that patch ontop of -mm so it includes
your patches.