Skip to contentSkip to conversation

afs: fsclient.c sparse endian annotations of operation_ID

PATCH In review Harvey Harrison 4 participants 4 messages Aug 26, 2008 – Aug 28, 2008 Afs filesystem

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 afs: fsclient.c sparse endian annotations of operation_ID

fs/afs/fsclient.c:319:11: warning: restricted __be32 degrades to integer fs/afs/fsclient.c:465:21: warning: incorrect type in assignment (different base types)
fs/afs/fsclient.c:465:21: expected restricted __be32 [usertype] operation_ID
fs/afs/fsclient.c:465:21: got int
fs/afs/fsclient.c:510:21: warning: incorrect type in assignment (different base types)
fs/afs/fsclient.c:510:21: expected restricted __be32 [usertype] operation_ID
fs/afs/fsclient.c:510:21: got int
fs/afs/fsclient.c:1304:10: warning: restricted __be32 degrades to integer fs/afs/fsclient.c:1366:21: warning: incorrect type in assignment (different base types)
fs/afs/fsclient.c:1366:21: expected restricted __be32 [usertype] operation_ID
fs/afs/fsclient.c:1366:21: got int
fs/afs/fsclient.c:1417:21: warning: incorrect type in assignment (different base types)
fs/afs/fsclient.c:1417:21: expected restricted __be32 [usertype] operation_ID
fs/afs/fsclient.c:1417:21: got int
fs/afs/fsclient.c:1463:21: warning: incorrect type in assignment (different base types)
fs/afs/fsclient.c:1463:21: expected restricted __be32 [usertype] operation_ID
fs/afs/fsclient.c:1463:21: got int

---
 fs/afs/fsclient.c |   14 +++++++-------
 1 files changed, 7 insertions(+), 7 deletions(-)
fs/afs/fsclient.c +7 -7
index 023b95b..2928067 100644
@@ -316,7 +316,7 @@ static int afs_deliver_fs_fetch_data(struct afs_call *call,
 	case 0:
 		call->offset = 0;
 		call->unmarshall++;
-		if (call->operation_ID != FSFETCHDATA64) {
+		if (call->operation_ID != __constant_htonl(FSFETCHDATA64)) {
 			call->unmarshall++;
 			goto no_msw;
 		}
@@ -462,7 +462,7 @@ static int afs_fs_fetch_data64(struct afs_server *server,
 	call->reply3 = buffer;
 	call->service_id = FS_SERVICE;
 	call->port = htons(AFS_FS_PORT);
-	call->operation_ID = FSFETCHDATA64;
+	call->operation_ID = __constant_htonl(FSFETCHDATA64);
 
 	/* marshall the parameters */
 	bp = call->request;
@@ -507,7 +507,7 @@ int afs_fs_fetch_data(struct afs_server *server,
 	call->reply3 = buffer;
 	call->service_id = FS_SERVICE;
 	call->port = htons(AFS_FS_PORT);
-	call->operation_ID = FSFETCHDATA;
+	call->operation_ID = __constant_htonl(FSFETCHDATA);
 
 	/* marshall the parameters */
 	bp = call->request;
@@ -1301,7 +1301,7 @@ static int afs_deliver_fs_store_status(struct afs_call *call,
 
 	/* unmarshall the reply once we've received all of it */
 	store_version = NULL;
-	if (call->operation_ID == FSSTOREDATA)
+	if (call->operation_ID == __constant_htonl(FSSTOREDATA))
 		store_version = &call->store_version;
 
 	bp = call->buffer;
@@ -1363,7 +1363,7 @@ static int afs_fs_setattr_size64(struct afs_server *server, struct key *key,
 	call->service_id = FS_SERVICE;
 	call->port = htons(AFS_FS_PORT);
 	call->store_version = vnode->status.data_version + 1;
-	call->operation_ID = FSSTOREDATA;
+	call->operation_ID = __constant_htonl(FSSTOREDATA);
 
 	/* marshall the parameters */
 	bp = call->request;
@@ -1414,7 +1414,7 @@ static int afs_fs_setattr_size(struct afs_server *server, struct key *key,
 	call->service_id = FS_SERVICE;
 	call->port = htons(AFS_FS_PORT);
 	call->store_version = vnode->status.data_version + 1;
-	call->operation_ID = FSSTOREDATA;
+	call->operation_ID = __constant_htonl(FSSTOREDATA);
 
 	/* marshall the parameters */
 	bp = call->request;
@@ -1460,7 +1460,7 @@ int afs_fs_setattr(struct afs_server *server, struct key *key,
 	call->reply = vnode;
 	call->service_id = FS_SERVICE;
 	call->port = htons(AFS_FS_PORT);
-	call->operation_ID = FSSTORESTATUS;
+	call->operation_ID = __constant_htonl(FSSTORESTATUS);
 
 	/* marshall the parameters */
 	bp = call->request;
Signature · 1 line
1.6.0.340.g84854
maintainer
#1 lore raw

fs/afs/fsclient.c static int afs_deliver_fs_fetch_data(struct afs_call *call, · line 319

Quoted 1 line · Harvey Harrison
fs/afs/fsclient.c +1 -0
+		if (call->operation_ID != __constant_htonl(FSFETCHDATA64)) {

Doesn't htonl() resolve to this for a constant argument? Following through the definitions, it certainly looks like it ought to:

	<linux/byteorder/generic.h>
	#undef htonl
	#define ___htonl(x) __cpu_to_be32(x)

	<linux/byteorder/little_endian.h>
	#define __cpu_to_be32(x) ((__force __be32)__swab32((x)))

	<linux/byteorder/swab.h>
	#  define __swab32(x) \
	(__builtin_constant_p((__u32)(x)) ? \
	 ___constant_swab32((x)) : \
	 __fswab32((x)))

at least for GCC with optimisation enabled.

Of course, linux/byteorder.h and linux/swab.h seem to do much the same. Any idea why we have both sets? There seems to be unnecessary redundancy.

David

#2 lore raw

From: David Howells <[email protected]>
Date: Wed, 27 Aug 2008 12:12:24 +0100

Quoted 6 lines

fs/afs/fsclient.c static int afs_deliver_fs_fetch_data(struct afs_call *call, · line 319

Quoted 1 line · Harvey Harrison
fs/afs/fsclient.c +1 -0
+		if (call->operation_ID != __constant_htonl(FSFETCHDATA64)) {

Doesn't htonl() resolve to this for a constant argument? Following through the definitions, it certainly looks like it ought to:

Yes it does.

The __constant_*() interfaces should only be uses for things that must be evaluated at compile time (static data initializations, switch statement case values etc.).

#3 lore raw
Quoted 10 lines · David Miller

From: David Howells <[email protected]>
Date: Wed, 27 Aug 2008 12:12:24 +0100

Quoted 6 lines

fs/afs/fsclient.c static int afs_deliver_fs_fetch_data(struct afs_call *call, · line 319

Quoted 1 line · Harvey Harrison
fs/afs/fsclient.c +1 -0
+		if (call->operation_ID != __constant_htonl(FSFETCHDATA64)) {

Doesn't htonl() resolve to this for a constant argument? Following through the definitions, it certainly looks like it ought to:

Quoted 5 lines

Yes it does.

The __constant_*() interfaces should only be uses for things that must be evaluated at compile time (static data initializations, switch statement case values etc.).

It's not just "must be evaluated at compile time"; it's "must be C constant expressions", which is much stricter. And no, in the case quoted above __constant_ form is not needed. Just use htonl()...