diff mbox series

patches to move ksmbd and cifs under new subdirectory

Message ID CAH2r5msVBGuRbv2tEuZWLR6_pSNNaoeihx=CjvgZ7NxwCNqZvA@mail.gmail.com (mailing list archive)
State New, archived
Headers show
Series patches to move ksmbd and cifs under new subdirectory | expand

Commit Message

Steve French May 22, 2023, 4:33 p.m. UTC 
Following up on the email thread suggestion to move fs/ksmbd and
fs/cifs and fs/smbfs_common all under a common directory fs/smb, here
is an updated
patchset for that (added one small patch).


    1) smb3: move Documentation/filesystems/cifs to
Documentation/filesystems/smb
    As suggested by Namjae, update the directory for ksmbd/cifs.ko
Documentation.
    Documentation/filesystems/cifs contains both server and client information
    so its pathname is misleading.  In addition, the directory fs/smb
    now contains both server and client, so move Documentation/filesystems/cifs
    to Documentation/filesystems/smb

    Suggested-by: Namjae Jeon <linkinjeon@kernel.org>

2) cifs: correct references in Documentation to old fs/cifs path
    The fs/cifs directory has moved to fs/smb/client, correct mentions
    of this in Documentation and comments.

3)  smb: move client and server files to common directory fs/smb
   As suggested by Linus, move CIFS/SMB3 related client and server
   files (cifs.ko and ksmbd.ko and helper modules) to new fs/smb subdirectory:

       fs/cifs --> fs/smb/client
       fs/ksmbd --> fs/smb/server
       fs/smbfs_common --> fs/smb/conmon


Thanks,

Steve

Comments

Tom Talpey May 22, 2023, 5:20 p.m. UTC | #1 
On 5/22/2023 12:33 PM, Steve French wrote:
> Following up on the email thread suggestion to move fs/ksmbd and
> fs/cifs and fs/smbfs_common all under a common directory fs/smb, here
> is an updated
> patchset for that (added one small patch).
> 
> 
>      1) smb3: move Documentation/filesystems/cifs to
> Documentation/filesystems/smb
>      As suggested by Namjae, update the directory for ksmbd/cifs.ko
> Documentation.
>      Documentation/filesystems/cifs contains both server and client information
>      so its pathname is misleading.  In addition, the directory fs/smb
>      now contains both server and client, so move Documentation/filesystems/cifs
>      to Documentation/filesystems/smb
> 
>      Suggested-by: Namjae Jeon <linkinjeon@kernel.org>
> 
> 2) cifs: correct references in Documentation to old fs/cifs path
>      The fs/cifs directory has moved to fs/smb/client, correct mentions
>      of this in Documentation and comments.
> 
> 3)  smb: move client and server files to common directory fs/smb
>     As suggested by Linus, move CIFS/SMB3 related client and server
>     files (cifs.ko and ksmbd.ko and helper modules) to new fs/smb subdirectory:
> 
>         fs/cifs --> fs/smb/client
>         fs/ksmbd --> fs/smb/server
>         fs/smbfs_common --> fs/smb/conmon

There's a typo "conmon" here, which is also present in the changelog.
The "git mv" in the patch is correct.

Regarding the fs/smb/Kconfig, curious why "CIFS" selects SMB_CLIENT?
There's no need to bring in the client in a server-only config.
Or, did this mean to be SMBFS to bring in fs/smb/common instead?
Or, maybe it's time to do away with the "CIFS" option entirely?

config CIFS
  	tristate "SMB3 and CIFS support (advanced network filesystem)"
  	depends on INET
+	select SMB_CLIENT <-- ??
  	select NLS
  	select CRYPTO
  	select CRYPTO_MD5

Either way, is this second SMB_CLIENT=y in fs/smb/Kconfig a typo? Should
it be =m?

+config SMBFS
+	tristate
+	default y if CIFS=y || SMB_CLIENT=y || SMB_SERVER=y
+	default m if CIFS=m || SMB_CLIENT=y  <-- ? || SMB_SERVER=m

Tom.
Linus Torvalds May 22, 2023, 5:33 p.m. UTC | #2 
On Mon, May 22, 2023 at 9:33 AM Steve French <smfrench@gmail.com> wrote:
>
> Following up on the email thread suggestion to move fs/ksmbd and
> fs/cifs and fs/smbfs_common all under a common directory fs/smb, here
> is an updated patchset for that (added one small patch).

Looks fine to me.

I wouldn't have noticed the typo that Tom Talpey mentioned (misspelled
"common" in the commit message of the first patch), and that
SMB_CLIENT config variable is odd.

I'm actually surprised that Kconfig didn't complain about the

        select SMB_CLIENT

when there is no actual declaration of that Kconfig variable, just a random use.

That thing seems confusing and confused, and isn't related to the
renaming, so please drop the new random SMB_CLIENT config variable. If
you want to introduce a new Kconfig variable later for some reason,
that's fine, but please don't mix those kinds of changes up with pure
renames..

                Linus
