linux-ext4 - Re: [PATCH 1/3] jbd2 : Make jbd2 transaction handle allocation to return errors and handle them gracefully.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <AA32FD05-E9BD-40AB-8495-EAD0547B5E02@dilger.ca>
Date:	Mon, 24 Jan 2011 18:06:24 -0600
From:	Andreas Dilger <adilger.kernel@...ger.ca>
To:	Jan Kara <jack@...e.cz>
Cc:	Joel Becker <jlbec@...lplan.org>, Ted Ts'o <tytso@....edu>,
	Manish Katiyar <mkatiyar@...il.com>,
	ext4 <linux-ext4@...r.kernel.org>
Subject: Re: [PATCH 1/3] jbd2 : Make jbd2 transaction handle allocation to return errors and handle them gracefully.

On 2011-01-24, at 06:31, Jan Kara wrote:
> On Sat 22-01-11 22:29:01, Joel Becker wrote:
>> On Sun, Jan 23, 2011 at 12:40:49AM -0500, Ted Ts'o wrote:
>>> Hmm, I wonder if it would be better to use
>>> 
>>> jbd2_journal_start(...)
>>> 
>>> and
>>> 
>>> jbd2_journal_start_nofail(...)
>> 
>> 	This API is markedly better to read.  Btw, does _nofail() mean no
>> possible failures, or just no memory errors?  If it is no failures, I'd
>> love to see the function become void.

I also prefer changing the function name instead of adding an argument.

>  jbd2_journal_start can always fail e.g. because the journal is aborted.
> So it really just means no memory failures...
> 
>>> The tradeoff is that long-term, the code is more readable (as opposed
>>> to having people look up what a random "true" or "false" value means).
>>> But short-term, while it will make the patch smaller, it also makes
>>> the patch harder audit, since we need to look at all of the places
>>> where we _haven't_ made a change to make sure those call sites can
>>> tolerate an error return.
>> 
>> 	I think we should start with jbd2_journal_start_can_fail() or
>> something like it, and change it back to jbd2_journal_start() in the
>> next window.  It's a silly name, but it catches exactly what you are
>> worried about.
> 
>  Yes, I think this would be nice for auditting (but for that matter
> current interface with additional argument isn't bad either and we can
> just do the rename to _nofail in the final patch...).

The reason I don't like the "true" and "false" arguments is that it isn't at all clear which functions have "false" because they cannot fail, and which ones just haven't been updated yet.

In that light, I'd prefer to add _two_ new functions, one that indicates the function needs to retry (as it does now), and one that indicates that the caller will handle the error.  That way it is clear which functions have been investigated, and which ones haven't been looked at yet.  Once all of the functions have been changed, we can remove the old jbd2_journal_start() function to catch any patches that have not been updated to the new functions.

Maybe jbd2_journal_start_canfail() and jbd2_journal_start_retry()?


Cheers, Andreas





--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html