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

[Fabio M. Di Nitto]

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.

>
>>> 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.

> 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.

> 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.