From mboxrd@z Thu Jan 1 00:00:00 1970 From: Steven Whitehouse Date: Mon, 13 Oct 2008 14:32:45 +0100 Subject: [Cluster-devel] Re: master - libgfs2: Add support for UUID generation to gfs2_mkfs In-Reply-To: References: <20081010160641.7A962C046B@lists.fedorahosted.org> <1223885408.25004.33.camel@quoit> Message-ID: <1223904765.25004.47.camel@quoit> List-Id: To: cluster-devel.redhat.com MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Hi, On Mon, 2008-10-13 at 10:37 +0200, Fabio M. Di Nitto wrote: > On Mon, 13 Oct 2008, Steven Whitehouse wrote: > > > Hi, > > > > On Fri, 2008-10-10 at 19:31 +0200, Fabio M. Di Nitto wrote: > >> On Fri, 10 Oct 2008, Steven Whitehouse wrote: > >> > >>> Gitweb: http://git.fedorahosted.org/git/cluster.git?p=cluster.git;a=commitdiff;h=85049a0824daa9abaa38f5dca377767907b53b39 > >>> Commit: 85049a0824daa9abaa38f5dca377767907b53b39 > >>> Parent: d763f902abf33655e635fc3b8b1919fd31d4f66c > >>> Author: Steven Whitehouse > >>> AuthorDate: Fri Oct 10 16:04:29 2008 +0100 > >>> Committer: Steven Whitehouse > >>> CommitterDate: Fri Oct 10 16:12:12 2008 +0100 > >>> > >>> libgfs2: Add support for UUID generation to gfs2_mkfs > >> > >> nice... > >> > >>> Uses /dev/urandom to create 16 byte UUIDs for GFS2 filesystems > >>> at mkfs time. Backwards and forwards compatible with all > >>> GFS2 filesystems. > >> > >>> You'll need a set of kernel headers with > >>> the new field defined in order for this feature to be > >>> enabled. Bugzilla #242690 > >> > >> What kernel version is required to have UUID support? is it in .27 or will > >> be in .28? > >> > > It will be .28, but this is a build dep only. It doesn't make any > > difference what kernel you run once mkfs has been compiled. > > Absolutely fine. I need to make sure to test when we switch to .28 > headers. > Ok, sounds good. > > > >>> diff --git a/gfs2/libgfs2/structures.c b/gfs2/libgfs2/structures.c > >>> index eb4c7bd..002edb6 100644 > >>> --- a/gfs2/libgfs2/structures.c > >>> +++ b/gfs2/libgfs2/structures.c > >>> @@ -58,7 +58,17 @@ build_sb(struct gfs2_sbd *sdp) > >>> sb.sb_root_dir = sdp->md.rooti->i_di.di_num; > >>> strcpy(sb.sb_lockproto, sdp->lockproto); > >>> strcpy(sb.sb_locktable, sdp->locktable); > >>> - > >>> +#ifdef GFS2_HAS_UUID > >>> + { > >>> + int fd = open("/dev/urandom", O_RDONLY); > >>> + int n; > >>> + if (fd >= 0) > >>> + n = read(fd, &sb.sb_uuid, 16); > >>> + if (fd < 0 || n != 16) > >>> + memset(&sb.sb_uuid, 0, 16); > >>> + close(fd); > >>> + } > >>> +#endif > >> > >> NACK. > >> > >> Please use libuuid for this operation. > >> > > > Hmm, well there are some problems with that... it will introduce another > > build dep, and one which we can't #ifdef around since it will be in the > > rpm rather than the source. > > This is not an issue. > Well I'm very cautious about adding any deps. If we have to do it, then it seems ok, but I'd rather avoid it if possible. > > Also libuuid seems somewhat over engineered. > > Right, it is, but there are situations where /dev/urandom might not have > the entropy required to spit out one single bit. I have seen it happening > a few times, specially on remote servers where there is no mouse or > keyboard to generate randomness and took over a few hours to generate an > ssh key. > /dev/urandom should not block, neither should ssh be using it, it ought to be using /dev/random instead (which might block for the reason you mention). Bearing in mind that creating a filesystem requires disk I/O, that should help to refill the entropy pool anyway. > > I guess we should set the format correctly though. Something like: > > > > sb.sb_uuid[7] &= ~0xf0; /* time_hi_and_version */ > > sb.sb_uuid[7] |= 0x40; > > sb.sb_uuid[8] &= ~0xc0; /* clock_seq_hi_and_reserved */ > > sb.sb_uuid[8] |= 0x40; > > > > after the memset would do the trick. There doesn't seem much point using > > any of the other UUID formats since they seem to result less entropy > > than the random method and we have a reasonable random number generator > > available (which is not a PRNG despite the implication in the libuuid > > source). > > > > Even with those 6 bits made constant we still have 122 bits left so the > > chances of clashing UUIDs is 1 in 2^122 assuming that we can rely > > on /dev/urandom so I don't think we are likely to have any problems > > here. > > I would still prefer to avoid implementating our own version of a UUID > generator and rely on what's already done. > > Fabio > > -- > I'm going to make him an offer he can't refuse. Hmmm, well I've put my thinking cap on over this one.... I'm not convinced yet, but watch this space.... Steve.