Steve French May 23, 2023, 6:39 a.m. UTC | #3 
On Mon, May 22, 2023 at 12:33 PM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> On Mon, May 22, 2023 at 9:33 AM Steve French <smfrench@gmail.com> wrote:
> >
> > Following up on the email thread suggestion to move fs/ksmbd and
> > fs/cifs and fs/smbfs_common all under a common directory fs/smb, here
> > is an updated patchset for that (added one small patch).
>
> Looks fine to me.
>
> I wouldn't have noticed the typo that Tom Talpey mentioned (misspelled
> "common" in the commit message of the first patch), and that
> SMB_CLIENT config variable is odd.
>
> I'm actually surprised that Kconfig didn't complain about the
>
>         select SMB_CLIENT
>
> when there is no actual declaration of that Kconfig variable, just a random use.
>
> That thing seems confusing and confused, and isn't related to the
> renaming, so please drop the new random SMB_CLIENT config variable. If
> you want to introduce a new Kconfig variable later for some reason,
> that's fine, but please don't mix those kinds of changes up with pure
> renames.

I have removed CONFIG_SMB_CLIENT (and fixed the spelling typo in the comment).
Updated patch 1 attached (the other two are unchanged).

My reason for adding CONFIG_SMB_CLIENT, enabling CONFIG_SMB_CLIENT
when CONFIG_CIFS was enabled, I was trying to make the Makefile more clear
(without changing any behavior):

e.g. this seemed less confusing
+++ b/fs/smb/Makefile
@@ -0,0 +1,5 @@
+# SPDX-License-Identifier: GPL-2.0
+
+obj-$(CONFIG_SMBFS)            += common/
+obj-$(CONFIG_SMB_CLIENT)      += client/
+obj-$(CONFIG_SMB_SERVER)       += server/

instead of

+++ b/fs/smb/Makefile
@@ -0,0 +1,5 @@
+# SPDX-License-Identifier: GPL-2.0
+
+obj-$(CONFIG_SMBFS)         += common/
+obj-$(CONFIG_CIFS)             += client/
+obj-$(CONFIG_SMB_SERVER)       += server/
Linus Torvalds May 23, 2023, 5:34 p.m. UTC | #4 
On Mon, May 22, 2023 at 11:39 PM Steve French <smfrench@gmail.com> wrote:
>
> My reason for adding CONFIG_SMB_CLIENT, enabling CONFIG_SMB_CLIENT
> when CONFIG_CIFS was enabled, I was trying to make the Makefile more clear
> (without changing any behavior):

That sounds ok, but I think it should be done separately from the
move. Keep the move as a pure move/rename, not "new things".

Also, when you actually do this cleanup, I think you really should just do

  config SMB
        tristate

  config SMB_CLIENT
        tristate

to declare them, but *not* have that

        default y if CIFS=y || SMB_SERVER=y
        default m if CIFS=m || SMB_SERVER=m

kind of noise anywhere. Not for SMBFS, not for SMB_CLIENT.

Just do

        select SMBFS
        select SMB_CLIENT

in the current CIFS Kconfig entry. And then SMB_SERVER can likewise do

        select SMBFS

and I think it will all automatically do what those much more complex
"default" expressions currently do.

But again - I think this kind of "clean things up" should be entirely
separate from the pure code movement. Don't do new functionality when
moving things, just do the minimal required infrastructure changes to
make things work with the movement.

              Linus
Steve French May 24, 2023, 2:41 a.m. UTC | #5 
Lightly updated (e.g. to include a missing trivial change needed to
Documentation/filesystems/index.rst that Namjae noticed).  See
attached.

Presumably can defer the additional cleanup/prettying (ie those beyond
those required for the directory rename) with distinct patches later.

On Tue, May 23, 2023 at 12:35 PM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> On Mon, May 22, 2023 at 11:39 PM Steve French <smfrench@gmail.com> wrote:
> >
> > My reason for adding CONFIG_SMB_CLIENT, enabling CONFIG_SMB_CLIENT
> > when CONFIG_CIFS was enabled, I was trying to make the Makefile more clear
> > (without changing any behavior):
>
> That sounds ok, but I think it should be done separately from the
> move. Keep the move as a pure move/rename, not "new things".
>
> Also, when you actually do this cleanup, I think you really should just do
>
>   config SMB
>         tristate
>
>   config SMB_CLIENT
>         tristate
>
> to declare them, but *not* have that
>
>         default y if CIFS=y || SMB_SERVER=y
>         default m if CIFS=m || SMB_SERVER=m
>
> kind of noise anywhere. Not for SMBFS, not for SMB_CLIENT.
>
> Just do
>
>         select SMBFS
>         select SMB_CLIENT
>
> in the current CIFS Kconfig entry. And then SMB_SERVER can likewise do
>
>         select SMBFS
>
> and I think it will all automatically do what those much more complex
> "default" expressions currently do.
>
> But again - I think this kind of "clean things up" should be entirely
> separate from the pure code movement. Don't do new functionality when
> moving things, just do the minimal required infrastructure changes to
> make things work with the movement.
>
>               Linus
Steve French May 24, 2023, 4:27 a.m. UTC | #6 
One more minor change (fs/smb/common/Makefile was missing a two line change).
Running automated tests now.

