Date: Mon, 12 Jun 2017 17:20:52 -0700 From: Mark Johnston <markj@FreeBSD.org> To: Konstantin Belousov <kostikbel@gmail.com> Cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r319868 - in head/sys: kern sys Message-ID: <20170613002052.GC73695@wkstn-mjohnston.west.isilon.com> In-Reply-To: <20170612213300.GV2088@kib.kiev.ua> References: <201706122014.v5CKEitK071089@repo.freebsd.org> <20170612202952.GU2088@kib.kiev.ua> <20170612204704.GA73695@wkstn-mjohnston.west.isilon.com> <20170612213300.GV2088@kib.kiev.ua>
next in thread | previous in thread | raw e-mail | index | archive | help
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;
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20170613002052.GC73695>