Date: Tue, 1 Jan 2002 02:11:44 +1100 (EST) From: Bruce Evans <bde@zeta.org.au> To: Maxim Konovalov <maxim@macomnet.ru> Cc: Sheldon Hearn <sheldonh@starjuice.net>, Poul-Henning Kamp <phk@FreeBSD.ORG>, <current@FreeBSD.ORG>, <chad@FreeBSD.ORG> Subject: Re: Junior Kernel Hacker Task: ccdinit stack usage. Message-ID: <20020101015003.H6507-100000@gamplex.bde.org> In-Reply-To: <20011230160821.L36208-100000@news1.macomnet.ru>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sun, 30 Dec 2001, Maxim Konovalov wrote:
> On 15:04+0200, Dec 30, 2001, Sheldon Hearn wrote:
> > Note that you don't need to (and shouldn't as per style(9)) initialize
> > tmppath to NULL.
>
> Do you mean:
>
> : Be careful to not obfuscate the code by initializing variables
> : in the declarations. Use this feature only thoughtfully. DO NOT
> : use function calls in initializers.
>
> or something else?
Yes. That and the fact that the initialization has no effect.
> > Also, your bzero() is unnecessary if you use the M_ZERO flag to
> > MALLOC(9).
>
> MALLOC(9) is above on for() cycle but bzero(3) is not needed even
> without M_ZERO because copyinstr(9) copies the terminating NULL too.
> bzero(3) was in original code so I decided to leave it.
MALLOC(9) should say: "The MALLOC(9) macro is deprecated. Do not use it
except for style-bug-for-bug compatibility with code that already uses it".
Note that ccd.c already uses the malloc(9) function for all malloc()-like
memory allocations, and that there is another rule about using a
consistent style.
If the result of malloc(9) were directly assigned to tmppath, then it
would be obvious that initializing it to NULL has no effect.
> Index: ccd.c
> ===================================================================
> RCS file: /home/ncvs/src/sys/dev/ccd/ccd.c,v
> retrieving revision 1.95
> diff -u -r1.95 ccd.c
> --- ccd.c 17 Nov 2001 00:46:08 -0000 1.95
> +++ ccd.c 30 Dec 2001 13:42:05 -0000
> @@ -394,7 +394,7 @@
> int maxsecsize;
> struct partinfo dpart;
> struct ccdgeom *ccg = &cs->sc_geom;
> - char tmppath[MAXPATHLEN];
> + char *tmppath = NULL;
> int error = 0;
>
> #ifdef DEBUG
See above.
> @@ -414,6 +414,7 @@
> */
> maxsecsize = 0;
> minsize = 0;
> + tmppath = malloc(MAXPATHLEN, M_DEVBUF, M_WAITOK);
I think the malloc type should be M_TEMP here. M_TEMP is certainly not
wrong for anything that could be a local variable if there were enough
space. Some device buffers could go on the stack if there were enough
space, since they don't need to live after the function returns, but
temppath is not a device buffer.
> @@ -422,7 +423,6 @@
> /*
> * Copy in the pathname of the component.
> */
> - bzero(tmppath, sizeof(tmppath)); /* sanity */
> if ((error = copyinstr(cpaths[ix], tmppath,
> MAXPATHLEN, &ci->ci_pathlen)) != 0) {
> #ifdef DEBUG
OK.
> @@ -488,6 +488,9 @@
> cs->sc_size += size;
> }
>
> + free(tmppath, M_DEVBUF);
> + tmppath = NULL;
> +
> /*
> * Don't allow the interleave to be smaller than
> * the biggest component sector.
This is not necessary, because the buffer has to be freed later in all of
the goto cases, and probably not very useful, because the allocation won't
live much longer. Removing it would fix the following style bugs:
- extra blank line before free().
- free() is not style-bug-for-bug compatible with MALLOC(). Code that
obfuscates malloc() using MALLOC() should also obfuscate free() using
FREE().
> @@ -577,6 +580,8 @@
> ci--;
> free(ci->ci_path, M_DEVBUF);
> }
> + if (tmppath != NULL)
> + free(tmppath, M_DEVBUF);
> free(cs->sc_cinfo, M_DEVBUF);
> return (error);
> }
OK
Bruce
To Unsubscribe: send mail to majordomo@FreeBSD.org
with "unsubscribe freebsd-current" in the body of the message
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20020101015003.H6507-100000>