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]
Date:	Tue, 05 Jun 2007 10:46:43 -0700
From:	Mingming Cao <cmm@...ibm.com>
To:	Laurent Vivier <Laurent.Vivier@...l.net>
Cc:	"Jose R. Santos" <jrs@...ibm.com>,
	Dave Kleikamp <shaggy@...ux.vnet.ibm.com>,
	Andreas Dilger <adilger@...sterfs.com>,
	linux-ext4 <linux-ext4@...r.kernel.org>
Subject: Re: [RFC][PATCH] Set JBD2_FEATURE_INCOMPAT_64BIT on filesystems
	larger than 32-bit blocks (take 2).

On Tue, 2007-06-05 at 18:07 +0200, Laurent Vivier wrote:
> Jose R. Santos wrote:
> > On Tue, 05 Jun 2007 16:03:44 +0200
> > Laurent Vivier <Laurent.Vivier@...l.net> wrote:
> >> Jose R. Santos wrote:
> >>> Hi Laurent,
> >>>
> >>> In this particular case though, the value of s_blocks_count_hi should not be
> >>> uses on its own.  The correct way would be to use ext4_blocks_count() which
> >>> already does the endian conversion.  If you think the code could confuse
> >>> people as to how to access the data in s_blocks_count_hi, wouldn't hiding it
> >>> through the use of a macro make more sense than doing an unnecessary endian
> >>> conversion?
> >>>
> >> Yes, I think the code could confuse people, but I don't think defining "Yet
> >> Another Macro" is a good choice (IMHO).
> >>
> >> I think we can resolve this (non-)issue by two ways:
> >> - using le32_to_cpu() (but I agree it does an unnecessary endian conversion on
> >> big-endian systems)
> > 
> > I just think that adding extra instructions for the sake of slightly
> > better code readability is wrong, especially when the value
> > s_blocks_count_hi should not be used on its own.
> > 
> >> - put a comment on the line (but are we allowed to put comments in kernel source
> >> code... ;-) )
> > 
> > One advantage of a macro here is that we would make the code more
> > explicit and should be able to eliminate the need for those 4 lines of
> > comments that this patch adds.
> 
> IMHO, you should do as _you_ think it is better... but as Mingming did the first
> comment perhaps she can explain what she thought.
> 

The better choice to me is using ext4_blocks_count() to hide the details
of the little endian. It's fine to use s_blocks_count_hi directly, just
to make it clear, this is on-disk superblock data and better to do
little endian conversion like read-in other on-disk superblock fields. 

Yeah, it probably unnecessary in this case, but I don't think the extra
instruction plays an important role in the performance, (this is only
called at mount time, and there are lots of other places doing little
endian conversion in ext4_fill_super() anyway).

Mingming
> Regards,
> Laurent

-
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