From mboxrd@z Thu Jan 1 00:00:00 1970 From: mjn3@codepoet.org (Manuel Novoa III) Subject: Re: fs/minix/bitmap.c is broken in CVS Date: Thu, 20 Jun 2002 22:23:37 -0600 Sender: linux-8086-owner@vger.kernel.org Message-ID: <20020621042337.GA23253@codepoet.org> References: <20020618122458.GA12082@codepoet.org> Mime-Version: 1.0 Return-path: Content-Disposition: inline In-Reply-To: List-Id: Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: Riley Williams Cc: linux-8086@vger.kernel.org Hello Riley, On Thu, Jun 20, 2002 at 06:45:03PM +0100, Riley Williams wrote: > Hi Manuel. > > > Apparently during the recent changes, someone replaced the code > > sequence > > > > if (j == find_first_zero_bit(bh->b_data, 8192)) > > panic("still zero bit!%d\n", j); > > > > with > > > > j == (block_t) find_first_zero_bit(bh->b_data, 8192); > > if (j) > > panic("still zero bit!%d\n", j); > > > > in function minix_new_block in file fs/minix/bitmap.c. > > If they did, it wouldn't compile. Sure it would compile. A better compiler than bcc would probably issue a warning because the result of the == operator would be discarded, but it is still valid C. > However, there is only one = sign > (it's the assignment operator, not the equality comparison operator > that occurs there). Riley, look at the code. It is _supposed_ to be == since the intent is to check and make sure the bit you want set by set_bit has actually been set. I found this because trying to write to a new file was causing a kernel panic. > I also replaced similar sequences elsewhere in the kernel source. Unfortunatly, doing so can be an anti-optimization wrt size when using bcc. My size reduction patch probably reversed a number of your changes. > I've been running the ELKS kernel source through the splint source > checking tool, and cleaning the source up so that splint doesn't report > any warnings whatsoever in its -weak analysis mode. Here's the version > of those two lines that reports clean as far as splint is concerned... > > > if ((j = ((block_t) find_first_zero_bit(bh->b_data, 8192)))) > > panic("still zero bit!%d\n", j); If you check the older versions in cvs, you'll see that it was == for comparison, not = for assignment. > I don't know about you, but there's far too many brackets in there for > my liking, which is why I split it into two lines. It doesn't affect the > code produced. The parenthesis don't bother me as long as the code is formatted reasonably. And there are situations were it _does_ affect the code produced. If you'd like, I'd be happy to provide you with some examples. That would have to wait until Monday or so as I am currently on the road. > Best wishes from Riley. And to you, Manuel