Date: Mon, 01 Jul 2013 08:53:47 -0500 From: Pedro Giffuni <pfg@FreeBSD.org> To: d@delphij.net Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org, Xin Li <delphij@delphij.net> Subject: Re: svn commit: r252435 - in head/sys/ufs: ffs ufs Message-ID: <51D189EB.1000208@FreeBSD.org> In-Reply-To: <51D12EF8.5000108@delphij.net> References: <201307010300.r6130GWT035496@svn.freebsd.org> <51D12EF8.5000108@delphij.net>
next in thread | previous in thread | raw e-mail | index | archive | help
Hello; Thanks for reviewing ... El 01/07/2013 2:25 a. m., Xin Li escribió: > -----BEGIN PGP SIGNED MESSAGE----- > Hash: SHA256 > > On 6/30/13 8:00 PM, Pedro F. Giffuni wrote: >> Author: pfg Date: Mon Jul 1 03:00:15 2013 New Revision: 252435 >> URL: http://svnweb.freebsd.org/changeset/base/252435 >> >> Log: Change i_gen in UFS to an unsigned type. >> > [...] >> Modified: head/sys/ufs/ffs/ffs_vfsops.c >> ============================================================================== >> >> > - --- head/sys/ufs/ffs/ffs_vfsops.c Mon Jul 1 02:48:27 2013 (r252434) >> +++ head/sys/ufs/ffs/ffs_vfsops.c Mon Jul 1 03:00:15 2013 >> (r252435) @@ -1791,7 +1791,7 @@ ffs_vgetf(mp, ino, flags, vpp, >> ffs_flags * already have one. This should only happen on old >> filesystems. */ if (ip->i_gen == 0) { - ip->i_gen = arc4random() / >> 2 + 1; + ip->i_gen = arc4random() + 1; if ((vp->v_mount->mnt_flag >> & MNT_RDONLY) == 0) { ip->i_flag |= IN_MODIFIED; DIP_SET(ip, i_gen, >> ip->i_gen); >> > > Why ip->i_gen must be non-zero here? (I think it does not. Note that > arc4random() can return UINT32_MAX so the new code does not guarantee > this anyway, while old code does). > Zero is the expected value when the disk is very old and has no i_gen. In reality this is likely to be dead code, the real set up of i_gen is done in newfs and newfs_random() already produces and unsigned int. > If my understanding of the code is right, I think it doesn't really > hurt to have 0 in ip->i_gen in the places where arc4random() is used > (next time it would be regenerated), so probably we can just use > ip->i_gen = arc4random()? > That is pretty much what newfs_random() does. > However, if I was wrong, you probably want some construction like this: > > %%% for (;;) { > %%% ip->i_gen = arc4random(); > %%% if (ip->i_gen != UINT32_MAX) > %%% break; > %%% } > %%% ip->i_gen++; > Theorically having an open loop in a filesystem is a bad programming practice. I think I prefer the simple arc4random but still having a potential overflow here is not a problem. > Or we probably need to import a variant of arc4random_uniform into kernel? > What we have is sufficient and perhaps somewhat overdesigned, i_gen doesn't have to be too random at all. Cheers, Pedro.
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?51D189EB.1000208>