Thanks for the reviews, Paulo and Enzo. Please read my replies below: On Sat, Nov 6, 2021 at 7:10 AM Steve French <smfrench@xxxxxxxxx> wrote: > > Interesting suggestions, probably worth experimenting with but even the original patch could help a lot eg help server to understand type of client connecting to it when debugging > > On Fri, Nov 5, 2021, 21:38 Enzo Matsumiya <ematsumiya@xxxxxxx> wrote: >> >> Hi Shyam, >> >> I have some observations/suggestions regarding your patch: >> >> On 11/06, Shyam Prasad N wrote: >> >Hi Steve, >> > >> >Please review this patch, and let me know what you think. >> >Having this info in the workstation field of session setup helps >> >server debugging in two ways. >> >1. It helps identify the client by node name. >> >2. It helps get the kernel release running on the client side. >> > >> >https://github.com/sprasad-microsoft/smb3-kernel-client/commit/d988e704dd9170c19ff94d9421c017e65dbbaac1.patch >> >> - AFAICS it doesn't consider runtime hostname changes. Is it important >> to keep track of it? Would changing it mid-auth steps break it >> somehow? I think that's okay. AFAIK, that's only used for debugging/troubleshooting purposes. So it doesn't need to be a 100% accurate. >> >> - I didn't understand the purpose of CIFS_DEFAULT_WORKSTATION_NAME. Why >> not simply use utsname()->nodename? Or even init_utsname()->nodename, which >> is supposed to be always valid. I initially did not have the utsname changes. That idea was an afterthought. Sure. I'll update the patch to fix this. >> >> - Ditto for CIFS_MAX_WORKSTATION_LEN. utsname()->nodename has at most 65 >> bytes (__NEW_UTS_LEN + 1) anyway. Perhaps using MAXHOSTNAMELEN from >> <asm/param.h> would be a more generic approach. >> (btw this is because nodename is the unqualified hostname, sans-domain) Noted. >> >> - Instead of setting workstation_name to "nodename:release", why not >> implement the VERSION structure (MS-NLMP 2.2.2.10)? Then use >> LINUX_VERSION_* from <linux/version.h> or parse utsname()->release. That's a good idea. Let me explore that too. >> >> >I ran some basic testing with the patch. Seems to serve the purpose. >> >Please let me know if I'm missing something. >> >> I hope I didn't miss anything. >> >> >> Cheers, >> >> Enzo -- Regards, Shyam
