[RFC] Unify all programs into a single 'ksmbdctl' binary

[RFC] Unify all programs into a single 'ksmbdctl' binary

[Enzo Matsumiya]

Hi Namjae, list,

I've been working on the unification of all ksmbd-tools programs into a
single 'ksmbdctl' binary, and I would like to invite everyone to test
and/or provide me feedback on the implementation.

Since this a big-ish refactor, for now I'm sharing the code via my
GitHub repo:

https://github.com/ematsumiya/ksmbd-tools/tree/ksmbdctl

I can split it into smaller commits later on, if approved for merge.

Commit message below, for a better explanation.

Cheers,

Enzo

==================================
commit 1135e0f6b592fb48d6b20b919c44ddb961d0c51d
Author: Enzo Matsumiya <ematsumiya@suse.de>
Date:   Tue Nov 30 15:22:35 2021 -0300

     Unify all programs into a single 'ksmbdctl' binary

     This commit unifies all existing programs
     (ksmbd.{adduser,addshare,control,mountd}) into a single ksmbdctl binary.

     The intention is to make it more like other modern tools (e.g. git,
     nvme, virsh, etc) which have more clear user interface, readable
     commands, and also makes it easier to script.

     Example commands:
       # ksmbdctl share add myshare -o "guest ok=yes, writable=yes, path=/mnt/data"
       # ksmbdctl user add myuser
       # ksmbdctl user add -i $HOME/mysmb.conf anotheruser
       # ksmbdctl daemon start
     
     Besides adding a new "share list" command, any previously working
     functionality shouldn't be affected.

     Basic testing was done manually. Updated README to reflect these
     modifications.

     TODO:
     - run more complex tests in more complext environments
     - implement tests (for each command and subcommand)
     - create an abstract command interface, to make it easier to add/modify
       the commands
     - find and fix obvious bugs I missed

     Signed-off-by: Enzo Matsumiya <ematsumiya@suse.de>

Re: [RFC] Unify all programs into a single 'ksmbdctl' binary

[Namjae Jeon]

2021-12-01 3:47 GMT+09:00, Enzo Matsumiya <ematsumiya@suse.de>:
> Hi Namjae, list,
Hi Enzo,

First, Thanks for your work!
Cc: other maintainers.

>
> I've been working on the unification of all ksmbd-tools programs into a
> single 'ksmbdctl' binary, and I would like to invite everyone to test
> and/or provide me feedback on the implementation.
While checking this out, I'd love to hear from other maintainers.
>
> Since this a big-ish refactor, for now I'm sharing the code via my
> GitHub repo:
>
> https://github.com/ematsumiya/ksmbd-tools/tree/ksmbdctl
>
> I can split it into smaller commits later on, if approved for merge.
Great.
>
> Commit message below, for a better explanation.
I will check it and give feedback soon.
Thanks!
>
> Cheers,
>
> Enzo
>
> ==================================
> commit 1135e0f6b592fb48d6b20b919c44ddb961d0c51d
> Author: Enzo Matsumiya <ematsumiya@suse.de>
> Date:   Tue Nov 30 15:22:35 2021 -0300
>
>      Unify all programs into a single 'ksmbdctl' binary
>
>      This commit unifies all existing programs
>      (ksmbd.{adduser,addshare,control,mountd}) into a single ksmbdctl
> binary.
>
>      The intention is to make it more like other modern tools (e.g. git,
>      nvme, virsh, etc) which have more clear user interface, readable
>      commands, and also makes it easier to script.
>
>      Example commands:
>        # ksmbdctl share add myshare -o "guest ok=yes, writable=yes,
> path=/mnt/data"
>        # ksmbdctl user add myuser
>        # ksmbdctl user add -i $HOME/mysmb.conf anotheruser
>        # ksmbdctl daemon start
>
>      Besides adding a new "share list" command, any previously working
>      functionality shouldn't be affected.
>
>      Basic testing was done manually. Updated README to reflect these
>      modifications.
>
>      TODO:
>      - run more complex tests in more complext environments
>      - implement tests (for each command and subcommand)
>      - create an abstract command interface, to make it easier to
> add/modify
>        the commands
>      - find and fix obvious bugs I missed
>
>      Signed-off-by: Enzo Matsumiya <ematsumiya@suse.de>
>
>

Re: [RFC] Unify all programs into a single 'ksmbdctl' binary

