Re: [PATCH] fix error return get/set_native_max functions

Re: [PATCH] fix error return get/set_native_max functions

[Andries.Brouwer]

> This change is okay, thanks.
> However changing coding style is not...

An interesting remark.

I belong to the people who look at kernel source on a screen
with 80 columns. Code that is wider and wraps is unreadable.

Now of course you might react "buy a better monitor", but in fact
this restriction leads to cleaner code. There is something wrong
when code is indented too deeply, and almost always a cleanup is
possible that splits some inner stuff out as a separate function.

As a side effect of that you'll see in patches from me changes
that bring the code within the 80-column limit.

> -static unsigned long idedisk_set_max_address(ide_drive_t *drive, unsigned long addr_req)
> +static unsigned long
> +idedisk_set_max_address(ide_drive_t *drive, unsigned long addr_req)

It is a matter of taste precisely which transformation is best
in order to bring the source within the 80-column limit,
but having the type on the preceding line is very common
in the kernel source (and elsewhere), so among the possible
ways of splitting this line this is a very natural one.

I am not interested in a discussion about style, but will defend
the 80-column limit.

Andries


---
Functions should be short and sweet, and do just one thing.  They should
fit on one or two screenfuls of text (the ISO/ANSI screen size is 80x24,
as we all know), and do one thing and do that well.

Re: [PATCH] fix error return get/set_native_max functions

[Bartlomiej Zolnierkiewicz]

On Tue, 5 Aug 2003 Andries.Brouwer@cwi.nl wrote:

> > This change is okay, thanks.
> > However changing coding style is not...
>
> An interesting remark.
>
> I belong to the people who look at kernel source on a screen
> with 80 columns. Code that is wider and wraps is unreadable.

/me too

> Now of course you might react "buy a better monitor", but in fact
> this restriction leads to cleaner code. There is something wrong
> when code is indented too deeply, and almost always a cleanup is
> possible that splits some inner stuff out as a separate function.
>
> As a side effect of that you'll see in patches from me changes
> that bring the code within the 80-column limit.

I think that mixing such changes with real changes is a bad thing.

> > -static unsigned long idedisk_set_max_address(ide_drive_t *drive, unsigned long addr_req)
> > +static unsigned long
> > +idedisk_set_max_address(ide_drive_t *drive, unsigned long addr_req)
>
> It is a matter of taste precisely which transformation is best
> in order to bring the source within the 80-column limit,
> but having the type on the preceding line is very common
> in the kernel source (and elsewhere), so among the possible
> ways of splitting this line this is a very natural one.

It is not so common in drivers/ide/...

static unsigned long idedisk_set_max_address(ide_drive_t *drive,
					     unsigned long addr_req)

This format also clearly shows that actual function name suck and should
be shortened :-).

> I am not interested in a discussion about style, but will defend
> the 80-column limit.

Sure, 80-column is a must ;-).
--
Bartlomiej

Re: [PATCH] fix error return get/set_native_max functions

[Roman Zippel]

Hi,

On Tue, 5 Aug 2003 Andries.Brouwer@cwi.nl wrote:

> > This change is okay, thanks.
> > However changing coding style is not...
> 
> An interesting remark.
> 
> I belong to the people who look at kernel source on a screen
> with 80 columns. Code that is wider and wraps is unreadable.

Get a better editor?
While I think 80 columns is a worthwhile goal, I don't see a good reason 
to wrap a random line only because it exceeds the limit by a few 
characters. What is especially annoying are patches with a one line fix
hidden within 10 other formatting changes (I've seen this already from 
you). Please respect others people code and try to avoid randoming 
formatting changes or at least keep them separate.

bye, Roman

Re: [PATCH] fix error return get/set_native_max functions

[Andries.Brouwer]

