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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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(-)





[Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux