Date: Thu, 12 Jul 2001 01:25:17 -0700 From: Dima Dorfman <dima@unixfreak.org> To: Matt Dillon <dillon@earth.backplane.com> Cc: Mark Peek <mark@whistle.com>, stable@FreeBSD.ORG Subject: Re: cron exited on signal 11 Message-ID: <20010712082517.E7B7D3E2F@bazooka.unixfreak.org> In-Reply-To: <200107120809.f6C898Q42216@earth.backplane.com>; from dillon@earth.backplane.com on "Thu, 12 Jul 2001 01:09:08 -0700 (PDT)"
next in thread | previous in thread | raw e-mail | index | archive | help
Matt Dillon <dillon@earth.backplane.com> writes: > :On the contrary, I think free_entry() should assume that all members > :aren't NULL because if they're NULL, it's probably a bug (in which > :case a core dump is advisable). > : > :Below is a copy of an e-mail I sent to Mark Peek on this topic. The > :only thing I'd like to add is that the patch below addresses two > :different problems. The first is that free_entry() simply shouldn't > :be called after the eof: label. The second is that the `class' member > :is unnecessarily treated inconsistently from the others. They really > :don't belong in the same patch, because the former is a bugfix while > :the latter is a style/consistency fix. > > Well, I think I disagree here... the eof label does a 'general cleanup'. > It is entirely appropriate for it to call free_entry() here, as long > as free_entry() is designed to handle a partially instantiated structure. > This is a fairly typical programming practice used to avoid having to > do piecemeal unwinds of partially instantiated structures all over the > code (which is what you did in your patch). So I believe the > proper solution is to imply make it so that free_entry() can deal with > a partially instantiated structure - a one line fix. As I said, this is a matter of opinion. I happen to agree with you, but cron *already* does piecemeal unwinds. Just look at the example fragment in my e-mail: > :> : ch = get_string(cmd, MAX_COMMAND, file, "\n"); > :> : > :> : /* a file without a \n before the EOF is rude, so we'll complain... > :> : */ > :> : if (ch == EOF) { > :> : env_free(e->envp); > :> : ecode = e_cmd; > :> : goto eof; > :> : } Notice the call to env_free(). That routine does proper piecemeal cleanup in all cases and for all members of the structure except `class' (which wasn't there when the original author wrote the program). It may not have been the right choice, but it's already been made, and I think remaining consistent with the rest of the file (or routine) is desirable. Changing free_entry() to support partially initialized structures may mask bugs. This is the only routine that initializes the structure and it does it completely; if a field isn't initialized, that's a bug. I guess what I'm saying is that although I wouldn't write it like that, since it's already written it'd be nice to remain consistent. Regards, Dima Dorfman dima@unixfreak.org To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-stable" in the body of the message
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20010712082517.E7B7D3E2F>