All of lore.kernel.org
 help / color / mirror / Atom feed
From: Douglas Gilbert <dgilbert@interlog.com>
To: James Bottomley <James.Bottomley@suse.de>
Cc: Boaz Harrosh <bharrosh@panasas.com>,
	"Martin K. Petersen" <martin.petersen@oracle.com>,
	linux-scsi@vger.kernel.org
Subject: Re: [PATCH 5/5] scsi_debug: Implement support for DIF Type 2
Date: Thu, 27 Aug 2009 11:17:18 -0400	[thread overview]
Message-ID: <4A96A37E.6010700@interlog.com> (raw)
In-Reply-To: <1251380478.6426.11.camel@mulgrave.site>

James Bottomley wrote:
> On Thu, 2009-08-27 at 12:35 +0300, Boaz Harrosh wrote:
>> On 08/27/2009 09:58 AM, Martin K. Petersen wrote:
>>>>>>>> "Boaz" == Boaz Harrosh <bharrosh@panasas.com> writes:
>>>>> + *lba = (u64)cmd[19] | (u64)cmd[18] << 8 |
>>>>> + (u64)cmd[17] << 16 | (u64)cmd[16] << 24 |
>>>>> + (u64)cmd[15] << 32 | (u64)cmd[14] << 40 |
>>>>> + (u64)cmd[13] << 48 | (u64)cmd[12] << 56;
>>> Boaz> get_unaligned_be64()
>>>
>>> As you noticed further down in that patch I do generally use the
>>> get_unaligned_* macros in "my own" code.
>>>
>>> However, when I update somebody else's code I try to match the existing
>>> style.  And in this case rest of get_data_transfer_info() is using
>>> explicit shifts and to me it looks absolutely horrendous to mix the two.
>>>
>>> I generally avoid mixing cleanups and new functionality.  I don't have a
>>> problem with switching over to the macros, but in that case I think the
>>> whole function should be updated.  And that should be an orthogonal
>>> patch.
>>>
>> I don't know. For me it is like checkpatch. I do not submit code over 80
>> chars even if surrounding code does. "The new code rule".
> 
> I'd take that as a guideline rather than a rule ... Especially when
> lindent will generate 80 column long function templates over tens of
> lines squashing about five letters per line ...
> 
>> I generally agree with what you say but I think there is a balance.
>> Personally, I think this is over the balance point, but it's your call.
> 
> The general rule is not to confuse coding styles, so the correct way to
> add stuff is to use the existing conventions in the file.  You can
> optionally convert the entire style if necessary.  However, for these
> get_cpu_be macros, there's no real benefit other than saving typing, so
> a global conversion effort simply isn't worth it.

There is another aspect that I look at: portability to
the user space of Linux and other operating systems.

Are there C99 or POSIX equivalents of get_unaligned_be64()?

Recently sg_read_block_limits was contributed to sg3_utils.
Its author found a novel solution to this "problem" with
network stack calls: ntohl() and friends. So I ran with it
until I found it broke my builds in the MinGW environment
for Windows. Rather than put in conditional compiles and
wonder if my MinGW executables would get bloated, I opted
to go back to what has worked for the last 25 years.

Doug Gilbert

  parent reply	other threads:[~2009-08-27 15:17 UTC|newest]

Thread overview: 34+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2009-08-26  6:17 DIF/DIX updates for 2.6.32 Martin K. Petersen
2009-08-26  6:17 ` [PATCH 1/5] SCSI: Add support for 32-byte CDBs Martin K. Petersen
2009-08-26 12:16   ` Boaz Harrosh
2009-08-27  6:38     ` Martin K. Petersen
2009-08-26  6:17 ` [PATCH 2/5] SCSI: Deprecate SCSI_PROT_*_CONVERT operations Martin K. Petersen
2009-08-26  6:17 ` [PATCH 3/5] sd: Detach DIF from block integrity infrastructure Martin K. Petersen
2009-08-26  6:18 ` [PATCH 4/5] sd: Support disks formatted with DIF Type 2 Martin K. Petersen
2009-08-26 12:26   ` Boaz Harrosh
2009-08-27  6:41     ` Martin K. Petersen
2009-08-26  6:18 ` [PATCH 5/5] scsi_debug: Implement support for " Martin K. Petersen
2009-08-26 12:40   ` Boaz Harrosh
2009-08-27  6:58     ` Martin K. Petersen
2009-08-27  9:35       ` Boaz Harrosh
2009-08-27 13:41         ` James Bottomley
2009-08-27 14:20           ` Boaz Harrosh
2009-08-27 14:30             ` James Bottomley
2009-08-27 14:47               ` Boaz Harrosh
2009-08-27 14:54                 ` James Bottomley
2009-08-27 15:17           ` Douglas Gilbert [this message]
2009-08-27 15:39             ` Boaz Harrosh
2009-08-26 11:54 ` DIF/DIX updates for 2.6.32 Boaz Harrosh
2009-08-27  6:34   ` Martin K. Petersen
2009-08-27  9:49     ` Boaz Harrosh
2009-08-27 13:46       ` James Bottomley
2009-08-27 14:40         ` Boaz Harrosh
2009-08-27 14:51           ` James Bottomley
2009-08-27 15:18             ` Boaz Harrosh
2009-08-27 15:22               ` James Bottomley
2009-08-27 20:02             ` Martin K. Petersen
2009-08-27 20:05               ` Chris Mason
  -- strict thread matches above, loose matches on Subject: below --
2009-09-04  8:36 Martin K. Petersen
2009-09-04  8:36 ` [PATCH 5/5] scsi_debug: Implement support for DIF Type 2 Martin K. Petersen
2009-09-11 19:20 DIF/DIX updates for 2.6.32 Martin K. Petersen
2009-09-11 19:20 ` [PATCH 5/5] scsi_debug: Implement support for DIF Type 2 Martin K. Petersen
2009-09-11 23:06   ` Douglas Gilbert
2009-09-18 21:32 Final DIF/DIX patches for 2.6.32 Martin K. Petersen
2009-09-18 21:33 ` [PATCH 5/5] scsi_debug: Implement support for DIF Type 2 Martin K. Petersen

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=4A96A37E.6010700@interlog.com \
    --to=dgilbert@interlog.com \
    --cc=James.Bottomley@suse.de \
    --cc=bharrosh@panasas.com \
    --cc=linux-scsi@vger.kernel.org \
    --cc=martin.petersen@oracle.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.