[PATCH 3/6] IDE: ide-cd: fix test unsigned var < 0

[PATCH 3/6] IDE: ide-cd: fix test unsigned var < 0

[Roel Kluin]

Is this the right fix in this case?
---
valid is unsigned and cannot be below 0.,
    
Signed-off-by: Roel Kluin <12o3l@tiscali.nl>
---
diff --git a/drivers/ide/ide-cd.c b/drivers/ide/ide-cd.c
index c8d0e87..a60f5df 100644
--- a/drivers/ide/ide-cd.c
+++ b/drivers/ide/ide-cd.c
@@ -182,8 +182,6 @@ void cdrom_analyze_sense_data(ide_drive_t *drive,
 			sector &= ~(bio_sectors -1);
 			valid = (sector - failed_command->sector) << 9;
 
-			if (valid < 0)
-				valid = 0;
 			if (sector < get_capacity(info->disk) &&
 				drive->probed_capacity - sector < 4 * 75) {
 				set_capacity(info->disk, sector);

Re: [PATCH 3/6] IDE: ide-cd: fix test unsigned var < 0

[Bartlomiej Zolnierkiewicz]

Hi,

On Wednesday 16 April 2008, Roel Kluin wrote:
> Is this the right fix in this case?

Yes, but it seems that 'valid' is only written to
so it may be as well removed completely.

Care to update the patch?

Thanks,
Bart

> ---
> valid is unsigned and cannot be below 0.,
>     
> Signed-off-by: Roel Kluin <12o3l@tiscali.nl>
> ---
> diff --git a/drivers/ide/ide-cd.c b/drivers/ide/ide-cd.c
> index c8d0e87..a60f5df 100644
> --- a/drivers/ide/ide-cd.c
> +++ b/drivers/ide/ide-cd.c
> @@ -182,8 +182,6 @@ void cdrom_analyze_sense_data(ide_drive_t *drive,
>  			sector &= ~(bio_sectors -1);
>  			valid = (sector - failed_command->sector) << 9;
>  
> -			if (valid < 0)
> -				valid = 0;
>  			if (sector < get_capacity(info->disk) &&
>  				drive->probed_capacity - sector < 4 * 75) {
>  				set_capacity(info->disk, sector);

Re: [PATCH 3/6] IDE: ide-cd: fix test unsigned var < 0

[Roel Kluin]

Bartlomiej Zolnierkiewicz wrote:
> Hi,
> 
> On Wednesday 16 April 2008, Roel Kluin wrote:
>> Is this the right fix in this case?
> 
> Yes, but it seems that 'valid' is only written to
> so it may be as well removed completely.
> 
> Care to update the patch?
> 
> Thanks,
> Bart

how about this?

Roel

---
Clean up cdrom_analyze_sense_data()

Signed-off-by: Roel Kluin <12o3l@tiscali.nl>
---
diff --git a/drivers/ide/ide-cd.c b/drivers/ide/ide-cd.c
index a60f5df..a5f3beb 100644
--- a/drivers/ide/ide-cd.c
+++ b/drivers/ide/ide-cd.c
@@ -143,8 +143,6 @@ void cdrom_analyze_sense_data(ide_drive_t *drive,
 			      struct request_sense *sense)
 {
 	unsigned long sector;
-	unsigned long bio_sectors;
-	unsigned long valid;
 	struct cdrom_info *info = drive->driver_data;
 
 	if (!cdrom_log_sense(drive, failed_command, sense))
@@ -174,13 +172,9 @@ void cdrom_analyze_sense_data(ide_drive_t *drive,
 				 (sense->information[2] <<  8) |
 				 (sense->information[3]);
 
-			bio_sectors = bio_sectors(failed_command->bio);
-			if (bio_sectors < 4)
-				bio_sectors = 4;
 			if (drive->queue->hardsect_size == 2048)
 				sector <<= 2;	/* Device sector size is 2K */
-			sector &= ~(bio_sectors -1);
-			valid = (sector - failed_command->sector) << 9;
+			sector &= ~(max(bio_sectors(failed_command->bio) - 1, 3));
 
 			if (sector < get_capacity(info->disk) &&
 				drive->probed_capacity - sector < 4 * 75) {

Re: [PATCH 3/6] IDE: ide-cd: fix test unsigned var < 0

[Boris Petkov]

On Thu, Apr 17, 2008 at 5:51 AM, Roel Kluin <12o3l@tiscali.nl> wrote:
> Bartlomiej Zolnierkiewicz wrote:
>  > Hi,
>  >
>  > On Wednesday 16 April 2008, Roel Kluin wrote:
>  >> Is this the right fix in this case?
>  >
>  > Yes, but it seems that 'valid' is only written to
>  > so it may be as well removed completely.
>  >
>  > Care to update the patch?
>  >
>  > Thanks,
>  > Bart
>
>  how about this?
>
>  Roel
>
>  ---
>  Clean up cdrom_analyze_sense_data()
>
>
>  Signed-off-by: Roel Kluin <12o3l@tiscali.nl>
>  ---
>  diff --git a/drivers/ide/ide-cd.c b/drivers/ide/ide-cd.c
>  index a60f5df..a5f3beb 100644
>
> --- a/drivers/ide/ide-cd.c
>  +++ b/drivers/ide/ide-cd.c
>  @@ -143,8 +143,6 @@ void cdrom_analyze_sense_data(ide_drive_t *drive,
>                               struct request_sense *sense)
>   {
>         unsigned long sector;
>  -       unsigned long bio_sectors;
>  -       unsigned long valid;
>         struct cdrom_info *info = drive->driver_data;
>
>         if (!cdrom_log_sense(drive, failed_command, sense))
>  @@ -174,13 +172,9 @@ void cdrom_analyze_sense_data(ide_drive_t *drive,
>                                  (sense->information[2] <<  8) |
>                                  (sense->information[3]);
>
>  -                       bio_sectors = bio_sectors(failed_command->bio);
>  -                       if (bio_sectors < 4)
>  -                               bio_sectors = 4;
>                         if (drive->queue->hardsect_size == 2048)
>                                 sector <<= 2;   /* Device sector size is 2K */
>  -                       sector &= ~(bio_sectors -1);
>
> -                       valid = (sector - failed_command->sector) << 9;
>  +                       sector &= ~(max(bio_sectors(failed_command->bio) - 1, 3));

Well, i don't think that this "optimization" makes the code more readable.
Besides, gcc does this anyway. Poor are only those who have to stare at it for
a couple of minutes just to understand what it says. Still, i don't object to
it completely and the "valid"-var can go. I'd rather keep the "unsigned long
bio_sectors;" part and do something of the likes of:

bio_sectors = bio_sectors(failed_command->bio);


(remove the "if (bio_sectors < 4)"-test)

... and later...

sector &= ~(max(bio_sectors - 1, 3));

which is, IMO, more readable.

Roel, would you redo your patch please? Thanks.

-- 
Regards/Gruß,
Boris

Re: [PATCH 3/6] IDE: ide-cd: fix test unsigned var < 0

[Roel Kluin]

Boris Petkov wrote:

> I'd rather keep the "unsigned long bio_sectors;" part and do something
> of the likes of:
> 
> bio_sectors = bio_sectors(failed_command->bio);
> 
> 
> (remove the "if (bio_sectors < 4)"-test)
> 
> ... and later...
> 
> sector &= ~(max(bio_sectors - 1, 3));
> 
> which is, IMO, more readable.
> 
> Roel, would you redo your patch please? Thanks.

While at it I also cleaned up some whitespace
---
Clean up cdrom_analyze_sense_data()

Signed-off-by: Roel Kluin <12o3l@tiscali.nl>
---
diff --git a/drivers/ide/ide-cd.c b/drivers/ide/ide-cd.c
index a60f5df..df9bf0d 100644
--- a/drivers/ide/ide-cd.c
+++ b/drivers/ide/ide-cd.c
@@ -133,65 +133,61 @@ static int cdrom_log_sense(ide_drive_t *drive, struct request *rq,
 		default:
 			log = 1;
 			break;
 	}
 	return log;
 }
 
-static
-void cdrom_analyze_sense_data(ide_drive_t *drive,
+static void cdrom_analyze_sense_data(ide_drive_t *drive,
 			      struct request *failed_command,
 			      struct request_sense *sense)
 {
 	unsigned long sector;
 	unsigned long bio_sectors;
-	unsigned long valid;
 	struct cdrom_info *info = drive->driver_data;
 
 	if (!cdrom_log_sense(drive, failed_command, sense))
 		return;
 
 	/*
 	 * If a read toc is executed for a CD-R or CD-RW medium where
 	 * the first toc has not been recorded yet, it will fail with
 	 * 05/24/00 (which is a confusing error)
 	 */
 	if (failed_command && failed_command->cmd[0] == GPCMD_READ_TOC_PMA_ATIP)
 		if (sense->sense_key == 0x05 && sense->asc == 0x24)
 			return;
 
- 	if (sense->error_code == 0x70) {	/* Current Error */
- 		switch(sense->sense_key) {
+	if (sense->error_code == 0x70) {	/* Current Error */
+		switch (sense->sense_key) {
 		case MEDIUM_ERROR:
 		case VOLUME_OVERFLOW:
 		case ILLEGAL_REQUEST:
 			if (!sense->valid)
 				break;
 			if (failed_command == NULL ||
 					!blk_fs_request(failed_command))
 				break;
 			sector = (sense->information[0] << 24) |
 				 (sense->information[1] << 16) |
 				 (sense->information[2] <<  8) |
 				 (sense->information[3]);
 
-			bio_sectors = bio_sectors(failed_command->bio);
-			if (bio_sectors < 4)
-				bio_sectors = 4;
 			if (drive->queue->hardsect_size == 2048)
 				sector <<= 2;	/* Device sector size is 2K */
-			sector &= ~(bio_sectors -1);
-			valid = (sector - failed_command->sector) << 9;
+
+			bio_sectors = bio_sectors(failed_command->bio);
+			sector &= ~(max(bio_sectors - 1, 3));
 
 			if (sector < get_capacity(info->disk) &&
 				drive->probed_capacity - sector < 4 * 75) {
 				set_capacity(info->disk, sector);
 			}
- 		}
- 	}
+		}
+	}
 
 	ide_cd_log_error(drive->name, failed_command, sense);
 }
 
 /*
  * Initialize a ide-cd packet command request
  */

Re: [PATCH 3/6] IDE: ide-cd: fix test unsigned var < 0

[Boris Petkov]

On Thu, Apr 17, 2008 at 1:04 PM, Roel Kluin <12o3l@tiscali.nl> wrote:
> Boris Petkov wrote:
>
>  > I'd rather keep the "unsigned long bio_sectors;" part and do something
>  > of the likes of:
>  >
>  > bio_sectors = bio_sectors(failed_command->bio);
>  >
>  >
>  > (remove the "if (bio_sectors < 4)"-test)
>  >
>  > ... and later...
>  >
>  > sector &= ~(max(bio_sectors - 1, 3));
>  >
>  > which is, IMO, more readable.
>  >
>  > Roel, would you redo your patch please? Thanks.
>
>  While at it I also cleaned up some whitespace
>
> ---
>  Clean up cdrom_analyze_sense_data()
>
>  Signed-off-by: Roel Kluin <12o3l@tiscali.nl>

Thanks a lot.

Acked-by: Borislav Petkov <petkovbb@gmail.com>


-- 
Regards/Gruß,
Boris

Re: [PATCH 3/6] IDE: ide-cd: fix test unsigned var < 0

[Bartlomiej Zolnierkiewicz]

On Wednesday 16 April 2008, Roel Kluin wrote:
> Is this the right fix in this case?
> ---
> valid is unsigned and cannot be below 0.,
>     
> Signed-off-by: Roel Kluin <12o3l@tiscali.nl>

applied

Re: [PATCH 3/6] IDE: ide-cd: fix test unsigned var < 0

[Bartlomiej Zolnierkiewicz]

On Thursday 17 April 2008, Boris Petkov wrote:
> On Thu, Apr 17, 2008 at 1:04 PM, Roel Kluin <12o3l@tiscali.nl> wrote:
> > Boris Petkov wrote:
> >
> >  > I'd rather keep the "unsigned long bio_sectors;" part and do something
> >  > of the likes of:
> >  >
> >  > bio_sectors = bio_sectors(failed_command->bio);
> >  >
> >  >
> >  > (remove the "if (bio_sectors < 4)"-test)
> >  >
> >  > ... and later...
> >  >
> >  > sector &= ~(max(bio_sectors - 1, 3));

drivers/ide/ide-cd.c: In function ‘cdrom_analyze_sense_data’:
drivers/ide/ide-cd.c:180: warning: comparison of distinct pointer types lacks a cast

[ which actually hints us into real issue -> please think what would
  happen if bio_sectors() returns _zero_ before and after the patch ]

I applied the patch replacing the above code with:

	bio_sectors = max(bio_sectors(failed_command->bio), 4U);
	sector &= ~(bio_sectors - 1);

Thanks,
Bart