On Mon, Aug 1, 2022 at 7:15 AM Enzo Matsumiya <ematsumiya@xxxxxxx> wrote: > > On 08/01, ronnie sahlberg wrote: > >Small nit : in cifssmb.c why comment out smb2proto.h, just delete the line. > > Agreed. Makes sense. Will fix this. > @Steve also, why ifdef everything instead of putting everything in a > "smb1" dir and just use Kconfig to build that dir? IOW like I did in in > my branch https://github.com/ematsumiya/linux/commits/refactor moving things to smb1 dir seems to be more related to the eventual move to having two modules instead of one, e.g. cifs.ko and an smb3.ko (or smbfs.ko or whatever we call it, today mounts can do "mount -t smb3" or "mount -t cifs" but we don't have separate modules for those). The other reason is that we don't have enough cifs (smb1 only) files for that to make sense yet. There are only two smb1 only C files. This patch adds the second one cifssmb.c (last release I added the first one smb1ops.c): cifs-$(CONFIG_CIFS_ALLOW_INSECURE_LEGACY) += smb1ops.o cifssmb.o We can probably split some headers out, but this step is needed first (isolating more SMB1/CIFS code from SMB3 code, and moving it where possible into SMB1/CIFS specific c files). > why ifdef everything Most of the code is in cifssmb.c and smb1ops.c but there are 58 "ifdef CONFIG_CIFS_ALLOW_INSECURE_LEGACY" but fewer than 20 of these are distinct functions (those we can move to cifssmb.c or a new smb1 helper c file) so most of those require code cleanup in those functions - and most are due to functions that have hard to read patterns like: if (smb1 unix extensions) { do some smb1 specific stuff which should be in a helper function return; } call some dialect specific helper function or similarly smb1 code wedged in dialect neutral code in file.c/dir.c/inode.c that we need to move e.g. if (backup_cred(cifs_sb) && is_smb1_server(server)) { FILE_DIRECTORY_INFO *fdi; SEARCH_ID_FULL_DIR_INFO *si; rc = cifs_backup_query_path_info(xid, tcon, sb, full_path, &smb1_backup_rsp_buf, &data); Let's do follow on patches to cleanup code like this in file.c/dir.c/inode.c -- Thanks, Steve
