From owner-freebsd-stable Thu Jul 12 1:25:21 2001 Delivered-To: freebsd-stable@freebsd.org Received: from bazooka.unixfreak.org (bazooka.unixfreak.org [63.198.170.138]) by hub.freebsd.org (Postfix) with ESMTP id 5E23F37B401 for ; Thu, 12 Jul 2001 01:25:18 -0700 (PDT) (envelope-from dima@unixfreak.org) Received: from hornet.unixfreak.org (hornet [63.198.170.140]) by bazooka.unixfreak.org (Postfix) with ESMTP id E7B7D3E2F; Thu, 12 Jul 2001 01:25:17 -0700 (PDT) To: Matt Dillon Cc: Mark Peek , stable@FreeBSD.ORG Subject: Re: cron exited on signal 11 In-Reply-To: <200107120809.f6C898Q42216@earth.backplane.com>; from dillon@earth.backplane.com on "Thu, 12 Jul 2001 01:09:08 -0700 (PDT)" Date: Thu, 12 Jul 2001 01:25:17 -0700 From: Dima Dorfman Message-Id: <20010712082517.E7B7D3E2F@bazooka.unixfreak.org> Sender: owner-freebsd-stable@FreeBSD.ORG Precedence: bulk List-ID: List-Archive: (Web Archive) List-Help: (List Instructions) List-Subscribe: List-Unsubscribe: X-Loop: FreeBSD.ORG Matt Dillon 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