Skip site navigation (1)Skip section navigation (2)
Date:      Sat, 22 Feb 2003 01:00:30 +0100
From:      Maxime Henrion <mux@freebsd.org>
To:        Nate Lawson <nate@root.org>
Cc:        src-committers@FreeBSD.org, cvs-src@FreeBSD.org, cvs-all@FreeBSD.org
Subject:   Re: cvs commit: src/sys/pci if_xl.c
Message-ID:  <20030222000030.GT60813@elvis.mu.org>
In-Reply-To: <20030221235701.GS60813@elvis.mu.org>
References:  <20030221222315.GR60813@elvis.mu.org> <Pine.BSF.4.21.0302211523570.54211-100000@root.org> <20030221235701.GS60813@elvis.mu.org>

next in thread | previous in thread | raw e-mail | index | archive | help
Maxime Henrion wrote:
> Nate Lawson wrote:
> > On Fri, 21 Feb 2003, Maxime Henrion wrote:
> > > Maxime Henrion wrote:
> > > These patches look mostly fine (I only looked at the busdma drivers).  I
> > > saw two minor nits though.  You're using "if (error != 0)", I personally
> > > prefer "if (error)" (I don't know if style(9) says something for this),
> > > but what's more important is that it makes style inconsistent in those
> > > drivers where the latter is used everywhere else.
> > 
> > I did it because of this example and the warning against "!error".
> >              error = function(a1, a2);
> >              if (error != 0)
> >                      exit(error);
> > 
> > But I don't care either way -- I try to stick to the original style when
> > possible (i.e. the xl changes do not remove the goto tree even though I
> > don't like that style).
> 
> Then let's follow the existing style in those drivers.
> 
> > > Also, there's an unreachable bus_teardown_intr() call in the xl(4) diff.
> > > It should be just removed since with your patches, the interrupt is the
> > > last thing allocated in the xl_init() function and thus we never need to
> > > deallocate it there.
> > 
> > I actually did that on purpose to handle future reordering if it
> > happens.  I think it's an advantage to have the resource cleanup be
> > (mostly) independent of the order it was allocated in.  In fact, with
> > a little more work, the attach error case and the detach case could be
> > unified.
> 
> Fair enough.  It would be nice to add a comment explaining this then so
> that people don't wonder about it.  Also, telling it to lint with a
> /* UNREACH */ or something might make sense.

s/UNREACH/NOTREACHED/

Maxime

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




Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20030222000030.GT60813>