Re: [RFC PATCH v2 00/10] cifs: rename of several structs and variables

From: Tom Talpey <tom@talpey.com>
To: Enzo Matsumiya <ematsumiya@suse.de>
Cc: linux-cifs@vger.kernel.org, smfrench@gmail.com, pc@cjr.nz,
	ronniesahlberg@gmail.com, nspmangalore@gmail.com
Subject: Re: [RFC PATCH v2 00/10] cifs: rename of several structs and variables
Date: Fri, 29 Jul 2022 15:12:40 -0400	[thread overview]
Message-ID: <c7068960-34bc-4783-ecdd-6c6164a9bfa7@talpey.com> (raw)
In-Reply-To: <20220726024105.5jd3uf6keus5bya5@cyberdelia>

On 7/25/2022 10:41 PM, Enzo Matsumiya wrote:
> On 07/25, Tom Talpey wrote:
>> On 7/25/2022 6:36 PM, Enzo Matsumiya wrote:
>>> Hi all,
>>>
>>> This patch set (v2) renames several cifs.ko data structures, 
>>> variables, and
>>> functions with the goal to improve readability of the code.
>>>
>>> In summary, what's been done:
>>> - change from CamelCase to snake_case
>>> - try to give more meaning to globals and struct members
>>> - rename status fields (server, ses, tcon), define constants for the
>>>   various statuses (4/11 can be shared between those structs, others are
>>>   specific to each)
>>> - rename of list_head variables to better represent whether they'are
>>>   used as a list element ("head") or a list per se. Also tried to give
>>>   more meaning to these, as "rlist", "tlist", "llist" looked confusing
>>>   and, sometimes, ambiguous.
>>> - remove redundant prefixes from struct members name, e.g.
>>>   tcon_tlink's tl_*, smb_rqst's rq_*, cifs_fattr's cf_*, etc
>>>
>>> No functional changes has been made.
>>>
>>> I know these touch some very old code that older devs are highly used
>>> to, but I see this as an improvement to reading the code for everyone.
>>>
>>> I'll be waiting for your reviews and feedback.
>>
>> Enzo, I think this effort is great. If you combine this modernization
>> with refactoring SMB1 into separate files to make it easier to do
>> away with entirely, I'll be even more supportive.
> 
> Tom, thanks for your feedback!
> As a matter of fact, I do have a branch with SMB1 code isolated [*] and the
> module renamed to "SMBFS" to modernize this module, while also keeping
> it SMB-version-agnostic.
> 
> I'm sending my changes gradually so I don't have to change a lot of
> things in case there's a lot of negative feedback.
> 
> 
> [*] - current discussions:
> - is "smbfs" the best module name? Steve suggested "smb3", but, again,
>    this ties the module to a specific SMB version

I think "smbfs" is more consistent with other kmods. And I'd be
careful about a specific version like "smb3", because that paints
it into the corner if it also supports smb2, or future dialects.

> - should SMB1 code be isolated as in source-code only or should it be a
>    different object (i.e. only built through kernel config and
>    "disable_legacy_dialects" wiped away)? Or even a separate module?

There's no single answer here, but my personal view is that it should
become a separate module. As a compile-time option, the distros are
basically forced to turn it on. We don't want that. As a module, they
can choose to leave it out. We do want that.

> In any case, I'm keeping "cifs" as a module alias for SMB1 code for now,
> and I think we'll need it that way for some time, but at least the
> internal migration will be done by then.

Yes, it's definitely needed (for now) to maintain a "cifs" module alias,
because user scripts might be using it to modload. In future if smb1
becomes a module, then drop "cifs" entirely.

Tom.

