RE: [PATCH 1/4] [SCSI] ips: remove ips_ha members that duplicate struct pci_dev members
ACK. Inspected; Mechanical, precise and no introduction of bugs. Sincerely -- Mark Salyzyn -Original Message- From: [EMAIL PROTECTED] [mailto:[EMAIL PROTECTED] On Behalf Of Jeff Garzik Sent: Wednesday, October 24, 2007 7:48 PM To: LKML; linux-scsi@vger.kernel.org Cc: [EMAIL PROTECTED] Subject: [PATCH 1/4] [SCSI] ips: remove ips_ha members that duplicate struct pci_dev members Signed-off-by: Jeff Garzik [EMAIL PROTECTED] --- drivers/scsi/ips.c | 178 drivers/scsi/ips.h | 20 +++ 2 files changed, 91 insertions(+), 107 deletions(-) - To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/4] [SCSI] ips: remove ips_ha members that duplicate struct pci_dev members
On Thu, Oct 25 2007 at 7:09 +0200, Jeff Garzik [EMAIL PROTECTED] wrote:
Andrew Morton wrote:
On Wed, 24 Oct 2007 19:48:26 -0400 (EDT) Jeff Garzik [EMAIL PROTECTED]
wrote:
drivers/scsi/ips.c | 178
this driver seems a bit of a basket case :(
What's going on here?
scb-dcdb.cmd_attribute =
ips_command_direction[scb-scsi_cmd-cmnd[0]];
/* Allow a WRITE BUFFER Command to Have no Data */
/* This is Used by Tape Flash Utilites */
if ((scb-scsi_cmd-cmnd[0] == WRITE_BUFFER) (scb-data_len ==
0))
scb-dcdb.cmd_attribute = 0;
if (!(scb-dcdb.cmd_attribute 0x3))
scb-dcdb.transfer_length = 0;
if (scb-data_len = IPS_MAX_XFER) {
I hope that's just busted indentation and not a missing {} block.
The driver is one of the grotty drivers people are afraid to touch,
therefore it bitrots even faster, in a vicious cycle. You don't have to
look hard at all to find checkpatch pukeage, very real bugs, and
Pythonesque silliness.
Not having hardware I went only for changes that are provable and
obvious, really...
Jeff
From the experience with gdth I would say that a first patch of any
serious cleanup, should be the whitespace and formating cleanup.
And now that we can all read what it says, start changing.
In the gdth case I know of 3 bugs that happen just because of that mess.
And I promise to send that patch for gdth sn.
I found that lint, even with the command line options recommended by
kernel, is to aggressive, and leaves lots of work to be fixed by hand.
(e.g it will touch the comments)
I found that a small util called astyle with the right settings for the
tab==8/use-tabs will give you a clean tab indent, but will not touch the
longer than 80, broken out lines, which keeps things better formated.
Morton checkpatch is very good in whining about bad files, but did anyone
attempt a script for linux-style Indentation, formating, and whitespace cleanup,
that give you something like 95% success. As it is lint gives you 50-70%
and astyle 60-80%
Boaz
-
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/4] [SCSI] ips: remove ips_ha members that duplicate struct pci_dev members
On Thu, Oct 25, 2007 at 05:04:38PM +0200, Boaz Harrosh wrote: I found that lint, even with the command line options recommended by Do you mean Lindent / indent? kernel, is to aggressive, and leaves lots of work to be fixed by hand. (e.g it will touch the comments) It's not perfect, but code beautification is an art, not a science ;-) A lot of ugly code can't be made beautiful by a simple parser like indent because what it really needs is refactoring. But you can't refactor until you've made it at least partially readable, so Lindent is the first step. -- Intel are signing my paycheques ... these opinions are still mine Bill, look, we understand that you're interested in selling us this operating system, but compare it to ours. We can't possibly take such a retrograde step. - To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/4] [SCSI] ips: remove ips_ha members that duplicate struct pci_dev members
Matthew Wilcox wrote: On Thu, Oct 25, 2007 at 05:04:38PM +0200, Boaz Harrosh wrote: I found that lint, even with the command line options recommended by Do you mean Lindent / indent? Yes indent. I found that there are better switches to indent than what's in Lindent. But both are crap as for instance it does not understand the linux style multi-line-comments and mess them up, or it tabafy's broken-out-lines. Which is not what we usually want. And lots of other stuff. astyle does a much better job just by being less aggressive. kernel, is to aggressive, and leaves lots of work to be fixed by hand. (e.g it will touch the comments) It's not perfect, but code beautification is an art, not a science ;-) A lot of ugly code can't be made beautiful by a simple parser like indent because what it really needs is refactoring. But you can't refactor until you've made it at least partially readable, so Lindent is the first step. I think I disagree 89% and agree 11%. - remove trailing spaces - Indent, - Convert Indents to tabs - split long lines to multi lines, indent up to the last indent level, than fill with spaces, right align. These can all be done by a machine, the Linux way. Than - Adjust broken out lines, like if and while statements to be more readable - Remove/add blank lines for readability or after end of locals. That can be adjusted by a person. But I think with a given code it can be made 89% acceptable by a machine and 11% adjusted by a man. That is before I start a single char of coding. Boaz - To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/4] [SCSI] ips: remove ips_ha members that duplicate struct pci_dev members
Boaz Harrosh wrote:
On Thu, Oct 25 2007 at 7:09 +0200, Jeff Garzik [EMAIL PROTECTED] wrote:
Andrew Morton wrote:
On Wed, 24 Oct 2007 19:48:26 -0400 (EDT) Jeff Garzik [EMAIL PROTECTED] wrote:
drivers/scsi/ips.c | 178
this driver seems a bit of a basket case :(
What's going on here?
scb-dcdb.cmd_attribute =
ips_command_direction[scb-scsi_cmd-cmnd[0]];
/* Allow a WRITE BUFFER Command to Have no Data */
/* This is Used by Tape Flash Utilites */
if ((scb-scsi_cmd-cmnd[0] == WRITE_BUFFER) (scb-data_len == 0))
scb-dcdb.cmd_attribute = 0;
if (!(scb-dcdb.cmd_attribute 0x3))
scb-dcdb.transfer_length = 0;
if (scb-data_len = IPS_MAX_XFER) {
I hope that's just busted indentation and not a missing {} block.
The driver is one of the grotty drivers people are afraid to touch,
therefore it bitrots even faster, in a vicious cycle. You don't have to
look hard at all to find checkpatch pukeage, very real bugs, and
Pythonesque silliness.
Not having hardware I went only for changes that are provable and
obvious, really...
Jeff
From the experience with gdth I would say that a first patch of any
serious cleanup, should be the whitespace and formating cleanup.
And now that we can all read what it says, start changing.
In the gdth case I know of 3 bugs that happen just because of that mess.
And I promise to send that patch for gdth sn.
I found that lint, even with the command line options recommended by
kernel, is to aggressive, and leaves lots of work to be fixed by hand.
(e.g it will touch the comments)
Yeah, I hear you, but don't mistake my kill-warnings work for embarking
upon a new clean-this-driver project ;-)
I tend to make incremental improvements all over the tree, rising tide
lifts all boats and that sort of thing.
If I remained and worked on cleaning every grotty driver I come across
while killing warnings or fixing interrupt handlers, I would have no
free time at all :)
Jeff
-
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/4] [SCSI] ips: remove ips_ha members that duplicate struct pci_dev members
thanks for reviewing these! - To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/4] [SCSI] ips: remove ips_ha members that duplicate struct pci_dev members
On Wed, 24 Oct 2007 19:48:26 -0400 (EDT) Jeff Garzik [EMAIL PROTECTED] wrote:
drivers/scsi/ips.c | 178
this driver seems a bit of a basket case :(
What's going on here?
scb-dcdb.cmd_attribute =
ips_command_direction[scb-scsi_cmd-cmnd[0]];
/* Allow a WRITE BUFFER Command to Have no Data */
/* This is Used by Tape Flash Utilites */
if ((scb-scsi_cmd-cmnd[0] == WRITE_BUFFER) (scb-data_len == 0))
scb-dcdb.cmd_attribute = 0;
if (!(scb-dcdb.cmd_attribute 0x3))
scb-dcdb.transfer_length = 0;
if (scb-data_len = IPS_MAX_XFER) {
I hope that's just busted indentation and not a missing {} block.
-
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/4] [SCSI] ips: remove ips_ha members that duplicate struct pci_dev members
Andrew Morton wrote:
On Wed, 24 Oct 2007 19:48:26 -0400 (EDT) Jeff Garzik [EMAIL PROTECTED] wrote:
drivers/scsi/ips.c | 178
this driver seems a bit of a basket case :(
What's going on here?
scb-dcdb.cmd_attribute =
ips_command_direction[scb-scsi_cmd-cmnd[0]];
/* Allow a WRITE BUFFER Command to Have no Data */
/* This is Used by Tape Flash Utilites */
if ((scb-scsi_cmd-cmnd[0] == WRITE_BUFFER) (scb-data_len == 0))
scb-dcdb.cmd_attribute = 0;
if (!(scb-dcdb.cmd_attribute 0x3))
scb-dcdb.transfer_length = 0;
if (scb-data_len = IPS_MAX_XFER) {
I hope that's just busted indentation and not a missing {} block.
The driver is one of the grotty drivers people are afraid to touch,
therefore it bitrots even faster, in a vicious cycle. You don't have to
look hard at all to find checkpatch pukeage, very real bugs, and
Pythonesque silliness.
Not having hardware I went only for changes that are provable and
obvious, really...
Jeff
-
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