[Sergey Senozhatsky]

On Wed, Dec 1, 2021 at 11:17 AM Namjae Jeon <linkinjeon@kernel.org> wrote:
>
> > I've been working on the unification of all ksmbd-tools programs into a
> > single 'ksmbdctl' binary, and I would like to invite everyone to test
> > and/or provide me feedback on the implementation.

Why?

Re: [RFC] Unify all programs into a single 'ksmbdctl' binary

[Enzo Matsumiya]

On 12/01, Sergey Senozhatsky wrote:
>On Wed, Dec 1, 2021 at 11:17 AM Namjae Jeon <linkinjeon@kernel.org> wrote:
>>
>> > I've been working on the unification of all ksmbd-tools programs into a
>> > single 'ksmbdctl' binary, and I would like to invite everyone to test
>> > and/or provide me feedback on the implementation.
>
>Why?

Hi Sergey. The reasoning was in the commit message included in the
email, but I'll quote it here and elaborate:

> ...
> The intention is to make it more like other modern tools (e.g. git,
> nvme, virsh, etc) which have more clear user interface, readable
> commands, and also makes it easier to script.
> 
> Example commands:
>   # ksmbdctl share add myshare -o "guest ok=yes, writable=yes, path=/mnt/data"
>   # ksmbdctl user add myuser
>   # ksmbdctl user add -i $HOME/mysmb.conf anotheruser
>   # ksmbdctl daemon start
> 
> Besides adding a new "share list" command, any previously working
> functionality shouldn't be affected.

- clearer user interface: having a single binary looks much clearer than
   having several separate programs when, e.g. the user is looking which
   program does what.

- more readable commands: continuing from topic above, the situation is
   improved especially when you see that, e.g., the ksmbd.addshare program
   also updates and deletes shares. With this unification, it is way more
   intuitive to use:

---- snip ----
% ./ksmbdctl share
Usage: ksmbdctl share <subcommand> <args> [options]
Share management.

List of available subcommands:
   add               Add a share
   delete            Delete a share
   update            Update a share
   list              List the names of all shares available
---- snip ----

- easier to script: having a defined structure for the commands makes it
   easier for users to automate ksmbd deployments. This is more of a
   consequence of the topics above, but should count as an advantage IMO.

As I also said in the commit message, the idea is to have an abstract
implementation so we can add/manage/remove commands without having to
rely on each command specifics. Of course, this shouldn't have much/any
effect for users, but will have great benefit for developers.

And, again, I ask you to consider the applications I used as inspiration
for such change, such as git, nvme-cli, virsh, which are tools that many
of us use every day and, e.g. "git help -a | grep "^   " | wc -l" says
there are about 144 git commands, but I sure don't have 144 git binaries
spread over my system.

I hope to have made myself clearer now, but please let me know
otherwise.


Cheers,

Enzo

Re: [RFC] Unify all programs into a single 'ksmbdctl' binary

[Aurélien Aptel]

On Thu, Dec 2, 2021 at 10:58 AM Sergey Senozhatsky
<senozhatsky@chromium.org> wrote:
> Why?

Just commenting from the peanut gallery but as a user I definitely
prefer one self-documenting tool with a predictable UI over 3.
It's also less maintenance work I would say.

We took the multi-program approach in cifs-utils and it quickly
becomes a mess as you inevitably add utilities. We later added smbinfo
as an attempt to unify things behind 1 tool.

Cheers,

Re: [RFC] Unify all programs into a single 'ksmbdctl' binary

[Sergey Senozhatsky]

Hello,

On Thu, Dec 2, 2021 at 1:57 AM Enzo Matsumiya <ematsumiya@suse.de> wrote:
> > ...
> > The intention is to make it more like other modern tools (e.g. git,
> > nvme, virsh, etc) which have more clear user interface, readable
> > commands, and also makes it easier to script.
> >
> > Example commands:
> >   # ksmbdctl share add myshare -o "guest ok=yes, writable=yes, path=/mnt/data"
> >   # ksmbdctl user add myuser
> >   # ksmbdctl user add -i $HOME/mysmb.conf anotheruser
> >   # ksmbdctl daemon start
> >
> > Besides adding a new "share list" command, any previously working
> > functionality shouldn't be affected.
>
> - clearer user interface: having a single binary looks much clearer than
>    having several separate programs when, e.g. the user is looking which
>    program does what.
>
> - more readable commands: continuing from topic above, the situation is
>    improved especially when you see that, e.g., the ksmbd.addshare program
>    also updates and deletes shares. With this unification, it is way more
>    intuitive to use:

