Would using

	switch (retval) {
	case ERROR:
	    {
		char buf[256], *errbuf = &buf;

		/* do error handling */

		break;
	    }

suffice?

Jaromir

YAMAMOTO Takashi wrote:
> 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>

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


-- 
Jaromir Dolecek <jdolecek@NetBSD.org> http://www.NetBSD.org/Ports/i386/ps2.html
-=- We should be mindful of the potential goal, but as the tantric    -=-
-=- Buddhist masters say, ``You may notice during meditation that you -=-
-=- sometimes levitate or glow.   Do not let this distract you.''     -=-