> Thanks again,
> 
> Enzo
> 
>>> v2:
>>>   - remove status typedefs (suggested by Christoph Hellwig)
>>>   - define status constants instead, reuse some between different
>>>     structs so we don't have to create a different set of statuses
>>>     for each cifs struct
>>>
>>> Enzo Matsumiya (10):
>>>   cifs: rename xid/mid globals
>>>   cifs: rename global counters
>>>   cifs: rename "TCP_Server_Info" struct to "cifs_server_info"
>>>   cifs: rename cifs{File,Lock,Inode}Info structs and more
>>>   cifs: convert server info vars to snake_case
>>>   cifs: change status and security types enums to constants
>>>   cifs: rename cifsFYI to debug_level
>>>   cifs: rename list_head fields
>>>   cifs: rename more CamelCase to snake_case
>>>   cifs: rename more list_heads, remove redundant prefixes
>>>
>>>  fs/cifs/Kconfig         |   2 +-
>>>  fs/cifs/asn1.c          |   4 +-
>>>  fs/cifs/cifs_debug.c    | 158 ++++-----
>>>  fs/cifs/cifs_debug.h    |  29 +-
>>>  fs/cifs/cifs_spnego.c   |   4 +-
>>>  fs/cifs/cifs_spnego.h   |   2 +-
>>>  fs/cifs/cifs_swn.c      |  24 +-
>>>  fs/cifs/cifs_swn.h      |   8 +-
>>>  fs/cifs/cifs_unicode.c  |   4 +-
>>>  fs/cifs/cifs_unicode.h  |   2 +-
>>>  fs/cifs/cifsacl.c       |  22 +-
>>>  fs/cifs/cifsencrypt.c   |  78 ++---
>>>  fs/cifs/cifsfs.c        | 124 +++----
>>>  fs/cifs/cifsglob.h      | 694 ++++++++++++++++++++--------------------
>>>  fs/cifs/cifsproto.h     | 172 +++++-----
>>>  fs/cifs/cifssmb.c       | 356 ++++++++++-----------
>>>  fs/cifs/connect.c       | 574 ++++++++++++++++-----------------
>>>  fs/cifs/dfs_cache.c     | 178 +++++------
>>>  fs/cifs/dfs_cache.h     |  40 +--
>>>  fs/cifs/dir.c           |  16 +-
>>>  fs/cifs/file.c          | 636 ++++++++++++++++++------------------
>>>  fs/cifs/fs_context.c    |   8 +-
>>>  fs/cifs/fs_context.h    |   2 +-
>>>  fs/cifs/fscache.c       |  18 +-
>>>  fs/cifs/fscache.h       |  10 +-
>>>  fs/cifs/inode.c         | 530 +++++++++++++++---------------
>>>  fs/cifs/ioctl.c         |  18 +-
>>>  fs/cifs/link.c          |  26 +-
>>>  fs/cifs/misc.c          | 185 ++++++-----
>>>  fs/cifs/netmisc.c       |   4 +-
>>>  fs/cifs/ntlmssp.h       |   6 +-
>>>  fs/cifs/readdir.c       | 344 ++++++++++----------
>>>  fs/cifs/sess.c          | 142 ++++----
>>>  fs/cifs/smb1ops.c       | 182 +++++------
>>>  fs/cifs/smb2file.c      |  36 +--
>>>  fs/cifs/smb2inode.c     | 136 ++++----
>>>  fs/cifs/smb2maperror.c  |   2 +-
>>>  fs/cifs/smb2misc.c      |  72 ++---
>>>  fs/cifs/smb2ops.c       | 555 ++++++++++++++++----------------
>>>  fs/cifs/smb2pdu.c       | 596 +++++++++++++++++-----------------
>>>  fs/cifs/smb2proto.h     |  68 ++--
>>>  fs/cifs/smb2transport.c | 112 +++----
>>>  fs/cifs/smbdirect.c     |  28 +-
>>>  fs/cifs/smbdirect.h     |  16 +-
>>>  fs/cifs/transport.c     | 236 +++++++-------
>>>  fs/cifs/xattr.c         |  12 +-
>>>  46 files changed, 3230 insertions(+), 3241 deletions(-)
>>>
> 