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: <20081117000445.GA3436@webber.adilger.int>
Date:	Sun, 16 Nov 2008 18:04:45 -0600
From:	Andreas Dilger <adilger@....com>
To:	Frank Mayhar <fmayhar@...gle.com>
Cc:	linux-ext4@...r.kernel.org, Michael Rubin <mrubin@...gle.com>
Subject: Re: [RFC PATCH 1/1] Allow ext4 to run without a journal, take 2.

On Nov 11, 2008  11:18 -0800, Frank Mayhar wrote:
> +static inline int ext4_handle_valid(handle_t *handle)
> +{
> +	if (!handle)
> +		return 0;

My preference is to use "if (handle == NULL)", because handle is not a
boolean.

> +	if (handle->h_transaction == NULL)
> +		return 0;

Does this ever happen?  Ah, is this the case where the handle is pointing
to the ext4_sb_info()?  I think I'd prefer to have a magic value here,
so that it isn't possible to accidentally dereference a pointer that
happens to have NULL data in it.

> +static inline int ext4_handle_dirty_metadata(handle_t *handle,
> +				struct inode *inode, struct buffer_head *bh)
> +{
> +	int err = 0;
> +
> +	if (ext4_handle_valid(handle)) {
> +		err = __ext4_journal_dirty_metadata(__func__, handle, bh);
> +	} else {
> +		err = __ext4_write_dirty_metadata(inode, bh);
> +	}

You don't need to initialize "err = 0" here.

>  struct ext4_sb_info {
> +	int *s_nojournal_flag;		/* Null to indicate "not a handle" */

I don't really see where and how this is used.

> @@ -97,6 +97,8 @@
>  {

Please, in the future use "diff -up" so that it includes the function
names in the patch context.  Without the function names it is difficult
to know what is being changed by the patch.

> @@ -4657,7 +4659,7 @@
> -	if (metadata) {
> +	if (ext4_handle_valid(handle) && metadata) {

It is probably a tiny bit more efficient to check "metadata" first.

> @@ -136,13 +136,16 @@
> +	if (journal) {
> +		if (is_journal_aborted(journal)) {
> +			ext4_abort(sb, __func__,
> +				   "Detected aborted journal");
> +			return ERR_PTR(-EROFS);
> +		}
> +		return jbd2_journal_start(journal, nblocks);
>  	}
> +	current->journal_info = (handle_t *)EXT4_SB(sb);
> +	return current->journal_info;

So, this appears to be the place where ext4_sb_info->s_nojournal_flag is
actually used.  To make this more clear, it would be better to do actually
reference this variable here so that it is easier for the reader to follow.

	current->journal_info = &EXT4_SB(sb)->s_nojournal_flag;

> @@ -588,7 +599,8 @@
>  	}
>  #endif
>  	ext4_discard_preallocations(inode);
> +	if (EXT4_SB(inode->i_sb)->s_journal)

I think this is the same thing as EXT4_JOURNAL(inode)?

>  static void ext4_write_super(struct super_block *sb)
>  {
> +	if (EXT4_SB(sb)->s_journal) {
> +		if (mutex_trylock(&sb->s_lock) != 0)
> +			BUG();
> +		sb->s_dirt = 0;
> +	}
> +	else
> +		ext4_commit_super(sb, EXT4_SB(sb)->s_es, 1);

This should be "} else { ... }"

> +			/*
> +			 * We don't want to clear needs_recovery flag when we failed
> +			 * to flush the journal.
> +			 */
> +			if (jbd2_journal_flush(journal) < 0)
> +				return;

This comment needs to be wrapped at 80 columns.


I like this patch a lot better than the previous one.  It remains very
readable, and isn't too intrusive to the code.

Cheers, Andreas
--
Andreas Dilger
Sr. Staff Engineer, Lustre Group
Sun Microsystems of Canada, Inc.

--
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

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