Skip to contentSkip to conversation

st.ko: change enlarge_buffer result

PATCH In review Bodo Stroesser 3 participants 3 messages Dec 2, 2013 – Dec 16, 2013

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

PATCH 3/3 st.ko: change enlarge_buffer result

From: Bodo Stroesser <[email protected]>
Date: Mon, 2 Dec 2013 18:52:10 +0100
Subject: [PATCH 3/3] st.ko: change enlarge_buffer result

enlarge_buffer() just returns 1 or 0 if it could or could not allocate the requested buffer.

In case of result 0, the callers always set the error to EOVERFLOW. I think, this is not a good errno for those cases, where enlarge_buffer() could not allocate the pages it needed.

So I changed enlarge_buffer() to return a meaningful result (-ENOMEM or -EOVERFLOW in case of error, 0 in case of success) and the callers to use this result.

I also removed a check in setup_buffering() that is done in enlarge_buffer() anyway.

---

drivers/scsi/st.c +6 -9
@@ -1221,10 +1221,9 @@ static int st_open(struct inode *inode,
 	}
 
 	/* See that we have at least a one page buffer available */
-	if (!enlarge_buffer(STp->buffer, PAGE_SIZE, STp->restr_dma)) {
+	if ((retval = enlarge_buffer(STp->buffer, PAGE_SIZE, STp->restr_dma)) != 0) {
 		printk(KERN_WARNING "%s: Can't allocate one page tape buffer.\n",
 		       name);
-		retval = (-EOVERFLOW);
 		goto err_out;
 	}
 
@@ -1517,11 +1516,9 @@ static int setup_buffering(struct scsi_t
 				clear_buffer(STbp);
 		}
 
-		if (bufsize > STbp->buffer_size &&
-		    !enlarge_buffer(STbp, bufsize, STp->restr_dma)) {
+		if ((retval = enlarge_buffer(STbp, bufsize, STp->restr_dma)) != 0) {
 			printk(KERN_WARNING "%s: Can't allocate %d byte tape buffer.\n",
 			       tape_name(STp), bufsize);
-			retval = (-EOVERFLOW);
 			goto out;
 		}
 		if (STp->block_size)
@@ -3723,7 +3720,7 @@ static int enlarge_buffer(struct st_buff
 	gfp_t priority;
 
 	if (new_size <= STbuffer->buffer_size)
-		return 1;
+		return 0;
 
 	max_segs = STbuffer->use_sg;
 
@@ -3747,7 +3744,7 @@ static int enlarge_buffer(struct st_buff
 	}
 	if (max_segs * (PAGE_SIZE << order) < new_size) {
 		if (order == ST_MAX_ORDER)
-			return 0;
+			return -EOVERFLOW;
 		normalize_buffer(STbuffer);
 		return enlarge_buffer(STbuffer, new_size, need_dma);
 	}
@@ -3760,7 +3757,7 @@ static int enlarge_buffer(struct st_buff
 		if (!page) {
 			DEB(STbuffer->buffer_size = got);
 			normalize_buffer(STbuffer);
-			return 0;
+			return -ENOMEM;
 		}
 
 		STbuffer->frp_segs += 1;
@@ -3771,7 +3768,7 @@ static int enlarge_buffer(struct st_buff
 	}
 	STbuffer->b_data = page_address(STbuffer->reserved_pages[0]);
 
-	return 1;
+	return 0;
 }
 
 
#1 lore raw
Quoted 7 lines · Bodo Stroesser

From: Bodo Stroesser <[email protected]>
Date: Mon, 2 Dec 2013 18:52:10 +0100
Subject: [PATCH 3/3] st.ko: change enlarge_buffer result

enlarge_buffer() just returns 1 or 0 if it could or could not allocate the requested buffer.

Quoted 9 lines

In case of result 0, the callers always set the error to EOVERFLOW. I think, this is not a good errno for those cases, where enlarge_buffer() could not allocate the pages it needed.

So I changed enlarge_buffer() to return a meaningful result (-ENOMEM or -EOVERFLOW in case of error, 0 in case of success) and the callers to use this result.

ENOMEM is used for telling the user that, in variable block mode, the byte count in read() is smaller than the next block. This may not sound like proper use of this code but this is how the tape drivers have done. When ENOMEM is not used, the patch would be only cosmetic.

Thanks,
Kai

#2 lore raw
Quoted 24 lines

-----Original Message-----
From: "Kai Mäkisara (Kolumbus)" [mailto:[email protected]]
Sent: Monday, December 16, 2013 7:49 PM
To: Strösser, Bodo
Cc: [email protected]
Subject: Re: [PATCH 3/3] st.ko: change enlarge_buffer result

Quoted 12 lines · Bodo Stroesser

From: Bodo Stroesser <[email protected]>
Date: Mon, 2 Dec 2013 18:52:10 +0100
Subject: [PATCH 3/3] st.ko: change enlarge_buffer result

enlarge_buffer() just returns 1 or 0 if it could or could not allocate the requested buffer.

In case of result 0, the callers always set the error to EOVERFLOW. I think, this is not a good errno for those cases, where enlarge_buffer() could not allocate the pages it needed.

Quoted 7 lines
Quoted 4 lines

So I changed enlarge_buffer() to return a meaningful result (-ENOMEM or -EOVERFLOW in case of error, 0 in case of success) and the callers to use this result.

ENOMEM is used for telling the user that, in variable block mode, the byte count in read() is smaller than the next block. This may not sound like proper use of this code but this is how the tape drivers have done.

Oops, sorry, I missed that. So I agree fully, that the patch does not make sense.

Thank you,
Bodo

Quoted 5 lines

When ENOMEM is not used, the patch would be only cosmetic.

Thanks,
Kai

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