2014-06-16 19:47:08

by Nicholas Krause

Subject: [PATCH] Check for Null return from logfs_readpage_nolock in btree_write_block

Signed-off-by: Nicholas Krause <[email protected]>
---
fs/logfs/readwrite.c | 2 ++
1 file changed, 2 insertions(+)

diff --git a/fs/logfs/readwrite.c b/fs/logfs/readwrite.c
index 4814031..adb9233 100644
--- a/fs/logfs/readwrite.c
+++ b/fs/logfs/readwrite.c
@@ -2210,6 +2210,8 @@ void btree_write_block(struct logfs_block *block)
page = logfs_get_write_page(inode, block->bix, block->level);

err = logfs_readpage_nolock(page);
+ if (!err)
+ return -ENOMEM;
BUG_ON(err);
BUG_ON(!PagePrivate(page));
BUG_ON(logfs_block(page) != block);
--
1.9.1

2014-06-16 20:06:39

by Mateusz Guzik

Subject: Re: [PATCH] Check for Null return from logfs_readpage_nolock in btree_write_block

On Mon, Jun 16, 2014 at 03:47:01PM -0400, Nicholas Krause wrote:
> Signed-off-by: Nicholas Krause <[email protected]>
> ---
> fs/logfs/readwrite.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/fs/logfs/readwrite.c b/fs/logfs/readwrite.c
> index 4814031..adb9233 100644
> --- a/fs/logfs/readwrite.c
> +++ b/fs/logfs/readwrite.c
> @@ -2210,6 +2210,8 @@ void btree_write_block(struct logfs_block *block)
> page = logfs_get_write_page(inode, block->bix, block->level);
>
> err = logfs_readpage_nolock(page);
> + if (!err)
> + return -ENOMEM;
> BUG_ON(err);
> BUG_ON(!PagePrivate(page));
> BUG_ON(logfs_block(page) != block);

This function returns 0 on success, which you turn into error condition
and return -ENOMEM.
But the function returns void, thus it cannot return the error.
It does not allocate anything, thus -ENOMEM would not be appropriate in
the first place.

Since the function returns error, nobody would check the condition in
the first place.

Even if it was not void, it would either have to return the error or
oops on BUG_ON(err).

Read the code more carefully and at least compile-test your changes.
Instructions how to compile the kernel can be found here:
http://kernelnewbies.org/FAQ/KernelCompilation

I would also suggest letting the patch wait few hours and have another
look before sending.

Cheers,
--
Mateusz Guzik

2014-06-17 01:23:15

by Jörn Engel

Subject: Re: [PATCH] Check for Null return from logfs_readpage_nolock in btree_write_block

On Mon, 16 June 2014 20:46:32 -0400, Nick Krause wrote:
>
> You are right about the compile errors it compiles but with warning.
> I am curious since the function is void how do you want me to
> give the error back to logfs and in turn the other subsystems
> of the kernel.

My first question would be: What is the problem? If there is no
problem, don't try to fix it. If there is a problem, your fix should
contain a decent description of the problem as part of the commit
message. Even if the patch itself ends up being bad, having a good
problem description with a first stab at a fix serves as a useful bug
report.

Jörn

--
Was there any time in human history where democratic forms continued
when the citizens could not credibly defend their rights?
-- Daniel Suarez