[Cluster-devel] Re: master - libgfs2: Add support for UUID generation to gfs2_mkfs

[Steven Whitehouse]

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 <swhiteho at redhat.com>
> >>> AuthorDate:    Fri Oct 10 16:04:29 2008 +0100
> >>> Committer:     Steven Whitehouse <swhiteho at redhat.com>
> >>> 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.