I've always preferred the UNIX way: one app does one thing and one thing only.
This is why we have cp and mv, mkdir and touch, etc. we don't have a
single vfs-ctl
app that takes several hundred arguments and whose man page is basically a
small book (20+ pages long). This way we:
- keep manpages short and clear
- avoid params conflicts and ambiguity
- keep eggs in different baskets

> I ask you to consider the applications I used as inspiration for such change, such as git

Git is a tricky example. But OK. If this will be implemented in a way
git builtins are
implemented - then I don't see why we would not want this.

git builtins are basically independent apps (they even link all
commands into separate
binaries and then symlink them during installation) which have been
rewritten from either
shell/perl scripts or standalone binaries in C and put into builtins/
directory, for performance
reasons. There are still a handful of shell/perl and standalone
binaries - e.g. git-remote-ftp,
git-http-fetch, git-http-push, ./git-bisect is a shell script. And so on.

Built-in commands are, basically, independent binaris that have a
common ancestor with the
only exception that git does not fork/exec them (not all of them).
They even have entry points
that resemble main() - they take "int argc, const char **argv" - and
git passes its argc and argv
down to built-ins.

Schematically

git: main(int argc, char **argv) {
      builtin = lookup_builtin_command();
      builtin->run(argc, argv);
}

Is this what you have in mind?

>   # ksmbdctl daemon start

Is this going to fork ksmb daemon? Otherwise this looks confusing. I'd
say that ksmbd daemon
needs to have a different name that will clearly show that it's a ksmb
daemon, not the "control"
tool that adds shares and deletes users.

Re: [RFC] Unify all programs into a single 'ksmbdctl' binary

[Enzo Matsumiya]

On 12/03, Sergey Senozhatsky wrote:
>Hello,
>
>On Thu, Dec 2, 2021 at 1:57 AM Enzo Matsumiya <ematsumiya@suse.de> wrote:
>> > ...
>> > The intention is to make it more like other modern tools (e.g. git,
>> > nvme, virsh, etc) which have more clear user interface, readable
>> > commands, and also makes it easier to script.
>> >
>> > Example commands:
>> >   # ksmbdctl share add myshare -o "guest ok=yes, writable=yes, path=/mnt/data"
>> >   # ksmbdctl user add myuser
>> >   # ksmbdctl user add -i $HOME/mysmb.conf anotheruser
>> >   # ksmbdctl daemon start
>> >
>> > Besides adding a new "share list" command, any previously working
>> > functionality shouldn't be affected.
>>
>> - clearer user interface: having a single binary looks much clearer than
>>    having several separate programs when, e.g. the user is looking which
>>    program does what.
>>
>> - more readable commands: continuing from topic above, the situation is
>>    improved especially when you see that, e.g., the ksmbd.addshare program
>>    also updates and deletes shares. With this unification, it is way more
>>    intuitive to use:
>
>I've always preferred the UNIX way: one app does one thing and one thing only.
>This is why we have cp and mv, mkdir and touch, etc. we don't have a
>single vfs-ctl
>app that takes several hundred arguments and whose man page is basically a
>small book (20+ pages long). This way we:
>- keep manpages short and clear
>- avoid params conflicts and ambiguity
>- keep eggs in different baskets

