svn commit: r319868 - in head/sys: kern sys

[Konstantin Belousov]

On Mon, Jun 12, 2017 at 05:20:52PM -0700, Mark Johnston wrote:
> On Tue, Jun 13, 2017 at 12:33:00AM +0300, Konstantin Belousov wrote:
> > On Mon, Jun 12, 2017 at 01:47:05PM -0700, Mark Johnston wrote:
> > > On Mon, Jun 12, 2017 at 11:29:52PM +0300, Konstantin Belousov wrote:
> > > > On Mon, Jun 12, 2017 at 08:14:44PM +0000, Mark Johnston wrote:
> > > > > +int
> > > > > +uuidcmp(const struct uuid *uuid1, const struct uuid *uuid2)
> > > > > +{
> > > > > +
> > > > > +	return (memcmp(uuid1, uuid2, sizeof(struct uuid)));
> > > > > +}
> > > > 
> > > > This is unsafe.  The function operation depends on the ABI properties
> > > > that there is no padding either between members, or at the end of
> > > > the structure.  Why not use by-member comparision ?
> > > 
> > > I interpreted the CTASSERT at the beginning of kern_uuid.c as a
> > > guarantee that no such padding will be present. kern_uuid.c also defines
> > > an alternate representation, struct uuid_private, and casts between the
> > > two.
> > 
> > I agree, your addition is consistent with the other code in kern_uuid.c,
> > which already depends on these features.  At least it contradicts
> > to what compiler authors try to teach C language users.
> > 
> > BTW, does uuid_private low/mid/hi union split of ll depend on endianess ?
> 
> It seems so. The current usage is ok because the two representations
> aren't mixed. That is, ll is only used to access the time fields of the
> global uuid_last, and the low/mid/hi fields are never used with
> uuid_last. It does look like a bit of a landmine though; I think it
> would be worth adding a small hint:
> 
> diff --git a/sys/kern/kern_uuid.c b/sys/kern/kern_uuid.c
> index 0953d901a592..78088e1c6f32 100644
> --- a/sys/kern/kern_uuid.c
> +++ b/sys/kern/kern_uuid.c
> @@ -58,7 +58,7 @@ CTASSERT(sizeof(struct uuid) == 16);
>  /* We use an alternative, more convenient representation in the generator. */
>  struct uuid_private {
>  	union {
> -		uint64_t	ll;		/* internal. */
> +		uint64_t	ll;	/* internal, for uuid_last only */
>  		struct {
>  			uint32_t	low;
>  			uint16_t	mid;
Indeed helpful, IMO.