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

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

[Fabio M. Di Nitto]

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

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

Thanks
Fabio

--
I'm going to make him an offer he can't refuse.

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

[Steven Whitehouse]

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

> > 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.
> 
> Thanks
> Fabio
> 
> --
> I'm going to make him an offer he can't refuse.
> 
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. Also libuuid seems somewhat over engineered.
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.

Steve.

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

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

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

[Fabio M. Di Nitto]

On Mon, 2008-10-13 at 14:32 +0100, Steven Whitehouse wrote:
> Hi,
> 

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

It is a fair concern.. but we are not trying to pull in libmono or .NET.
Our build system already requires python and perl... this is a small
one. Go for it.

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

ahahahah

Fabio