sg: fix read() error reporting

PATCH In review Tony Battersby 2 participants 4 messages Feb 11, 2015 – Feb 12, 2015

Subsystems (2)

SCSI SCSI SG driver

lore

Share & tools

lore

Tony Battersby

author

2015-02-11 16:32 UTC #0 lore raw

PATCH 1/1 sg: fix read() error reporting

Fix SCSI generic read() incorrectly returning success after detecting an error.

Cc <[email protected]>

Signed-off-by Tony Battersby <[email protected]>

---

For inclusion in kernel 3.20.

drivers/scsi/sg.c renamed from drivers/scsi/sg.c.orig +3 -3

@@ -546,7 +546,7 @@ static ssize_t
 sg_new_read(Sg_fd * sfp, char __user *buf, size_t count, Sg_request * srp)
 {
 	sg_io_hdr_t *hp = &srp->header;
-	int err = 0;
+	int err = 0, err2;
 	int len;
 
 	if (count < SZ_SG_IO_HDR) {

@@ -575,8 +575,8 @@ sg_new_read(Sg_fd * sfp, char __user *bu
 		goto err_out;
 	}
 err_out:
-	err = sg_finish_rem_req(srp);
-	return (0 == err) ? count : err;
+	err2 = sg_finish_rem_req(srp);
+	return err ? : err2 ? : count;
 }
 
 static ssize_t

Douglas Gilbert

maintainer

2015-02-11 17:45 UTC #1 lore raw

Quoted 21 lines · Tony Battersby

Fix SCSI generic read() incorrectly returning success after detecting an error.
Cc <[email protected]>
Signed-off-by Tony Battersby <[email protected]>
---
For inclusion in kernel 3.20.
drivers/scsi/sg.c renamed from drivers/scsi/sg.c.orig +1 -1
@@ -546,7 +546,7 @@ static ssize_t
  sg_new_read(Sg_fd * sfp, char __user *buf, size_t count, Sg_request * srp)
  {
  	sg_io_hdr_t *hp = &srp->header;
-	int err = 0;
+	int err = 0, err2;
  	int len;
 
  	if (count < SZ_SG_IO_HDR) {

drivers/scsi/sg.c sg_new_read(Sg_fd * sfp, char __user *bu · line 579

Quoted 7 lines

drivers/scsi/sg.c renamed from drivers/scsi/sg.c.orig +2 -2

@@ -575,8 +575,8 @@ sg_new_read(Sg_fd * sfp, char __user *bu
  		goto err_out;
  	}
  err_out:
-	err = sg_finish_rem_req(srp);
-	return (0 == err) ? count : err;
+	err2 = sg_finish_rem_req(srp);
+	return err ? : err2 ? : count;

Tony,
Your point is well made.

I just don't like that last line, using a gcc extension that hasn't even made it into C11 (or C++11). Wouldn't:

    return err ? err : (err2 ? err2 : count);

be a bit better? I think the following snippet makes the intent clear but would it generate any more code:

    if (err || err2)
	return err ? err : err2;
    else
        return count;

Doug Gilbert

Tony Battersby

author

2015-02-11 18:13 UTC #2 lore raw

Quoted 36 lines · Douglas Gilbert

Quoted 21 lines · Tony Battersby
Fix SCSI generic read() incorrectly returning success after detecting an error.
Cc <[email protected]>
Signed-off-by Tony Battersby <[email protected]>
---
For inclusion in kernel 3.20.
drivers/scsi/sg.c renamed from drivers/scsi/sg.c.orig +1 -1
@@ -546,7 +546,7 @@ static ssize_t
  sg_new_read(Sg_fd * sfp, char __user *buf, size_t count, Sg_request * srp)
  {
  	sg_io_hdr_t *hp = &srp->header;
-	int err = 0;
+	int err = 0, err2;
  	int len;
 
  	if (count < SZ_SG_IO_HDR) {
drivers/scsi/sg.c sg_new_read(Sg_fd * sfp, char __user *bu · line 579
Quoted 7 lines
drivers/scsi/sg.c renamed from drivers/scsi/sg.c.orig +2 -2
@@ -575,8 +575,8 @@ sg_new_read(Sg_fd * sfp, char __user *bu
  		goto err_out;
  	}
  err_out:
-	err = sg_finish_rem_req(srp);
-	return (0 == err) ? count : err;
+	err2 = sg_finish_rem_req(srp);
+	return err ? : err2 ? : count;
Tony,
Your point is well made.
I just don't like that last line, using a gcc extension that hasn't even made it into C11 (or C++11). Wouldn't:
     return err ? err : (err2 ? err2 : count);

Quoted 11 lines

be a bit better? I think the following snippet makes the intent clear but would it generate any more code:
     if (err || err2)
	return err ? err : err2;
     else
         return count;
Doug Gilbert

I checked before submitting, and the foo ? : bar construct is already used at least 455 times in the kernel, so it is nothing new.

find linux-3.19 -type f -name "*.[ch]" | xargs grep '\? :' | wc -l 455

But you are probably right; I think that with such small variable names your version is more readable due to the nesting. Updated patch below.

---

Fix SCSI generic read() incorrectly returning success after detecting an error.

Cc <[email protected]>

Signed-off-by Tony Battersby <[email protected]>

---

drivers/scsi/sg.c renamed from drivers/scsi/sg.c.orig +3 -3

@@ -546,7 +546,7 @@ static ssize_t
 sg_new_read(Sg_fd * sfp, char __user *buf, size_t count, Sg_request * srp)
 {
 	sg_io_hdr_t *hp = &srp->header;
-	int err = 0;
+	int err = 0, err2;
 	int len;
 
 	if (count < SZ_SG_IO_HDR) {

@@ -575,8 +575,8 @@ sg_new_read(Sg_fd * sfp, char __user *bu
 		goto err_out;
 	}
 err_out:
-	err = sg_finish_rem_req(srp);
-	return (0 == err) ? count : err;
+	err2 = sg_finish_rem_req(srp);
+	return err ? err : (err2 ? err2 : count);
 }
 
 static ssize_t

Douglas Gilbert

maintainer Acked-by

2015-02-12 23:31 UTC #3 lore raw

Quoted 33 lines · Tony Battersby

Quoted 32 lines · Douglas Gilbert
Quoted 21 lines · Tony Battersby
Fix SCSI generic read() incorrectly returning success after detecting an error.
Cc <[email protected]>
Signed-off-by Tony Battersby <[email protected]>
---
For inclusion in kernel 3.20.
drivers/scsi/sg.c renamed from drivers/scsi/sg.c.orig +1 -1
@@ -546,7 +546,7 @@ static ssize_t
   sg_new_read(Sg_fd * sfp, char __user *buf, size_t count, Sg_request * srp)
   {
   	sg_io_hdr_t *hp = &srp->header;
-	int err = 0;
+	int err = 0, err2;
   	int len;
 
   	if (count < SZ_SG_IO_HDR) {
drivers/scsi/sg.c sg_new_read(Sg_fd * sfp, char __user *bu · line 579
Quoted 7 lines
drivers/scsi/sg.c renamed from drivers/scsi/sg.c.orig +2 -2
@@ -575,8 +575,8 @@ sg_new_read(Sg_fd * sfp, char __user *bu
   		goto err_out;
   	}
   err_out:
-	err = sg_finish_rem_req(srp);
-	return (0 == err) ? count : err;
+	err2 = sg_finish_rem_req(srp);
+	return err ? : err2 ? : count;
Tony,
Your point is well made.

Quoted 7 lines

Quoted 7 lines
I just don't like that last line, using a gcc extension that hasn't even made it into C11 (or C++11). Wouldn't:
      return err ? err : (err2 ? err2 : count);
be a bit better? I think the following snippet makes the intent clear but would it generate any more code:
      if (err || err2)

An improvement on the above line would be:

      if (unlikely(err || err2))

Quoted 4 lines

	return err ? err : err2;
      else
          return count;

Another approach is to return the good path prior to the err_out label. This would require two calls to sg_finish_rem_req().

However Tony's clean-up is sufficient.

Quoted 9 lines

I checked before submitting, and the foo ? : bar construct is already used at least 455 times in the kernel, so it is nothing new.
find linux-3.19 -type f -name "*.[ch]" | xargs grep '\? :' | wc -l 455
But you are probably right; I think that with such small variable names your version is more readable due to the nesting. Updated patch below.

Quoted 7 lines

---
Fix SCSI generic read() incorrectly returning success after detecting an error.
Cc <[email protected]>
Signed-off-by Tony Battersby <[email protected]>

Acked-by Douglas Gilbert <[email protected]>

Quoted 26 lines

---

drivers/scsi/sg.c renamed from drivers/scsi/sg.c.orig +3 -4

@@ -546,7 +546,7 @@ static ssize_t
  sg_new_read(Sg_fd * sfp, char __user *buf, size_t count, Sg_request * srp)
  {
  	sg_io_hdr_t *hp = &srp->header;
-	int err = 0;
+	int err = 0, err2;
  	int len;
 
  	if (count < SZ_SG_IO_HDR) {

@@ -575,8 +575,8 @@ sg_new_read(Sg_fd * sfp, char __user *bu
  		goto err_out;
  	}
  err_out:
-	err = sg_finish_rem_req(srp);
-	return (0 == err) ? count : err;
+	err2 = sg_finish_rem_req(srp);
+	return err ? err : (err2 ? err2 : count);
  }
 
  static ssize_t
 
--