diff mbox

[10/23] afs: switch to use uuid_t and uuid_gen

Message ID 20170518062705.25902-11-hch@lst.de (mailing list archive)
State Not Applicable
Headers show

Commit Message

Christoph Hellwig May 18, 2017, 6:26 a.m. UTC 
Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: David Howells <dhowells@redhat.com>
---
 fs/afs/cmservice.c | 46 +++++++++++++++++++++++-----------------------
 fs/afs/internal.h  |  2 +-
 fs/afs/main.c      |  4 ++--
 3 files changed, 26 insertions(+), 26 deletions(-)

Comments

Andy Shevchenko May 22, 2017, 6:49 p.m. UTC | #1 
On Thu, 2017-05-18 at 08:26 +0200, Christoph Hellwig wrote:

Changelog?

> Signed-off-by: Christoph Hellwig <hch@lst.de>
> Reviewed-by: David Howells <dhowells@redhat.com>

> @@ -453,7 +453,7 @@ static int afs_deliver_cb_probe(struct afs_call
> *call)
>  static void SRXAFSCB_ProbeUuid(struct work_struct *work)
>  {
>  	struct afs_call *call = container_of(work, struct afs_call,
> work);
> -	struct uuid_v1 *r = call->request;
> +	uuid_t *r = call->request;
>  
>  	struct {
>  		__be32	match;
> 

Just to double check that this doesn't create a union aliasing.
Christoph Hellwig May 23, 2017, 8:49 a.m. UTC | #2 
On Mon, May 22, 2017 at 09:49:17PM +0300, Andy Shevchenko wrote:
> >  	struct afs_call *call = container_of(work, struct afs_call,
> > work);
> > -	struct uuid_v1 *r = call->request;
> > +	uuid_t *r = call->request;
> >  
> >  	struct {
> >  		__be32	match;
> > 
> 
> Just to double check that this doesn't create a union aliasing.

What do you mean with that?
Andy Shevchenko May 23, 2017, 1:11 p.m. UTC | #3 
On Tue, 2017-05-23 at 10:49 +0200, Christoph Hellwig wrote:
> On Mon, May 22, 2017 at 09:49:17PM +0300, Andy Shevchenko wrote:
> > >  	struct afs_call *call = container_of(work, struct
> > > afs_call,
> > > work);
> > > -	struct uuid_v1 *r = call->request;
> > > +	uuid_t *r = call->request;
> > >  
> > >  	struct {
> > >  		__be32	match;
> > > 
> > 
> > Just to double check that this doesn't create a union aliasing.
> 
> What do you mean with that?

Since we introduced a union it's possible that we might access the
member which wasn't last modified one. So, my comment is to give an
attention on such possibility and avoid if there is an aliasing
happened.
Christoph Hellwig May 25, 2017, 1 p.m. UTC | #4 
On Tue, May 23, 2017 at 04:11:39PM +0300, Andy Shevchenko wrote:
> Since we introduced a union it's possible that we might access the
> member which wasn't last modified one. So, my comment is to give an
> attention on such possibility and avoid if there is an aliasing
> happened.

We do for AFS (and XFS for fs fsid).  My preference would be to
not have the v1 struct defintion but instead provide a few
helpers in uuid.h that use get_unaligned_be* if needed:

	uuid_v1_time_low()
	uuid_v1_time_mid()
	uuid_v1_time_time_hi_and_version()..

From his previously reply it seems like Dave doesn't like that idea
too much, in which case I suspect moving struct uuid_v1 back into
afs and living with cast in it is the way to go.
Andy Shevchenko May 25, 2017, 1:29 p.m. UTC | #5 
On Thu, 2017-05-25 at 15:00 +0200, Christoph Hellwig wrote:
> On Tue, May 23, 2017 at 04:11:39PM +0300, Andy Shevchenko wrote:
> > Since we introduced a union it's possible that we might access the
> > member which wasn't last modified one. So, my comment is to give an
> > attention on such possibility and avoid if there is an aliasing
> > happened.
> 
> We do for AFS (and XFS for fs fsid).  My preference would be to
> not have the v1 struct defintion but instead provide a few
> helpers in uuid.h that use get_unaligned_be* if needed:
> 
> 	uuid_v1_time_low()
> 	uuid_v1_time_mid()
> 	uuid_v1_time_time_hi_and_version()..
> 
> From his previously reply it seems like Dave doesn't like that idea
> too much, in which case I suspect moving struct uuid_v1 back into
> afs and living with cast in it is the way to go.

Personally I don't like that union stuff, so, definitely my vote to get
rid of AFS stuff in generic helpers.

OTOH if there will be more users of such API then something like you
proposed would be sufficient without introducing a union.
diff mbox

Patch

diff --git a/fs/afs/cmservice.c b/fs/afs/cmservice.c
index 3062cceb5c2a..d4e77d12570c 100644
--- a/fs/afs/cmservice.c
+++ b/fs/afs/cmservice.c
@@ -350,7 +350,7 @@  static int afs_deliver_cb_init_call_back_state3(struct afs_call *call)
 {
 	struct sockaddr_rxrpc srx;
 	struct afs_server *server;
-	struct uuid_v1 *r;
+	uuid_t *r;
 	unsigned loop;
 	__be32 *b;
 	int ret;
@@ -380,20 +380,20 @@  static int afs_deliver_cb_init_call_back_state3(struct afs_call *call)
 		}
 
 		_debug("unmarshall UUID");
-		call->request = kmalloc(sizeof(struct uuid_v1), GFP_KERNEL);
+		call->request = kmalloc(sizeof(uuid_t), GFP_KERNEL);
 		if (!call->request)
 			return -ENOMEM;
 
 		b = call->buffer;
 		r = call->request;
-		r->time_low			= b[0];
-		r->time_mid			= htons(ntohl(b[1]));
-		r->time_hi_and_version		= htons(ntohl(b[2]));
-		r->clock_seq_hi_and_reserved 	= ntohl(b[3]);
-		r->clock_seq_low		= ntohl(b[4]);
+		r->v1.time_low			= b[0];
+		r->v1.time_mid			= htons(ntohl(b[1]));
+		r->v1.time_hi_and_version	= htons(ntohl(b[2]));
+		r->v1.clock_seq_hi_and_reserved = ntohl(b[3]);
+		r->v1.clock_seq_low		= ntohl(b[4]);
 
 		for (loop = 0; loop < 6; loop++)
-			r->node[loop] = ntohl(b[loop + 5]);
+			r->v1.node[loop] = ntohl(b[loop + 5]);
 
 		call->offset = 0;
 		call->unmarshall++;
@@ -453,7 +453,7 @@  static int afs_deliver_cb_probe(struct afs_call *call)
 static void SRXAFSCB_ProbeUuid(struct work_struct *work)
 {
 	struct afs_call *call = container_of(work, struct afs_call, work);
-	struct uuid_v1 *r = call->request;
+	uuid_t *r = call->request;
 
 	struct {
 		__be32	match;
@@ -476,7 +476,7 @@  static void SRXAFSCB_ProbeUuid(struct work_struct *work)
  */
 static int afs_deliver_cb_probe_uuid(struct afs_call *call)
 {
-	struct uuid_v1 *r;
+	uuid_t *r;
 	unsigned loop;
 	__be32 *b;
 	int ret;
@@ -502,20 +502,20 @@  static int afs_deliver_cb_probe_uuid(struct afs_call *call)
 		}
 
 		_debug("unmarshall UUID");
-		call->request = kmalloc(sizeof(struct uuid_v1), GFP_KERNEL);
+		call->request = kmalloc(sizeof(uuid_t), GFP_KERNEL);
 		if (!call->request)
 			return -ENOMEM;
 
 		b = call->buffer;
 		r = call->request;
-		r->time_low			= b[0];
-		r->time_mid			= htons(ntohl(b[1]));
-		r->time_hi_and_version		= htons(ntohl(b[2]));
-		r->clock_seq_hi_and_reserved 	= ntohl(b[3]);
-		r->clock_seq_low		= ntohl(b[4]);
+		r->v1.time_low			= b[0];
+		r->v1.time_mid			= htons(ntohl(b[1]));
+		r->v1.time_hi_and_version	= htons(ntohl(b[2]));
+		r->v1.clock_seq_hi_and_reserved = ntohl(b[3]);
+		r->v1.clock_seq_low		= ntohl(b[4]);
 
 		for (loop = 0; loop < 6; loop++)
-			r->node[loop] = ntohl(b[loop + 5]);
+			r->v1.node[loop] = ntohl(b[loop + 5]);
 
 		call->offset = 0;
 		call->unmarshall++;
@@ -568,13 +568,13 @@  static void SRXAFSCB_TellMeAboutYourself(struct work_struct *work)
 	memset(&reply, 0, sizeof(reply));
 	reply.ia.nifs = htonl(nifs);
 
-	reply.ia.uuid[0] = afs_uuid.time_low;
-	reply.ia.uuid[1] = htonl(ntohs(afs_uuid.time_mid));
-	reply.ia.uuid[2] = htonl(ntohs(afs_uuid.time_hi_and_version));
-	reply.ia.uuid[3] = htonl((s8) afs_uuid.clock_seq_hi_and_reserved);
-	reply.ia.uuid[4] = htonl((s8) afs_uuid.clock_seq_low);
+	reply.ia.uuid[0] = afs_uuid.v1.time_low;
+	reply.ia.uuid[1] = htonl(ntohs(afs_uuid.v1.time_mid));
+	reply.ia.uuid[2] = htonl(ntohs(afs_uuid.v1.time_hi_and_version));
+	reply.ia.uuid[3] = htonl((s8) afs_uuid.v1.clock_seq_hi_and_reserved);
+	reply.ia.uuid[4] = htonl((s8) afs_uuid.v1.clock_seq_low);
 	for (loop = 0; loop < 6; loop++)
-		reply.ia.uuid[loop + 5] = htonl((s8) afs_uuid.node[loop]);
+		reply.ia.uuid[loop + 5] = htonl((s8) afs_uuid.v1.node[loop]);
 
 	if (ifs) {
 		for (loop = 0; loop < nifs; loop++) {
diff --git a/fs/afs/internal.h b/fs/afs/internal.h
index 393672997cc2..7de45c8686a2 100644
--- a/fs/afs/internal.h
+++ b/fs/afs/internal.h
@@ -544,7 +544,7 @@  extern int afs_drop_inode(struct inode *);
  * main.c
  */
 extern struct workqueue_struct *afs_wq;
-extern struct uuid_v1 afs_uuid;
+extern uuid_t afs_uuid;
 
 /*
  * misc.c
diff --git a/fs/afs/main.c b/fs/afs/main.c
index 51d7d17bca57..75b3d3a8b1ba 100644
--- a/fs/afs/main.c
+++ b/fs/afs/main.c
@@ -31,7 +31,7 @@  static char *rootcell;
 module_param(rootcell, charp, 0);
 MODULE_PARM_DESC(rootcell, "root AFS cell name and VL server IP addr list");
 
-struct uuid_v1 afs_uuid;
+uuid_t afs_uuid;
 struct workqueue_struct *afs_wq;
 
 /*
@@ -43,7 +43,7 @@  static int __init afs_init(void)
 
 	printk(KERN_INFO "kAFS: Red Hat AFS client v0.1 registering.\n");
 
-	generate_random_uuid((unsigned char *)&afs_uuid);
+	uuid_gen(&afs_uuid);
 
 	/* create workqueue */
 	ret = -ENOMEM;