Attached updated patch

On Tue, May 23, 2023 at 9:41 PM Steve French <smfrench@gmail.com> wrote:
>
> Lightly updated (e.g. to include a missing trivial change needed to
> Documentation/filesystems/index.rst that Namjae noticed).  See
> attached.
>
> Presumably can defer the additional cleanup/prettying (ie those beyond
> those required for the directory rename) with distinct patches later.
>
> On Tue, May 23, 2023 at 12:35 PM Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> >
> > On Mon, May 22, 2023 at 11:39 PM Steve French <smfrench@gmail.com> wrote:
> > >
> > > My reason for adding CONFIG_SMB_CLIENT, enabling CONFIG_SMB_CLIENT
> > > when CONFIG_CIFS was enabled, I was trying to make the Makefile more clear
> > > (without changing any behavior):
> >
> > That sounds ok, but I think it should be done separately from the
> > move. Keep the move as a pure move/rename, not "new things".
> >
> > Also, when you actually do this cleanup, I think you really should just do
> >
> >   config SMB
> >         tristate
> >
> >   config SMB_CLIENT
> >         tristate
> >
> > to declare them, but *not* have that
> >
> >         default y if CIFS=y || SMB_SERVER=y
> >         default m if CIFS=m || SMB_SERVER=m
> >
> > kind of noise anywhere. Not for SMBFS, not for SMB_CLIENT.
> >
> > Just do
> >
> >         select SMBFS
> >         select SMB_CLIENT
> >
> > in the current CIFS Kconfig entry. And then SMB_SERVER can likewise do
> >
> >         select SMBFS
> >
> > and I think it will all automatically do what those much more complex
> > "default" expressions currently do.
> >
> > But again - I think this kind of "clean things up" should be entirely
> > separate from the pure code movement. Don't do new functionality when
> > moving things, just do the minimal required infrastructure changes to
> > make things work with the movement.
> >
> >               Linus
>
>
>
> --
> Thanks,
>
> Steve
diff mbox series

Patch

From 449ef93f80ae51c0edac57d74aae3bc113dcca58 Mon Sep 17 00:00:00 2001
From: Steve French <stfrench@microsoft.com>
Date: Mon, 22 May 2023 09:50:33 -0500
Subject: [PATCH 3/3] smb3: move Documentation/filesystems/cifs to
 Documentation/filesystems/smb

Documentation/filesystems/cifs contains both server and client information
so its pathname is misleading.  In addition, the directory fs/smb
now contains both server and client, so move Documentation/filesystems/cifs
to Documentation/filesystems/smb

Suggested-by: Namjae Jeon <linkinjeon@kernel.org>
Signed-off-by: Steve French <stfrench@microsoft.com>
---
 Documentation/filesystems/{cifs => smb}/cifsroot.rst | 0
 Documentation/filesystems/{cifs => smb}/index.rst    | 0
 Documentation/filesystems/{cifs => smb}/ksmbd.rst    | 0
 MAINTAINERS                                          | 2 +-
 4 files changed, 1 insertion(+), 1 deletion(-)
 rename Documentation/filesystems/{cifs => smb}/cifsroot.rst (100%)
 rename Documentation/filesystems/{cifs => smb}/index.rst (100%)
 rename Documentation/filesystems/{cifs => smb}/ksmbd.rst (100%)

diff --git a/Documentation/filesystems/cifs/cifsroot.rst b/Documentation/filesystems/smb/cifsroot.rst
similarity index 100%
rename from Documentation/filesystems/cifs/cifsroot.rst
rename to Documentation/filesystems/smb/cifsroot.rst
diff --git a/Documentation/filesystems/cifs/index.rst b/Documentation/filesystems/smb/index.rst
similarity index 100%
rename from Documentation/filesystems/cifs/index.rst
rename to Documentation/filesystems/smb/index.rst
diff --git a/Documentation/filesystems/cifs/ksmbd.rst b/Documentation/filesystems/smb/ksmbd.rst
similarity index 100%
rename from Documentation/filesystems/cifs/ksmbd.rst
rename to Documentation/filesystems/smb/ksmbd.rst
diff --git a/MAINTAINERS b/MAINTAINERS
index 902f763e845d..6152a4251ce7 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -11300,7 +11300,7 @@  R:	Tom Talpey <tom@talpey.com>
 L:	linux-cifs@vger.kernel.org
 S:	Maintained
 T:	git git://git.samba.org/ksmbd.git
-F:	Documentation/filesystems/cifs/ksmbd.rst
+F:	Documentation/filesystems/smb/ksmbd.rst
 F:	fs/smb/common/
 F:	fs/smb/server/
 
-- 
2.34.1