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 <[email protected]>
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
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 <[email protected]>
>
> 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.
On Mon, May 22, 2023 at 9:33 AM Steve French <[email protected]> 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
On Mon, May 22, 2023 at 12:33 PM Linus Torvalds
<[email protected]> wrote:
>
> On Mon, May 22, 2023 at 9:33 AM Steve French <[email protected]> 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/
--
Thanks,
Steve
On Mon, May 22, 2023 at 11:39 PM Steve French <[email protected]> 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
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
<[email protected]> wrote:
>
> On Mon, May 22, 2023 at 11:39 PM Steve French <[email protected]> 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
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 <[email protected]> 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
> <[email protected]> wrote:
> >
> > On Mon, May 22, 2023 at 11:39 PM Steve French <[email protected]> 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
--
Thanks,
Steve