Sure, I agree. Again, this could also be worked out in my proposal (I
haven't touched much besides README yet though).
For the missing stuff, e.g. manpages, we can also implement them the git
way, like "man 1 ksmbdctl" vs "man 1 ksmbdctl-share", where the former
would mention the latter, but only briefly and then indicate that a
sub-manpage exists for that command.

>> I ask you to consider the applications I used as inspiration for such change, such as git
>
> ... snip ...
>
>Built-in commands are, basically, independent binaris that have a
>common ancestor with the
>only exception that git does not fork/exec them (not all of them).
>They even have entry points
>that resemble main() - they take "int argc, const char **argv" - and
>git passes its argc and argv
>down to built-ins.
>
>Schematically
>
>git: main(int argc, char **argv) {
>      builtin = lookup_builtin_command();
>      builtin->run(argc, argv);
>}
>
>Is this what you have in mind?

Yes, I've implemented it exactly that way:

share/share_admin.h:
void share_usage(ksmbd_share_cmd cmd);
int share_cmd(int argc, char *argv[]);

user/user_admin.h:
void user_usage(ksmbd_user_cmd cmd);
int user_cmd(int argc, char *argv[]);

daemon/daemon.h:
void daemon_usage(ksmbd_daemon_cmd cmd);
int daemon_cmd(int argc, char *argv[]);

The *_usage() functions were something I was preparing to accomodate the
new command abstraction I mentioned earlier, but I still haven't got to
finish. I wanted to get this unification approved first.

>>   # ksmbdctl daemon start
>
>Is this going to fork ksmb daemon? Otherwise this looks confusing. I'd
>say that ksmbd daemon
>needs to have a different name that will clearly show that it's a ksmb
>daemon, not the "control"
>tool that adds shares and deletes users.

At daemon_process_start(), I did:

...
if(prctl(PR_SET_NAME, "ksmbd-daemon\0", 0, 0, 0);
	pr_info("Can't set program name: %s\n", strerr(errno));
...

which TBH I'm not sure is good enough. Alternatives/opinions are
welcome.


Cheers,

Enzo

Re: [RFC] Unify all programs into a single 'ksmbdctl' binary

[Enzo Matsumiya]

Hi all,

On 12/01, Namjae Jeon wrote:
>2021-12-01 3:47 GMT+09:00, Enzo Matsumiya <ematsumiya@suse.de>:
>> Hi Namjae, list,
>Hi Enzo,
>
>First, Thanks for your work!
>Cc: other maintainers.
>
>>
>> I've been working on the unification of all ksmbd-tools programs into a
>> single 'ksmbdctl' binary, and I would like to invite everyone to test
>> and/or provide me feedback on the implementation.
>While checking this out, I'd love to hear from other maintainers.
>>
>> Since this a big-ish refactor, for now I'm sharing the code via my
>> GitHub repo:
>>
>> https://github.com/ematsumiya/ksmbd-tools/tree/ksmbdctl
>>
>> I can split it into smaller commits later on, if approved for merge.
>Great.
>>
>> Commit message below, for a better explanation.
>I will check it and give feedback soon.
>Thanks!

Any feedback on this proposal? Should I focus on polishing + splitting
into meaningful commits?


Cheers,

Enzo

Re: [RFC] Unify all programs into a single 'ksmbdctl' binary

[Sergey Senozhatsky]

Hi,

On Thu, Dec 16, 2021 at 12:00 AM Enzo Matsumiya <ematsumiya@suse.de> wrote:
>
> Any feedback on this proposal? Should I focus on polishing + splitting
> into meaningful commits?

I haven't gotten a chance to look at it yet, but in general I have no
objections to the "git-way" implementation.

Re: [RFC] Unify all programs into a single 'ksmbdctl' binary

[Namjae Jeon]

2021-12-16 10:38 GMT+09:00, Sergey Senozhatsky <senozhatsky@chromium.org>:
> Hi,
>
> On Thu, Dec 16, 2021 at 12:00 AM Enzo Matsumiya <ematsumiya@suse.de> wrote:
>>
>> Any feedback on this proposal? Should I focus on polishing + splitting
>> into meaningful commits?
>
> I haven't gotten a chance to look at it yet, but in general I have no
> objections to the "git-way" implementation.
Thanks for your point out and opinion:)
Enzo, I will check your work at detail today. I will reply soon.
Thanks!
>

Re: [RFC] Unify all programs into a single 'ksmbdctl' binary

[Namjae Jeon]

2021-12-16 11:07 GMT+09:00, Namjae Jeon <linkinjeon@kernel.org>:
> 2021-12-16 10:38 GMT+09:00, Sergey Senozhatsky <senozhatsky@chromium.org>:
>> Hi,
>>
>> On Thu, Dec 16, 2021 at 12:00 AM Enzo Matsumiya <ematsumiya@suse.de>
>> wrote:
>>>
>>> Any feedback on this proposal? Should I focus on polishing + splitting
>>> into meaningful commits?
This work looks really good. Could you please split one mega patch
into small ones for review ?

Thanks!
>>
>> I haven't gotten a chance to look at it yet, but in general I have no
>> objections to the "git-way" implementation.
> Thanks for your point out and opinion:)
> Enzo, I will check your work at detail today. I will reply soon.
> Thanks!
>>
>