svn commit: r295362 - head/sys/fs/cd9660

[Bruce Evans]

On Sun, 7 Feb 2016, Pedro F. Giffuni wrote:

> Log:
>  cd9660: Drop an unnecessary check for NULL.
>
>  This was unnecessary and also confused Coverity.
>
>  Confirmed on:	NetBSD
>  CID:		978558

This leaves many similar bugs unfixed nearby.  One is a null pointer
panic, not just an unnecessary check.

> Modified: head/sys/fs/cd9660/cd9660_vfsops.c
> ==============================================================================
> --- head/sys/fs/cd9660/cd9660_vfsops.c	Sun Feb  7 01:45:24 2016	(r295361)
> +++ head/sys/fs/cd9660/cd9660_vfsops.c	Sun Feb  7 03:48:40 2016	(r295362)
> @@ -741,8 +741,7 @@ cd9660_vget_internal(mp, ino, flags, vpp
> 		if (off + isonum_711(isodir->length) >
> 		    imp->logical_block_size) {
> 			vput(vp);
> -			if (bp != 0)
> -				brelse(bp);
> +			brelse(bp);
> 			printf("fhtovp: directory crosses block boundary %d[off=%d/len=%d]\n",
> 			       off +isonum_711(isodir->length), off,
> 			       isonum_711(isodir->length));
>

- 11 lines later, there is the same bug in an #if 0 block.
- 9 lines earlier, bp was brelse()ed without a similar check.  bp is always
   null at that point, so checking for this would be just a style bug.  Not
   checking, but doing the wrong cleanup of calling brelse() after bread()
   fails, gives a null pointer panic in brelse() whenever bread() fails.

That seems to be all the similar bugs.  bp is correctly abused as a flag
later.  Other functions depend more critically on bread() setting bp to
NULL when it fails.  Typical code is to "goto out" when bread() fails,
and brelse() bp there if it is not null.

Not-so-similar bugs include spelling NULL as 0 or (empty).

bread(9) is undocumented.  Someone broke its KPI by changing it to a
macro.  This annoys me when I try to set a breakpoint at it in kernels
with the broken KPI.  The unimportant badly designed bread(3) is documented.

Bruce