From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1750994AbdALPlQ (ORCPT ); Thu, 12 Jan 2017 10:41:16 -0500 Received: from mout.kundenserver.de ([212.227.17.10]:55993 "EHLO mout.kundenserver.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750810AbdALPlO (ORCPT ); Thu, 12 Jan 2017 10:41:14 -0500 From: Arnd Bergmann To: David Howells Cc: linux-kernel@vger.kernel.org, ruchandani.tina@gmail.com, linux-afs@lists.infradead.org Subject: Re: [PATCH 1/2] afs: Move UUID struct to linux/uuid.h Date: Thu, 12 Jan 2017 16:40:38 +0100 Message-ID: <6962749.4mk109advR@wuerfel> User-Agent: KMail/5.1.3 (Linux/4.4.0-34-generic; KDE/5.18.0; x86_64; ; ) In-Reply-To: <13021.1484230376@warthog.procyon.org.uk> References: <107420020.65PPp1ZfLz@wuerfel> <148422219487.9419.2588525606361566422.stgit@warthog.procyon.org.uk> <13021.1484230376@warthog.procyon.org.uk> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" X-Provags-ID: V03:K0:s04L33V1keBSqSargh8ZYdskZL14GXdvuco9JEfUqAN+zVqXmHd Bwpz3OgddVLB4x9cCAV98T47ptz0y81H6UfgRtkaV8dIc6fH/SqsbXpJXkGRl8A6ftYXrUr fMeNo2R37dsH9ldtMA/CaJQJDcrZuEwMcg1y/x2QV23hWK5CMNIJxXiK5SX3L60CLvTA44v hvjQwxaMJ2xMAJkB9+tDQ== X-UI-Out-Filterresults: notjunk:1;V01:K0:0gd3QNOKFtM=:59dBA0GKXSPetbnDhrVWS5 FKcY4R/60RoQBXqZhU3Vp5aQ51X/Y5IZ42JiTls7gPA2hTaXdw9iRxoNHpI3qYSqyEi0gaBZ3 IY6K6gFhCqhC3G72VvGp15bLay1Nk2VSv1r2WrP+wfz5GoQ+LqgHt7LF9+3dtLvJeRSgIkiL7 8jIFoGtXFi83AJ318iArLPaairJFDHTSgUV8445+kyIJE6eFmfzjcZWvIDqMm+i7LzmBPkacX ij4ly5bIvzWH0OJ51hTp/9sGf5gwPXzc2QaNvdgFp/T15dj5oUCNMlMtF0ZVYL9VzXX/ARNvY Gw6NTqMVB+kaGqP5rkh/IdULI4mgJseWSaQFyHyK+V1GnQkUeSBhxh3tMuzXzBXSZjiYfKXeB 9qZg/iEUY3bRLm1Zg3+CHL/BoDrUk3rEud/q5/qKL61mO4CfduoL8iCMsffG3Q5aMlJJnOs+z bQwpPnO1BZ8zvKMzg5Bizx95Mx0lJ/EyD233TZIcyu9FVYhADjM/f89IXaA5835kd6WIYOQQD C1+N6tAFmuOvVCp6b5fAt3CYBgJ9BPevq2PzfNpRTN4LRenNzGoGJDC6T+Pgx4jIuH3aKj16T ATFfq3uFiISSDIROzPwqmmsDncTCa0ZRahHhxGeKxxjdnNQ51KyvF1vqwt6G2RyNyjoYoEvvf T+eLKqofIxZ32gklfNlH6l19K5MAt5ygzsSjT6Qkakbqq8cCtoT7MjDD+MJ1KFEm08Pygt+Yx HuP8VtxLs3TVFki0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thursday, January 12, 2017 2:12:56 PM CET David Howells wrote: > Arnd Bergmann wrote: > > > Looks good to me, but I wonder if this part: > > > > r = call->request; > > - r->time_low = ntohl(b[0]); > > - r->time_mid = ntohl(b[1]); > > - r->time_hi_and_version = ntohl(b[2]); > > + 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]); > > > > should be considered a bugfix and split out into a > > separate patch. > > I changed the definitions in the struct from u16/u32 to __be16/__be32 so it's > not a bugfix. Ok. > > From what I understand about the mess in UUID formats, the time fields can > > either be big-endian (as defined) or little-endian (for all things > > Microsoft), > > RFC 4122 specified that the multi-octet fields are stored MSB-first. > > > and you are changing the representation from CPU-specific to big-endian, > > which makes it different for x86 and most ARM at least. > > In-kernel, not in the protocol. Ok, I assumed that the uuid was later sent out over the wire again in the in-memory format, but you are right, it does get sent out in the AFS specific format as a series of 32-bit big-endian values rather than the RFC4122 format. > The problem is that you can't do what you put in your suggested patch and just > copy the UUID produced by the generate_random_uuid() over the afs_uuid struct > since that puts the version in the wrong place. Got it. One more thing then: > @@ -569,9 +569,9 @@ static void SRXAFSCB_TellMeAboutYourself(struct > work_struct *work)> > memset(&reply, 0, sizeof(reply)); > reply.ia.nifs = htonl(nifs); > > - reply.ia.uuid[0] = htonl(afs_uuid.time_low); > - reply.ia.uuid[1] = htonl(afs_uuid.time_mid); > - reply.ia.uuid[2] = htonl(afs_uuid.time_hi_and_version); > + reply.ia.uuid[0] = afs_uuid.time_low; > + reply.ia.uuid[1] = htonl(ntohl(afs_uuid.time_mid)); > + reply.ia.uuid[2] = htonl(ntohl(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); > for (loop = 0; loop < 6; loop++) Shouldn't this be ntohs() instead of ntohl(), like this: reply.ia.uuid[1] = htonl(ntohl(afs_uuid.time_mid)); reply.ia.uuid[2] = htonl(ntohl(afs_uuid.time_hi_and_version)); My head is spinning a little from all the byteswapping, but it looks to me like the data here ends up in the wrong half of the on-wire data. Can you double-check this? Arnd