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>