On 3/21/2022 11:01 AM, Steve French wrote:
Wouldn't u64 for these with no conversion (the original code was consistent before half of use of fields converted to le64) be faster, easier?
I guess that's what Paulo is going to do. It's certainly faster and easier, but I predict it won't close the door on future code issues. Someone is going to try to byteswap, or treat it as an integer somehow. I'm advocating for correctness. But fast and easy are your call. Tom.
On Mon, Mar 21, 2022, 07:55 Tom Talpey <tom@xxxxxxxxxx <mailto:tom@xxxxxxxxxx>> wrote:On 3/21/2022 8:30 AM, Paulo Alcantara wrote: > Tom Talpey <tom@xxxxxxxxxx <mailto:tom@xxxxxxxxxx>> writes: > >> On 3/20/2022 8:20 PM, Paulo Alcantara wrote: >>> The client used to partially convert the fids to le64, while storing >>> or sending them by using host endianness. This broke the client on >>> big-endian machines. Instead of converting them to le64, store them >>> verbatim and then avoid byteswapping when sending them over wire. >>> >>> Signed-off-by: Paulo Alcantara (SUSE) <pc@xxxxxx <mailto:pc@xxxxxx>> >>> --- >>> fs/cifs/smb2misc.c | 4 ++-- >>> fs/cifs/smb2ops.c | 8 +++---- >>> fs/cifs/smb2pdu.c | 53 ++++++++++++++++++++-------------------------- >>> 3 files changed, 29 insertions(+), 36 deletions(-) >>> >>> diff --git a/fs/cifs/smb2misc.c b/fs/cifs/smb2misc.c >>> index b25623e3fe3d..3b7c636be377 100644 >>> --- a/fs/cifs/smb2misc.c >>> +++ b/fs/cifs/smb2misc.c >>> @@ -832,8 +832,8 @@ smb2_handle_cancelled_mid(struct mid_q_entry *mid, struct TCP_Server_Info *serve >>> rc = __smb2_handle_cancelled_cmd(tcon, >>> le16_to_cpu(hdr->Command), >>> le64_to_cpu(hdr->MessageId),>>> - le64_to_cpu(rsp->PersistentFileId), >>> - le64_to_cpu(rsp->VolatileFileId));>>> + rsp->PersistentFileId, >>> + rsp->VolatileFileId); >> >> This conflicts with the statement "store them verbatim". Because the >> rsp->{Persistent,Volatile}FileId fields are u64 (integer) types, >> they are not being stored verbatim, they are being manipulated >> by the CPU load/store instructions. Storing them into a u8[8] >> array is more to the point. > > Yes, makes sense. > >> If course, if the rsp structure is purely private to the code, then >> the structure element type is similarly private. But a debugger, or >> a future structure reference, may once again get it wrong >> >> Are you rejecting the idea of using a byte array? > > No. That would work, too. I was just trying to avoid changing a lot of > places and eventually making it harder to backport. > > I'll go with the byte array then. If you want to reduce a bit of code change, you could let the compiler generate the loads and stores via memcpy, with (perhaps) a struct { u8[8] } instead of the bare array. Tom.
