On Thursday, January 12, 2017 2:12:56 PM CET David Howells wrote: > Arnd Bergmann <a...@arndb.de> 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