Re: Junior Kernel Hacker Task: ccdinit stack usage.
On Sun, 30 Dec 2001, Poul-Henning Kamp wrote: In message [EMAIL PROTECTED], Maxim Konovalov wr ites: Ohh and I forgot: Thanks for the patch! Tested Committed :-) Junior committer task: fix style bugs in submitted patches before committing. Bruce To Unsubscribe: send mail to [EMAIL PROTECTED] with unsubscribe freebsd-current in the body of the message
Re: Junior Kernel Hacker Task: ccdinit stack usage.
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 - 1.95
+++ ccd.c 30 Dec 2001 13:42:05 -
@@ -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 [EMAIL PROTECTED]
with unsubscribe freebsd-current in the body of the message
Re: Junior Kernel Hacker Task: ccdinit stack usage.
Hello,
On 12:23+0100, Dec 30, 2001, Poul-Henning Kamp wrote:
sys/dev/ccd/ccd.c:ccdinit() has a couple of very large items on
the stack.
Rewrite ccdinit() to allocate them with MALLOC(9) instead.
tmppath is a rather big one but I can't find the second item. What
about this patch:
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 - 1.95
+++ ccd.c 30 Dec 2001 12:15:25 -
@@ -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
@@ -414,6 +414,7 @@
*/
maxsecsize = 0;
minsize = 0;
+ MALLOC(tmppath, char *, MAXPATHLEN, M_DEVBUF, M_WAITOK);
for (ix = 0; ix cs-sc_nccdisks; ix++) {
vp = cs-sc_vpp[ix];
ci = cs-sc_cinfo[ix];
@@ -422,7 +423,7 @@
/*
* Copy in the pathname of the component.
*/
- bzero(tmppath, sizeof(tmppath));/* sanity */
+ bzero(tmppath, MAXPATHLEN); /* sanity */
if ((error = copyinstr(cpaths[ix], tmppath,
MAXPATHLEN, ci-ci_pathlen)) != 0) {
#ifdef DEBUG
@@ -487,6 +488,8 @@
ci-ci_size = size;
cs-sc_size += size;
}
+
+ FREE(tmppath, M_DEVBUF);
/*
* Don't allow the interleave to be smaller than
--
Maxim Konovalov, MAcomnet, Internet-Intranet Dept., system engineer
phone: +7 (095) 796-9079, mailto: [EMAIL PROTECTED]
To Unsubscribe: send mail to [EMAIL PROTECTED]
with unsubscribe freebsd-current in the body of the message
Re: Junior Kernel Hacker Task: ccdinit stack usage.
[Chad David copied for last comment in message.] On Sun, 30 Dec 2001 15:28:23 +0300, Maxim Konovalov wrote: sys/dev/ccd/ccd.c:ccdinit() has a couple of very large items on the stack. Rewrite ccdinit() to allocate them with MALLOC(9) instead. tmppath is a rather big one but I can't find the second item. What about this patch: I think the others are the partinfo and ccdgeom structures. Note that you don't need to (and shouldn't as per style(9)) initialize tmppath to NULL. Also, your bzero() is unnecessary if you use the M_ZERO flag to MALLOC(9). As an aside, what's the undocumented M_DEVBUF flag for? Ciao, Sheldon. To Unsubscribe: send mail to [EMAIL PROTECTED] with unsubscribe freebsd-current in the body of the message
Re: Junior Kernel Hacker Task: ccdinit stack usage.
On Sun, 30 Dec 2001 16:45:31 +0300, Maxim Konovalov wrote: struct partinfo holds only two pointers, ccg is a pointer too. I got confused with partinfo and disklabel, and didn't actually check ccg. *blush* 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. Bruce Evans will probably want tmppath initialized to NULL near the top of the function's code, rather than in the declarations. :-) man 9 malloc: : A type is defined using the malloc_type_t typedef via the : MALLOC_DECLARE() and MALLOC_DEFINE() macros. Take a look at /usr/src/sys/sys/malloc.h. That only tells me where it comes from, not when I'd want to use it. :-) What about this one: It's probably worth giving Poul-Henning time to wake up. ;-) Ciao, Sheldon. To Unsubscribe: send mail to [EMAIL PROTECTED] with unsubscribe freebsd-current in the body of the message
Re: Junior Kernel Hacker Task: ccdinit stack usage.
In message [EMAIL PROTECTED], Maxim Konovalov wr ites: tmppath is a rather big one but I can't find the second item. What about this patch: I think the others are the partinfo and ccdgeom structures. struct partinfo holds only two pointers, ccg is a pointer too. I meant partinfo, but I hadn't looked at it in enough detail to notice how small it actually was. My fault. As an aside, what's the undocumented M_DEVBUF flag for? It's a catch-all malloc bucket for things in drivers which are not worth allocating their own bucket for. M_TEMP is a similar catch-all bucket for which I belive the rule-of-thumb is that nothing should remain allocated by the time we return to userland. -- Poul-Henning Kamp | UNIX since Zilog Zeus 3.20 [EMAIL PROTECTED] | TCP/IP since RFC 956 FreeBSD committer | BSD since 4.3-tahoe Never attribute to malice what can adequately be explained by incompetence. To Unsubscribe: send mail to [EMAIL PROTECTED] with unsubscribe freebsd-current in the body of the message
Re: Junior Kernel Hacker Task: ccdinit stack usage.
In message [EMAIL PROTECTED], Maxim Konovalov wr ites: Ohh and I forgot: Thanks for the patch! Tested Committed :-) -- Poul-Henning Kamp | UNIX since Zilog Zeus 3.20 [EMAIL PROTECTED] | TCP/IP since RFC 956 FreeBSD committer | BSD since 4.3-tahoe Never attribute to malice what can adequately be explained by incompetence. To Unsubscribe: send mail to [EMAIL PROTECTED] with unsubscribe freebsd-current in the body of the message
