Re: netbsd-7 kernel and older vnconfig(8)

[Robert Elz <kre%munnari.OZ.AU@localhost>]

    Date:        Wed, 11 Nov 2015 21:06:56 -0500
    From:        christos%zoulas.com@localhost (Christos Zoulas)
    Message-ID:  <20151112020656.D581B17FDAB%rebar.astron.com@localhost>

  | The memset was wrong.

Sorry, I should have noticed that one...

I just did some more reading of the driver, and noticed one more weird
thing (unrelated to vnconfig -l) ...

The current code allows (and has since 1997, so fixing this is perhaps
not urgent!) the blocksize (sector size) of the emulated disc to be set
to anything that is an integer multiple of DEV_BSIZE (perhaps to 1536 for
example).

Back when this was done, it probably didn't matter, as the kernel just
worked exclusively with DEV_BSIZE, and vnd's thinking that they had
weird block sizes was probably invisible everywhere else.

But now I'm not sure that's true any more - I'm sure I've seen (in code
related to wedges perhaps) code that knows it can convert between DEV_BSIZE
units and sector size units by shifting - that is, that sector sizes are
always powers of two.

If you want to change this, a patch is appended.    I know there's a KNF
violation (xxx-1 without spaces) in the modified code (one of the two
updates) but the alternative was a line wrap, and personally that's a
trade off I'd accept, but ... (it also makes op prec more obvious without
requiring yet another set of ()'s as is done in the second one where line
wrap was unavoidable.)  [Aside: the existing !=0 KNF botch that was there is
fixed however!]   An alternative would be to just omit the != 0 part...

Also: I didn't bother to fix it, in case there is some hidden purpose, but
the test (visible below at the end of the patch)

 			    (vnd->sc_geom.vng_ntracks *
 			     vnd->sc_geom.vng_nsectors) == 0) {

seems to be a weirdly expensive way to test

		vnd->sc_geom.vng_ntracks == 0 || vnd->sc_geom.vng_nsectors == 0

If it is also intended to be checking for overflow of the product, just
happening to accidentally produce 0 as the result, then OK I suppose,
but I don't think I'd do it quite like that (it would be more useful to
verify that overflow doesn't happen.)

kre


--- vnd.c	2015-11-12 17:23:19.000000000 +0700
+++ vnd.c.secsize_fix	2015-11-12 17:40:11.000000000 +0700
@@ -1262,8 +1262,8 @@
 			/* note last offset is the file byte size */
 			vnd->sc_comp_numoffs = ntohl(ch->num_blocks)+1;
 			free(ch, M_TEMP);
-			if (vnd->sc_comp_blksz == 0 ||
-			    vnd->sc_comp_blksz % DEV_BSIZE !=0) {
+			if (vnd->sc_comp_blksz < DEV_BSIZE ||
+			    (vnd->sc_comp_blksz & vnd->sc_comp_blksz-1) != 0) {
 				VOP_UNLOCK(nd.ni_vp);
 				error = EINVAL;
 				goto close_and_exit;
@@ -1362,7 +1362,8 @@
 			 * XXX we?
 			 */
 			if (vnd->sc_geom.vng_secsize < DEV_BSIZE ||
-			    (vnd->sc_geom.vng_secsize % DEV_BSIZE) != 0 ||
+			    (vnd->sc_geom.vng_secsize &
+			     (vnd->sc_geom.vng_secsize - 1)) != 0 ||
 			    vnd->sc_geom.vng_ncylinders == 0 ||
 			    (vnd->sc_geom.vng_ntracks *
 			     vnd->sc_geom.vng_nsectors) == 0) {