From owner-cvs-all  Sat Jan  6 16:54:31 2001
From owner-cvs-all@FreeBSD.ORG  Sat Jan  6 16:54:24 2001
Return-Path: <owner-cvs-all@FreeBSD.ORG>
Delivered-To: cvs-all@freebsd.org
Received: from fw.wintelcom.net (ns1.wintelcom.net [209.1.153.20])
	by hub.freebsd.org (Postfix) with ESMTP
	id 9A4B937B400; Sat,  6 Jan 2001 16:54:24 -0800 (PST)
Received: (from bright@localhost)
	by fw.wintelcom.net (8.10.0/8.10.0) id f070sNV25982;
	Sat, 6 Jan 2001 16:54:23 -0800 (PST)
Date: Sat, 6 Jan 2001 16:54:23 -0800
From: Alfred Perlstein <bright@wintelcom.net>
To: Bosko Milekic <bmilekic@technokratis.com>
Cc: cvs-committers@FreeBSD.org, cvs-all@FreeBSD.org
Subject: Re: cvs commit: src/sys/dev/musycc musycc.c
Message-ID: <20010106165423.I15744@fw.wintelcom.net>
References: <200101062044.f06Kiex42615@freefall.freebsd.org> <20010106132943.E15744@fw.wintelcom.net> <005001c07839$40f06820$25cbca18@jehovah> <20010106161427.G15744@fw.wintelcom.net> <003c01c07842$cc648bd0$25cbca18@jehovah>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
User-Agent: Mutt/1.2.5i
In-Reply-To: <003c01c07842$cc648bd0$25cbca18@jehovah>; from bmilekic@technokratis.com on Sat, Jan 06, 2001 at 07:43:05PM -0500
Sender: bright@fw.wintelcom.net
Sender: owner-cvs-all@FreeBSD.ORG
Precedence: bulk
X-Loop: FreeBSD.ORG

* Bosko Milekic <bmilekic@technokratis.com> [010106 16:41] wrote:
> 
> > * Bosko Milekic <bmilekic@technokratis.com> [010106 15:33] wrote:
> > >
> > > I find this incredibly dumb. You're just wasting a check that need not be
> > > made. I know when I have to free m and when not to, it's dumb to have to
> > > check it again in errfree. It's Poul's call, however.
> >
> > You have an exceptional condition, it reduces the amount of code
> > someone reading must digest and it should reduce the amount of code
> > the processor needs to put in the I cache.
> 
>     The problem with it is that not all error conditions should lead to this
> exit path. For example, if you fail before the allocation of the mbuf loop,
> this is certainly not how you want to error. Also, if you fail before you
> MALLOC those buffers, you certainly don't want to try FREE()ing them. I
> suppose you could turn every single error condition in that function to jump
> to some label at the bottom of the function. If anything, I find _that_ to be
> more obfuscating the code than status quo.

That's a bit much, but it's a lot better to keep the normal codepath
clear and understandable than to have all the error handling inlined.

This is really a matter of policy and how you think, obviously we 
disagree, however I'm not saying it _has_ to be this way, just that
I prefer it to be that way. :)

I just would like you to consider my position on the subject.

> > There is also a lot of code that already does this:
> >
> > sosend(), soreceive(), soclose().
> 
>     Huh? What does sosend, soreceive, soclose have anything to do with this?
> Or am I misunderstanding?

At a certain point more code may be added that must do this cleanup,
it may happen after the mbuf it allocated.

All these functions have goto lables for exceptional conditions:

---
sosend:

release:
        sbunlock(&so->so_snd);
out:
        if (top)
                m_freem(top);
        if (control)
                m_freem(control);
        return (error);
}

---
soreceive:

bad:
                if (m)
                        m_freem(m);
                return (error);

---
soclose:

drop:
        if (so->so_pcb) {
                int error2 = (*so->so_proto->pr_usrreqs->pru_detach)(so);
                if (error == 0)
                        error = error2;
        }
discard:
        if (so->so_state & SS_NOFDREF)
                panic("soclose: NOFDREF");
        so->so_state |= SS_NOFDREF;
        sofree(so);
        splx(s);
        return (error);
}

> > What you're doing is giving a place to jump to further along in
> > case any other error conditions come up.
> 
>     What you are doing by moving the m_free down to the bottom is actually
> growing the number of instructions that are cached. In the first case, where
> you m_free at the top before the jump, you simply m_free in that case. In
> your suggestion, you have the added instructions that account for checking
> whether m is NULL, something you already know before jumping.

Yes, but since it's "shouldn't happen" it's better to put the test
at the end rather than inline it in the code.

-- 
-Alfred Perlstein - [bright@wintelcom.net|alfred@freebsd.org]
"I have the heart of a child; I keep it in a jar on my desk."


To Unsubscribe: send mail to majordomo@FreeBSD.org
with "unsubscribe cvs-all" in the body of the message