----Next_Part(Thu_Jun_27_17:16:22_2002_343)--
Content-Type: Text/Plain; charset=us-ascii
Content-Transfer-Encoding: 7bit

From: David Laight <david@l8s.co.uk>
Subject: Re: wd.c patch to reduce kernel stack usage
Date: Wed, 26 Jun 2002 21:51:33 +0100

> > is attached patch ok?
> > (in order to reduce kernel stack usage.)
> 
> I'd be tempted to go the whole hog and make wdperror malloc()
> a buffer that the caller has to free().
> 
> That way the buffer cam be allocated by code that knows how big
> it needs to be (and use snprint to ensure it isn't overrun.

like this?

---
YAMAMOTO Takashi<yamt@mwd.biglobe.ne.jp>

----Next_Part(Thu_Jun_27_17:16:22_2002_343)--
Content-Type: Text/Plain; charset=us-ascii
Content-Transfer-Encoding: 7bit
Content-Disposition: inline; filename="wd.diff"

Index: wd.c
===================================================================
RCS file: /cvs/cvsroot/syssrc/sys/dev/ata/wd.c,v
retrieving revision 1.220
diff -u -p -r1.220 wd.c
--- wd.c	2002/01/13 17:24:30	1.220
+++ wd.c	2002/06/27 08:16:45
@@ -181,7 +181,7 @@ void	wdattach	__P((struct device *, stru
 int	wddetach __P((struct device *, int));
 int	wdactivate __P((struct device *, enum devact));
 int	wdprint	__P((void *, char *));
-void    wdperror __P((struct ata_drive_datas *, int, char *));
+char *	wdperror __P((struct ata_drive_datas *, int));
 
 struct cfattach wd_ca = {
 	sizeof(struct wd_softc), wdprobe, wdattach, wddetach, wdactivate
@@ -589,14 +589,14 @@ wddone(v)
 {
 	struct wd_softc *wd = v;
 	struct buf *bp = wd->sc_bp;
-	char buf[256], *errbuf = buf;
+	char *buf = 0;
+	const char *errbuf;
 	WDCDEBUG_PRINT(("wddone %s\n", wd->sc_dev.dv_xname),
 	    DEBUG_XFERS);
 
 	if (bp == NULL)
 		return;
 	bp->b_resid = wd->sc_wdc_bio.bcount;
-	errbuf[0] = '\0';
 	switch (wd->sc_wdc_bio.error) {
 	case ERR_DMA:
 		errbuf = "DMA error";
@@ -612,11 +612,17 @@ wddone(v)
 		if (wd->sc_wdc_bio.r_error != 0 &&
 		    (wd->sc_wdc_bio.r_error & ~(WDCE_MC | WDCE_MCR)) == 0)
 			goto noerror;
-		wdperror(wd->drvp, wd->sc_wdc_bio.r_error, errbuf);
+		buf = wdperror(wd->drvp, wd->sc_wdc_bio.r_error);
+		if (buf)
+			errbuf = buf;
+		else
+			errbuf = ""; /* XXX */
 retry:		/* Just reset and retry. Can we do more ? */
 		wd->atabus->ata_reset_channel(wd->drvp);
 		diskerr(bp, "wd", errbuf, LOG_PRINTF,
 		    wd->sc_wdc_bio.blkdone, wd->sc_dk.dk_label);
+		if (buf)
+			free((void *)buf, M_DEVBUF);
 		if (wd->retries++ < WDIORETRIES) {
 			printf(", retrying\n");
 			callout_reset(&wd->sc_restart_ch, RECOVERYTIME,
@@ -937,12 +943,14 @@ wdgetdisklabel(wd)
 #endif
 }
 
-void
-wdperror(drvp, errno, buf)
+char *
+wdperror(drvp, errno)
 	struct ata_drive_datas *drvp;
 	int errno;
-	char *buf;
 {
+#define	WD_MAX_ERROR_LEN	256
+	char *buf, *buf0;
+	size_t buflen;
 	static char *errstr0_3[] = {"address mark not found",
 	    "track 0 not found", "aborted command", "media change requested",
 	    "id not found", "media changed", "uncorrectable data error",
@@ -955,20 +963,41 @@ wdperror(drvp, errno, buf)
 	int i;
 	char *sep = "";
 
+	buflen = WD_MAX_ERROR_LEN;
+	buf = malloc(buflen, M_DEVBUF, M_NOWAIT);
+	if (buf == NULL)
+		return NULL;
+
 	if (drvp->ata_vers >= 4)
 		errstr = errstr4_5;
 	else
 		errstr = errstr0_3;
 
-	if (errno == 0)
-		sprintf(buf, "error not notified");
+	if (errno == 0) {
+		if (snprintf(buf, buflen, "error not notified") < 0)
+			goto error;
+		return buf;
+	}
 
+	buf0 = buf;
 	for (i = 0; i < 8; i++) {
 		if (errno & (1 << i)) {
-			buf += sprintf(buf, "%s%s", sep, errstr[i]);
+			int ret;
+
+			ret = snprintf(buf, buflen, "%s%s", sep, errstr[i]);
+			if (ret < 0) {
+error:
+				free(buf0, M_DEVBUF);
+				return NULL;
+			}
+			if (ret >= buflen)
+				break;
+			buf += ret;
+			buflen -= ret;
 			sep = ", ";
 		}
 	}
+	return buf0;
 }
 
 int
@@ -1235,7 +1264,7 @@ wddump(dev, blkno, va, size)
 	struct disklabel *lp;   /* disk's disklabel */
 	int part, err;
 	int nblks;	/* total number of sectors left to write */
-	char errbuf[256];
+	char *errbuf;
 
 	/* Check if recursive dump; if so, punt. */
 	if (wddoingadump)
@@ -1313,8 +1342,13 @@ again:
 			break;
 		case ERROR:
 			errbuf[0] = '\0';
-			wdperror(wd->drvp, wd->sc_wdc_bio.r_error, errbuf);
-			printf("wddump: %s", errbuf);
+			errbuf = wdperror(wd->drvp, wd->sc_wdc_bio.r_error);
+			if (errbuf) {
+				printf("wddump: %s", errbuf);
+				free(errbuf, M_DEVBUF);
+			}
+			else
+				printf("wddump: error");
 			err = EIO;
 			break;
 		case NOERROR: 

----Next_Part(Thu_Jun_27_17:16:22_2002_343)----