From owner-svn-src-all@FreeBSD.ORG Mon Jul 1 13:53:51 2013 Return-Path: Delivered-To: svn-src-all@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by hub.freebsd.org (Postfix) with ESMTP id 51039C16 for ; Mon, 1 Jul 2013 13:53:51 +0000 (UTC) (envelope-from pfg@FreeBSD.org) Received: from nm41-vm8.bullet.mail.gq1.yahoo.com (nm41-vm8.bullet.mail.gq1.yahoo.com [67.195.87.95]) by mx1.freebsd.org (Postfix) with ESMTP id D9DE31CE8 for ; Mon, 1 Jul 2013 13:53:50 +0000 (UTC) Received: from [216.39.60.180] by nm41.bullet.mail.gq1.yahoo.com with NNFMP; 01 Jul 2013 13:53:49 -0000 Received: from [208.71.42.200] by tm16.bullet.mail.gq1.yahoo.com with NNFMP; 01 Jul 2013 13:53:49 -0000 Received: from [127.0.0.1] by smtp211.mail.gq1.yahoo.com with NNFMP; 01 Jul 2013 13:53:49 -0000 X-Yahoo-Newman-Id: 841028.71113.bm@smtp211.mail.gq1.yahoo.com X-Yahoo-Newman-Property: ymail-3 X-YMail-OSG: 8Ftprq8VM1nXeBZDiJxOvdUGSnVF1La.pubs0bs4G1M7Zmv cOQ4j0CimTUXaQOwHmpCqDtSLVvQYvhcV6dazp0u4.Hq7nh3EglRt.LuEk_e f_4XqFX8JT6bAw8t8mDmtOFJBH56TWp5apvMifm8UPWizsuEIpbizjsJkX.v 6JxrEom6lJ4EQP8AA5eXD4tqfXhJ3SZQETbbTr5X.F.2xRb711oNWsaXteK0 gP6pRfESOGyqATbw6hV3WqDfKoN25KmMnUTXkYPj.xzl0gzIvqRmtl6FSRvv 3.TACTLckAeDBGKyJlnYDJkD4NP9XimQwnLmTosyuuAVG8pEknH_KxHW4Es9 uY_Z5D97fkv56CxsYJ0GLpDhNPYfvr5NBbuy88EBTYQRZRsjwd9rRy9p1Iq_ tm2wC3oaN2NWVhoY6gzaziTsxxHOMTLrM33VxxlOFy0mbUo3.sVAjrXCHoQw QCzGwI41CmlT2HN7t7IzTVmOI78F4sPzRQEIvmvUnmQt7kxqNNWSeHbIvQAQ 109iqW2fUfcqJoks0Ec9A5Jt2FJZQA6aLIB5hkhS6xtTyRqaDWNMURfUOqK8 kgNU8h65iZMmD5oXHeZ_oVZi.vMcIJgdfP1XF_3I1xGtyPDxRYumUS25zH88 NMCKtS52fLFj0Xo7LquLt83SW9Myx4UTY X-Yahoo-SMTP: xcjD0guswBAZaPPIbxpWwLcp9Unf X-Rocket-Received: from [192.168.0.102] (pfg@190.157.126.109 with ) by smtp211.mail.gq1.yahoo.com with SMTP; 01 Jul 2013 06:53:49 -0700 PDT Message-ID: <51D189EB.1000208@FreeBSD.org> Date: Mon, 01 Jul 2013 08:53:47 -0500 From: Pedro Giffuni Organization: FreeBSD User-Agent: Mozilla/5.0 (Windows NT 6.2; WOW64; rv:17.0) Gecko/20130620 Thunderbird/17.0.7 MIME-Version: 1.0 To: d@delphij.net Subject: Re: svn commit: r252435 - in head/sys/ufs: ffs ufs References: <201307010300.r6130GWT035496@svn.freebsd.org> <51D12EF8.5000108@delphij.net> In-Reply-To: <51D12EF8.5000108@delphij.net> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org, Xin Li X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 01 Jul 2013 13:53:51 -0000 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.