svn commit: r252435 - in head/sys/ufs: ffs ufs

[Pedro Giffuni]

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.