All of lore.kernel.org
 help / color / mirror / Atom feed
From: "Jose R. Santos" <jrs@us.ibm.com>
To: Laurent Vivier <Laurent.Vivier@bull.net>
Cc: Dave Kleikamp <shaggy@linux.vnet.ibm.com>,
	cmm@us.ibm.com, Andreas Dilger <adilger@clusterfs.com>,
	linux-ext4 <linux-ext4@vger.kernel.org>
Subject: Re: [RFC][PATCH] Set JBD2_FEATURE_INCOMPAT_64BIT on filesystems larger than 32-bit blocks (take 2).
Date: Tue, 5 Jun 2007 08:49:37 -0500	[thread overview]
Message-ID: <20070605084937.7622258a@gara> (raw)
In-Reply-To: <4665649D.8010302@bull.net>

On Tue, 05 Jun 2007 15:26:53 +0200
Laurent Vivier <Laurent.Vivier@bull.net> wrote:

> Dave Kleikamp wrote:
> > Jose is right.  The endian conversion is unnecessary.
> > 
> > Shaggy
> 
> But by using le32_to_cpu(es->s_blocks_count_hi) you explicitly mark the variable
> as a little-endian.
> So if someone reads the code, he knows this is a little-endian value and this
> allows to avoid errors if later variable must be tested for other value than 0.
> 
> For instance, you have :
> 
> if(es->s_blocks_count_hi)
> 
> and later the value should be compared to 10, how do you know easily you should use:
> 
> if (le32_to_cpu(es->s_blocks_count_hi) == 10)
> 
> instead of
> 
> if(es->s_blocks_count_hi == 10)
> 
> I think writing like Mingming asks should allow to avoid errors later.
> 
> (and code becomes really self-explicit...)
> 
> Regards,
> Laurent

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?

-JRS

  reply	other threads:[~2007-06-05 13:49 UTC|newest]

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2007-06-01 15:52 [RFC][PATCH] Set JBD2_FEATURE_INCOMPAT_64BIT on filesystems larger than 32-bit blocks Jose R. Santos
2007-06-01 22:54 ` Andreas Dilger
2007-06-04 16:32   ` [RFC][PATCH] Set JBD2_FEATURE_INCOMPAT_64BIT on filesystems larger than 32-bit blocks (take 2) Jose R. Santos
2007-06-04 17:57     ` Andreas Dilger
2007-06-04 23:01       ` Mingming Cao
2007-06-04 23:32         ` Andreas Dilger
2007-06-05 11:41         ` Jose R. Santos
2007-06-05 13:14           ` Dave Kleikamp
2007-06-05 13:26             ` Laurent Vivier
2007-06-05 13:49               ` Jose R. Santos [this message]
2007-06-05 14:03                 ` Laurent Vivier
2007-06-05 15:46                   ` Jose R. Santos
2007-06-05 16:07                     ` Laurent Vivier
2007-06-05 17:46                       ` Mingming Cao
2007-06-05 19:58                         ` Jose R. Santos

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20070605084937.7622258a@gara \
    --to=jrs@us.ibm.com \
    --cc=Laurent.Vivier@bull.net \
    --cc=adilger@clusterfs.com \
    --cc=cmm@us.ibm.com \
    --cc=linux-ext4@vger.kernel.org \
    --cc=shaggy@linux.vnet.ibm.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.