From owner-freebsd-current Mon Dec 31 7:12:20 2001 Delivered-To: freebsd-current@freebsd.org Received: from mailman.zeta.org.au (mailman.zeta.org.au [203.26.10.16]) by hub.freebsd.org (Postfix) with ESMTP id AC05C37B423; Mon, 31 Dec 2001 07:12:03 -0800 (PST) Received: from bde.zeta.org.au (bde.zeta.org.au [203.2.228.102]) by mailman.zeta.org.au (8.9.3/8.8.7) with ESMTP id CAA02899; Tue, 1 Jan 2002 02:11:53 +1100 Date: Tue, 1 Jan 2002 02:11:44 +1100 (EST) From: Bruce Evans X-X-Sender: To: Maxim Konovalov Cc: Sheldon Hearn , Poul-Henning Kamp , , Subject: Re: Junior Kernel Hacker Task: ccdinit stack usage. In-Reply-To: <20011230160821.L36208-100000@news1.macomnet.ru> Message-ID: <20020101015003.H6507-100000@gamplex.bde.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Sender: owner-freebsd-current@FreeBSD.ORG Precedence: bulk List-ID: List-Archive: (Web Archive) List-Help: (List Instructions) List-Subscribe: List-Unsubscribe: X-Loop: FreeBSD.ORG 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