IDE: ide-cd: fix test unsigned var < 0

PATCH Applied Roel Kluin 3 participants 8 messages Apr 16, 2008 – Apr 27, 2008 libata

lore

Share & tools

lore

Roel Kluin

author

2008-04-16 02:06 UTC #0 lore raw

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

Is this the right fix in this case?
---
valid is unsigned and cannot be below 0.,

Signed-off-by Roel Kluin <[email protected]>

---

drivers/ide/ide-cd.c +0 -2

index c8d0e87..a60f5df 100644
@@ -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);

Bartlomiej Zolnierkiewicz

2008-04-16 22:27 UTC #1 lore raw

Hi,

Quoted 1 line · Roel Kluin

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

Quoted 18 lines

---
valid is unsigned and cannot be below 0.,

Signed-off-by Roel Kluin <[email protected]>

---

drivers/ide/ide-cd.c +0 -2

index c8d0e87..a60f5df 100644
@@ -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);

Roel Kluin

author

2008-04-17 03:51 UTC #2 lore raw

Quoted 5 lines · Bartlomiej Zolnierkiewicz

Hi,
Quoted 1 line · Roel Kluin
Is this the right fix in this case?

Quoted 7 lines

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 <[email protected]>

---

drivers/ide/ide-cd.c +1 -7

index a60f5df..a5f3beb 100644
@@ -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) {

Boris Petkov

2008-04-17 09:12 UTC #3 lore raw

Quoted 49 lines · Roel Kluin

Quoted 5 lines · Bartlomiej Zolnierkiewicz
Hi,
Quoted 1 line · Roel Kluin
Is this the right fix in this case?
Quoted 7 lines
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 <[email protected]>
---
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);

Quoted 2 lines

- 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.

Signature · 2 lines

Regards/Gruß,
Boris

Roel Kluin

author

2008-04-17 11:04 UTC #4 lore raw

Quoted 8 lines · Boris Petkov

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)

Quoted 7 lines

... 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 <[email protected]>

---

drivers/ide/ide-cd.c +8 ~~-12~~

index a60f5df..df9bf0d 100644
@@ -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
  */

Boris Petkov

Acked-by Borislav Petkov

2008-04-17 12:01 UTC #5 lore raw

Quoted 16 lines · Roel Kluin

Quoted 14 lines · Boris Petkov
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.

Quoted 8 lines

Quoted 1 line
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 <[email protected]>

Thanks a lot.

Acked-by Borislav Petkov <[email protected]>

Signature · 2 lines

Regards/Gruß,
Boris

Bartlomiej Zolnierkiewicz

2008-04-27 19:54 UTC #6 lore raw

Quoted 6 lines · Boris Petkov

Quoted 5 lines · Roel Kluin
Quoted 3 lines · Boris Petkov
I'd rather keep the "unsigned long bio_sectors;" part and do something of the likes of:

Quoted 8 lines

Quoted 8 lines
Quoted 8 lines
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

Bartlomiej Zolnierkiewicz

Applied

2008-04-27 19:54 UTC #7 lore raw

In reply to Roel Kluin

Quoted 5 lines · Roel Kluin

Is this the right fix in this case?
---
valid is unsigned and cannot be below 0.,
Signed-off-by Roel Kluin <[email protected]>

applied