> Giant patch? :( Can you split it?

Of course.

A moment ago I split off the third part. More will follow.
(1 was bookkeeping in sector_t, 2 was addr++ fix).

This third part takes code that was repeated three times,
namely a debugging printout of the IDENTIFY DEVICE result,
and leaves only a single copy, that moreover is a bit more correct.

One of the things that were wrong in the debugging part
is also wrong in the non-debugging part, namely the ordering
of the bitfields on a bigendian architecture.
Probably that will be the next installment.

Let me send this in four subparts.
Three parts that remove the debugging code, and one part
that adds a single copy, in a new file ide-identify.h
that contains stuff that will also correct and simplify
other code.

Andries

Re: [PATCH] fix error return get/set_native_max functions

[Bartlomiej Zolnierkiewicz]

On Tue, 5 Aug 2003 Andries.Brouwer@cwi.nl wrote:

> By the way, is your tree visible somewhere?

Not yet.

> I have a giant patch with quite a lot of improvements and
> minor bug fixes, all in the geometry / IDENTIFY area.

Giant patch? :( Can you split it?

Splitting on obvious & non-obvious parts will help greatly.
Obvious parts can go into Linus' tree quickly.

> Usually I diff against Linus' most recent version, but we
> already had one by Erik and two by me, and stuff overlaps,
> and I do not yet see any of this in Linus' tree.

Because it takes a while to check them
(especially if they are not splitted).

--
Bartlomiej

Re: [PATCH] fix error return get/set_native_max functions

[Andries.Brouwer]

By the way, is your tree visible somewhere?

I have a giant patch with quite a lot of improvements and
minor bug fixes, all in the geometry / IDENTIFY area.
Usually I diff against Linus' most recent version, but we
already had one by Erik and two by me, and stuff overlaps,
and I do not yet see any of this in Linus' tree.
Usually I wait with sending the next until I see the previous
patch applied, but when there are many fragments that process
is excruciatingly slow. So, maybe I can come with a series of
patches against your tree?

Re: [PATCH] fix error return get/set_native_max functions

[Bartlomiej Zolnierkiewicz]

On Tue, 5 Aug 2003 Andries.Brouwer@cwi.nl wrote:

> In ide-disk.c we have functions
>   idedisk_read_native_max_address
>   idedisk_read_native_max_address_ext
>   idedisk_set_max_address
>   idedisk_set_max_address_ext
> that are documented as
>
>  /*
>   * Sets maximum virtual LBA address of the drive.
>   * Returns new maximum virtual LBA address (> 0) or 0 on failure.
>   */
>
> The IDE command they execute returns the largest address,
> and 1 is added to get the capacity.
> Unfortunately, the code does
>
> 	addr = 0;
> 	if (ide_command_succeeds) {
> 		addr = ...
> 	}
> 	addr++;
>
> so that the return value on error is 1 instead of 0.
> The patch below moves the addr++.
>
> Andries

This change is okay, thanks.

However changing coding style is not...

> @@ -1002,7 +1003,8 @@
>   * Sets maximum virtual LBA address of the drive.
>   * Returns new maximum virtual LBA address (> 0) or 0 on failure.
>   */
> -static unsigned long idedisk_set_max_address(ide_drive_t *drive, unsigned long addr_req)
> +static unsigned long
> +idedisk_set_max_address(ide_drive_t *drive, unsigned long addr_req)
>  {
>  	ide_task_t args;
>  	unsigned long addr_set = 0;

--
Bartlomiej

[PATCH] fix error return get/set_native_max functions

[Andries.Brouwer]

In ide-disk.c we have functions
  idedisk_read_native_max_address
  idedisk_read_native_max_address_ext
  idedisk_set_max_address
  idedisk_set_max_address_ext
that are documented as

 /*
  * Sets maximum virtual LBA address of the drive.
  * Returns new maximum virtual LBA address (> 0) or 0 on failure.
  */

The IDE command they execute returns the largest address,
and 1 is added to get the capacity.
Unfortunately, the code does

	addr = 0;
	if (ide_command_succeeds) {
		addr = ...
	}
	addr++;

so that the return value on error is 1 instead of 0.
The patch below moves the addr++.

Andries

diff -u --recursive --new-file -X /linux/dontdiff a/drivers/ide/ide-disk.c b/drivers/ide/ide-disk.c
--- a/drivers/ide/ide-disk.c	Mon Jul 28 05:39:23 2003
+++ b/drivers/ide/ide-disk.c	Tue Aug  5 21:00:34 2003
@@ -964,12 +964,13 @@
 		     | ((args.tfRegister[  IDE_HCYL_OFFSET]       ) << 16)
 		     | ((args.tfRegister[  IDE_LCYL_OFFSET]       ) <<  8)
 		     | ((args.tfRegister[IDE_SECTOR_OFFSET]       ));
+		addr++;	/* since the return value is (maxlba - 1), we add 1 */
 	}
-	addr++;	/* since the return value is (maxlba - 1), we add 1 */
 	return addr;
 }
 
-static unsigned long long idedisk_read_native_max_address_ext(ide_drive_t *drive)
+static unsigned long long
+idedisk_read_native_max_address_ext(ide_drive_t *drive)
 {
 	ide_task_t args;
 	unsigned long long addr = 0;
@@ -992,8 +993,8 @@
 			   ((args.tfRegister[IDE_LCYL_OFFSET])<<8) |
 			    (args.tfRegister[IDE_SECTOR_OFFSET]);
 		addr = ((__u64)high << 24) | low;
+		addr++;	/* since the return value is (maxlba - 1), we add 1 */
 	}
-	addr++;	/* since the return value is (maxlba - 1), we add 1 */
 	return addr;
 }
 
@@ -1002,7 +1003,8 @@
  * Sets maximum virtual LBA address of the drive.
  * Returns new maximum virtual LBA address (> 0) or 0 on failure.
  */
-static unsigned long idedisk_set_max_address(ide_drive_t *drive, unsigned long addr_req)
+static unsigned long
+idedisk_set_max_address(ide_drive_t *drive, unsigned long addr_req)
 {
 	ide_task_t args;
 	unsigned long addr_set = 0;
@@ -1024,12 +1026,13 @@
 			 | ((args.tfRegister[  IDE_HCYL_OFFSET]       ) << 16)
 			 | ((args.tfRegister[  IDE_LCYL_OFFSET]       ) <<  8)
 			 | ((args.tfRegister[IDE_SECTOR_OFFSET]       ));
+		addr_set++;
 	}
-	addr_set++;
 	return addr_set;
 }
 
-static unsigned long long idedisk_set_max_address_ext(ide_drive_t *drive, unsigned long long addr_req)
+static unsigned long long
+idedisk_set_max_address_ext(ide_drive_t *drive, unsigned long long addr_req)
 {
 	ide_task_t args;
 	unsigned long long addr_set = 0;
@@ -1059,6 +1062,7 @@
 			   ((args.tfRegister[IDE_LCYL_OFFSET])<<8) |
 			    (args.tfRegister[IDE_SECTOR_OFFSET]);
 		addr_set = ((__u64)high << 24) | low;
+		addr_set++;
 	}
 	return addr_set;
 }