On Sat, Nov 6, 2021 at 9:08 AM Shyam Prasad N <nspmangalore@xxxxxxxxx> wrote: > > 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. Hi Enzo, One "problem" with using just the version field is that it has a predefined format. This may not help us in decoding what distro and if it is a custom kernel. utsname()->release gives all that. So I'm planning to use workstation_name in the "hostname:release" format. Let me know if you see some issues with it. > > >> > >> >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 -- Regards, Shyam
