[PATCH] New option "mangled names = illegal"
Jeremy Allison
jra at samba.org
Thu Dec 15 23:25:54 UTC 2016
On Thu, Dec 15, 2016 at 01:28:00PM +0100, Ralph Böhme wrote:
> Hi!
>
> This one seemed obvious to me, I must be missing something. :)
>
> With "mangled names = yes" we create mangled names for
>
> 1) any name that contains illegal NTFS characters
> 2) any name that exceeds 8.3 length limits
>
> In case of 2 the mangled name is put into the shortname buffer in find
> responses. Modern clients don't use the shortname anymore, so having an
> additional "mangled names" option that mangles only for case 1), but not 2)
> would be useful and reduce load on the server.
>
> Attached is a patch that implements such an option, I've called it "mangled
> names = illegal".
>
> A benchmark shows that it improves performance by more then 10% in corner cases
> like a directory with 20k files with long names:
>
> $ cd /path/share/dir
> $ i=1; while [ $i -lt 20001 ] ; do echo longfilename$i; touch longfilename$i; ((i++)); done
>
> "mangled named = yes"
> =====================
>
> $ time ./bin/smbclient -U slow%x -m smb3 //localhost/share -c "ls dir\*" > /dev/null
> Domain=[SLOWSERVER] OS=[] Server=[]
>
> real 0m0.402s
> user 0m0.055s
> sys 0m0.036s
>
> "mangled named = illegal"
> =========================
>
> $ time ./bin/smbclient -U slow%x -m smb3 //localhost/share -c "ls dir\*" > /dev/null
> Domain=[SLOWSERVER] OS=[] Server=[]
>
> real 0m0.352s
> user 0m0.063s
> sys 0m0.031s
>
> What do you think?
I *really* like this. Will try and get time to review tomorrow !
Thanks,
Jeremy.
> From f4767e1579fbb593971bbbb118495be108cde887 Mon Sep 17 00:00:00 2001
> From: Ralph Boehme <slow at samba.org>
> Date: Thu, 17 Nov 2016 14:22:41 +0100
> Subject: [PATCH 1/3] s3/smbd: convert "mangled names" option to an enum
>
> This is in preparation of adding an additional setting for this
> option. No change in behaviour by this commit, that comes in the next
> one.
>
> Signed-off-by: Ralph Boehme <slow at samba.org>
> ---
> lib/param/loadparm.h | 3 +++
> lib/param/param_table.c | 10 ++++++++++
> source3/param/loadparm.c | 2 +-
> source3/smbd/mangle.c | 4 ++--
> source3/smbd/trans2.c | 12 ++++++++++--
> 5 files changed, 26 insertions(+), 5 deletions(-)
>
> diff --git a/lib/param/loadparm.h b/lib/param/loadparm.h
> index f9fb7d8..2cd5cca 100644
> --- a/lib/param/loadparm.h
> +++ b/lib/param/loadparm.h
> @@ -236,6 +236,9 @@ enum inheritowner_options {
> INHERIT_OWNER_UNIX_ONLY
> };
>
> +/* mangled names options */
> +enum mangled_names_options {MANGLED_NAMES_NO, MANGLED_NAMES_YES};
> +
> /*
> * Default passwd chat script.
> */
> diff --git a/lib/param/param_table.c b/lib/param/param_table.c
> index 4b5234a..8eb791a 100644
> --- a/lib/param/param_table.c
> +++ b/lib/param/param_table.c
> @@ -315,6 +315,16 @@ static const struct enum_list enum_inherit_owner_vals[] = {
> {INHERIT_OWNER_UNIX_ONLY, "unix only"},
> {-1, NULL}};
>
> +static const struct enum_list enum_mangled_names[] = {
> + {MANGLED_NAMES_NO, "no"},
> + {MANGLED_NAMES_NO, "false"},
> + {MANGLED_NAMES_NO, "0"},
> + {MANGLED_NAMES_YES, "yes"},
> + {MANGLED_NAMES_YES, "true"},
> + {MANGLED_NAMES_YES, "1"},
> + {-1, NULL}
> +};
> +
> /* Note: We do not initialise the defaults union - it is not allowed in ANSI C
> *
> * NOTE: Handling of duplicated (synonym) parameters:
> diff --git a/source3/param/loadparm.c b/source3/param/loadparm.c
> index d8da749..ac9ba49 100644
> --- a/source3/param/loadparm.c
> +++ b/source3/param/loadparm.c
> @@ -201,7 +201,7 @@ static struct loadparm_service sDefault =
> .oplocks = true,
> .kernel_oplocks = false,
> .level2_oplocks = true,
> - .mangled_names = true,
> + .mangled_names = MANGLED_NAMES_YES,
> .wide_links = false,
> .follow_symlinks = true,
> .sync_always = false,
> diff --git a/source3/smbd/mangle.c b/source3/smbd/mangle.c
> index a8988f0..3649bf7 100644
> --- a/source3/smbd/mangle.c
> +++ b/source3/smbd/mangle.c
> @@ -104,7 +104,7 @@ bool mangle_is_8_3_wildcards(const char *fname, bool check_case,
> bool mangle_must_mangle(const char *fname,
> const struct share_params *p)
> {
> - if (!lp_mangled_names(p)) {
> + if (lp_mangled_names(p) == MANGLED_NAMES_NO) {
> return False;
> }
> return mangle_fns->must_mangle(fname, p);
> @@ -140,7 +140,7 @@ bool name_to_8_3(const char *in,
>
> /* name mangling can be disabled for speed, in which case
> we just truncate the string */
> - if (!lp_mangled_names(p)) {
> + if (lp_mangled_names(p) == MANGLED_NAMES_NO) {
> strlcpy(out, in, 13);
> return True;
> }
> diff --git a/source3/smbd/trans2.c b/source3/smbd/trans2.c
> index 6999b2d..6fe3f92 100644
> --- a/source3/smbd/trans2.c
> +++ b/source3/smbd/trans2.c
> @@ -2440,11 +2440,17 @@ NTSTATUS smbd_dirptr_lanman2_entry(TALLOC_CTX *ctx,
> bool ok;
> uint64_t last_entry_off = 0;
> NTSTATUS status;
> + enum mangled_names_options mangled_names;
> + bool marshall_with_83_names;
> +
> + mangled_names = lp_mangled_names(conn->params);
>
> ZERO_STRUCT(state);
> state.conn = conn;
> state.info_level = info_level;
> - state.check_mangled_names = lp_mangled_names(conn->params);
> + if (mangled_names != MANGLED_NAMES_NO) {
> + state.check_mangled_names = true;
> + }
> state.has_wild = dptr_has_wild(dirptr);
> state.got_exact_match = false;
>
> @@ -2480,12 +2486,14 @@ NTSTATUS smbd_dirptr_lanman2_entry(TALLOC_CTX *ctx,
>
> *got_exact_match = state.got_exact_match;
>
> + marshall_with_83_names = (mangled_names == MANGLED_NAMES_YES);
> +
> status = smbd_marshall_dir_entry(ctx,
> conn,
> flags2,
> info_level,
> name_list,
> - state.check_mangled_names,
> + marshall_with_83_names,
> requires_resume_key,
> mode,
> fname,
> --
> 2.7.4
>
>
> From c36e93243627a804b3dcecd1475eac51f780ea2b Mon Sep 17 00:00:00 2001
> From: Ralph Boehme <slow at samba.org>
> Date: Thu, 17 Nov 2016 14:24:07 +0100
> Subject: [PATCH 2/3] s3/smbd: new "mangled names" setting "illegal"
>
> This does mangling for names with illegal NTFS characters, but not for
> names longer then 8.3:
>
> Name mangling with mangled named = yes
> ======================================
>
> Mangled | Short | Name
> ----------------------------
> | | foo
> | yes | 123456789
> yes | | foo:bar
>
> Name mangling with mangled named = illegal
> ==========================================
>
> Mangled | Short | Name
> ----------------------------
> | | foo
> | | 123456789
> yes | | foo:bar
>
> Setting "mangled names = illegal" is the most sensible setting for
> modern clients that don't use the shortname anymore.
>
> Signed-off-by: Ralph Boehme <slow at samba.org>
> ---
> docs-xml/smbdotconf/filename/manglednames.xml | 20 +++++++++++++++++++-
> lib/param/loadparm.h | 2 +-
> lib/param/param_table.c | 1 +
> 3 files changed, 21 insertions(+), 2 deletions(-)
>
> diff --git a/docs-xml/smbdotconf/filename/manglednames.xml b/docs-xml/smbdotconf/filename/manglednames.xml
> index bd5d97f..972834e 100644
> --- a/docs-xml/smbdotconf/filename/manglednames.xml
> +++ b/docs-xml/smbdotconf/filename/manglednames.xml
> @@ -1,5 +1,6 @@
> <samba:parameter name="mangled names"
> - type="boolean"
> + type="enum"
> + enumlist="enum_mangled_names"
> context="S"
> parm="1"
> xmlns:samba="http://www.samba.org/samba/DTD/samba-doc">
> @@ -11,6 +12,22 @@
> <para>See the section on <smbconfoption name="name mangling"/> for
> details on how to control the mangling process.</para>
>
> + <para>Possible option settings are</para>
> +
> + <itemizedlist>
> + <listitem><para><emphasis>yes (default)</emphasis> -
> + enables name mangling for all not DOS 8.3 conforming
> + names.</para></listitem>
> +
> + <listitem><para><emphasis>no</emphasis> - disables any
> + name mangling.</para></listitem>
> +
> + <listitem><para><emphasis>illegal</emphasis> - does
> + mangling for names with illegal NTFS characters. This
> + is the most sensible setting for modern clients that
> + don't use the shortname anymore.</para></listitem>
> + </itemizedlist>
> +
> <para>If mangling is used then the mangling method is as follows:</para>
>
> <itemizedlist>
> @@ -56,4 +73,5 @@
> do not change between sessions.</para>
> </description>
> <value type="default">yes</value>
> +<value type="example">illegal</value>
> </samba:parameter>
> diff --git a/lib/param/loadparm.h b/lib/param/loadparm.h
> index 2cd5cca..6d01b37 100644
> --- a/lib/param/loadparm.h
> +++ b/lib/param/loadparm.h
> @@ -237,7 +237,7 @@ enum inheritowner_options {
> };
>
> /* mangled names options */
> -enum mangled_names_options {MANGLED_NAMES_NO, MANGLED_NAMES_YES};
> +enum mangled_names_options {MANGLED_NAMES_NO, MANGLED_NAMES_YES, MANGLED_NAMES_ILLEGAL};
>
> /*
> * Default passwd chat script.
> diff --git a/lib/param/param_table.c b/lib/param/param_table.c
> index 8eb791a..95c3b8c 100644
> --- a/lib/param/param_table.c
> +++ b/lib/param/param_table.c
> @@ -319,6 +319,7 @@ static const struct enum_list enum_mangled_names[] = {
> {MANGLED_NAMES_NO, "no"},
> {MANGLED_NAMES_NO, "false"},
> {MANGLED_NAMES_NO, "0"},
> + {MANGLED_NAMES_ILLEGAL, "illegal"},
> {MANGLED_NAMES_YES, "yes"},
> {MANGLED_NAMES_YES, "true"},
> {MANGLED_NAMES_YES, "1"},
> --
> 2.7.4
>
>
> From d3714c79dee8a406f91b9af2d9f84db608cfa49b Mon Sep 17 00:00:00 2001
> From: Ralph Boehme <slow at samba.org>
> Date: Thu, 15 Dec 2016 13:05:50 +0100
> Subject: [PATCH 3/3] s3/torture: add a test for "mangled names = invalid"
>
> This checks both that illegal NTFS names are still mangled and that long
> names have no shortname.
>
> Signed-off-by: Ralph Boehme <slow at samba.org>
> ---
> selftest/target/Samba3.pm | 4 ++
> source3/selftest/tests.py | 5 ++
> source3/torture/torture.c | 147 ++++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 156 insertions(+)
>
> diff --git a/selftest/target/Samba3.pm b/selftest/target/Samba3.pm
> index 9013652..1d77c97 100755
> --- a/selftest/target/Samba3.pm
> +++ b/selftest/target/Samba3.pm
> @@ -1830,6 +1830,10 @@ sub provision($$$$$$$$)
> copy = tmp
> acl_xattr:ignore system acls = yes
> acl_xattr:default acl style = windows
> +
> +[mangle_illegal]
> + copy = tmp
> + mangled names = illegal
> ";
> close(CONF);
>
> diff --git a/source3/selftest/tests.py b/source3/selftest/tests.py
> index d9d32cc..2feb5c8 100755
> --- a/source3/selftest/tests.py
> +++ b/source3/selftest/tests.py
> @@ -79,6 +79,11 @@ tests = ["OPLOCK-CANCEL"]
> for t in tests:
> plantestsuite("samba3.smbtorture_s3.plain(nt4_dc).%s" % t, "nt4_dc", [os.path.join(samba3srcdir, "script/tests/test_smbtorture_s3.sh"), t, '//$SERVER_IP/tmp', '$USERNAME', '$PASSWORD', smbtorture3, "", "-l $LOCAL_PATH"])
>
> +env = "nt4_dc"
> +tests = ["MANGLE-ILLEGAL"]
> +for t in tests:
> + plantestsuite("samba3.smbtorture_s3.plain(%s).%s" % (env, t), env, [os.path.join(samba3srcdir, "script/tests/test_smbtorture_s3.sh"), t, '//$SERVER_IP/mangle_illegal', '$USERNAME', '$PASSWORD', smbtorture3, "", "-l $LOCAL_PATH"])
> +
> tests = ["RW1", "RW2", "RW3"]
> for t in tests:
> plantestsuite("samba3.smbtorture_s3.vfs_aio_fork(simpleserver).%s" % t, "simpleserver", [os.path.join(samba3srcdir, "script/tests/test_smbtorture_s3.sh"), t, '//$SERVER_IP/vfs_aio_fork', '$USERNAME', '$PASSWORD', smbtorture3, "", "-l $LOCAL_PATH"])
> diff --git a/source3/torture/torture.c b/source3/torture/torture.c
> index 8f0f136..4554bc4 100644
> --- a/source3/torture/torture.c
> +++ b/source3/torture/torture.c
> @@ -8448,6 +8448,152 @@ static bool run_mangle1(int dummy)
> return true;
> }
>
> +static NTSTATUS mangle_illegal_list_shortname_fn(const char *mntpoint,
> + struct file_info *f,
> + const char *mask,
> + void *state)
> +{
> + if (f->short_name == NULL) {
> + return NT_STATUS_OK;
> + }
> +
> + if (strlen(f->short_name) == 0) {
> + return NT_STATUS_OK;
> + }
> +
> + printf("unexpected shortname: %s\n", f->short_name);
> +
> + return NT_STATUS_OBJECT_NAME_INVALID;
> +}
> +
> +static NTSTATUS mangle_illegal_list_name_fn(const char *mntpoint,
> + struct file_info *f,
> + const char *mask,
> + void *state)
> +{
> + char *name = state;
> +
> + printf("name: %s\n", f->name);
> + fstrcpy(name, f->name);
> + return NT_STATUS_OK;
> +}
> +
> +static bool run_mangle_illegal(int dummy)
> +{
> + struct cli_state *cli = NULL;
> + struct cli_state *cli_posix = NULL;
> + const char *fname = "\\MANGLE_ILLEGAL\\this_is_a_long_fname_to_be_mangled.txt";
> + const char *illegal_fname = "MANGLE_ILLEGAL/foo:bar";
> + char *mangled_path = NULL;
> + uint16_t fnum;
> + fstring name;
> + fstring alt_name;
> + NTSTATUS status;
> +
> + printf("starting mangle-illegal test\n");
> +
> + if (!torture_open_connection(&cli, 0)) {
> + return False;
> + }
> +
> + smbXcli_conn_set_sockopt(cli->conn, sockops);
> +
> + if (!torture_open_connection(&cli_posix, 0)) {
> + return false;
> + }
> +
> + smbXcli_conn_set_sockopt(cli_posix->conn, sockops);
> +
> + status = torture_setup_unix_extensions(cli_posix);
> + if (!NT_STATUS_IS_OK(status)) {
> + return false;
> + }
> +
> + cli_rmdir(cli, "\\MANGLE_ILLEGAL");
> + status = cli_mkdir(cli, "\\MANGLE_ILLEGAL");
> + if (!NT_STATUS_IS_OK(status)) {
> + printf("mkdir1 failed : %s\n", nt_errstr(status));
> + return False;
> + }
> +
> + /*
> + * Create a file with illegal NTFS characters and test that we
> + * get a usable mangled name
> + */
> +
> + cli_setatr(cli_posix, illegal_fname, 0, 0);
> + cli_posix_unlink(cli_posix, illegal_fname);
> +
> + status = cli_posix_open(cli_posix, illegal_fname, O_RDWR|O_CREAT|O_EXCL,
> + 0600, &fnum);
> + if (!NT_STATUS_IS_OK(status)) {
> + printf("POSIX create of %s failed (%s)\n",
> + illegal_fname, nt_errstr(status));
> + return false;
> + }
> +
> + status = cli_close(cli_posix, fnum);
> + if (!NT_STATUS_IS_OK(status)) {
> + printf("close failed (%s)\n", nt_errstr(status));
> + return false;
> + }
> +
> + status = cli_list(cli, "\\MANGLE_ILLEGAL\\*", 0, mangle_illegal_list_name_fn, &name);
> + if (!NT_STATUS_IS_OK(status)) {
> + d_printf("cli_list failed: %s\n", nt_errstr(status));
> + return false;
> + }
> +
> + mangled_path = talloc_asprintf(talloc_tos(), "\\MANGLE_ILLEGAL\\%s", name);
> + if (mangled_path == NULL) {
> + return false;
> + }
> +
> + status = cli_openx(cli, mangled_path, O_RDONLY, DENY_NONE, &fnum);
> + if (!NT_STATUS_IS_OK(status)) {
> + d_printf("cli_openx(%s) failed: %s\n", mangled_path, nt_errstr(status));
> + TALLOC_FREE(mangled_path);
> + return false;
> + }
> + TALLOC_FREE(mangled_path);
> + cli_close(cli, fnum);
> +
> + cli_setatr(cli_posix, illegal_fname, 0, 0);
> + cli_posix_unlink(cli_posix, illegal_fname);
> +
> + /*
> + * Create a file with a long name and check that we got *no* short name.
> + */
> +
> + status = cli_ntcreate(cli, fname, 0, GENERIC_ALL_ACCESS|DELETE_ACCESS,
> + FILE_ATTRIBUTE_NORMAL, 0, FILE_OVERWRITE_IF,
> + 0, 0, &fnum, NULL);
> + if (!NT_STATUS_IS_OK(status)) {
> + d_printf("open %s failed: %s\n", fname, nt_errstr(status));
> + return false;
> + }
> + cli_close(cli, fnum);
> +
> + status = cli_list(cli, fname, 0, mangle_illegal_list_shortname_fn, &alt_name);
> + if (!NT_STATUS_IS_OK(status)) {
> + d_printf("cli_list failed\n");
> + return false;
> + }
> +
> + cli_unlink(cli, fname, 0);
> + cli_rmdir(cli, "\\MANGLE_ILLEGAL");
> +
> + if (!torture_close_connection(cli_posix)) {
> + return false;
> + }
> +
> + if (!torture_close_connection(cli)) {
> + return false;
> + }
> +
> + return true;
> +}
> +
> static size_t null_source(uint8_t *buf, size_t n, void *priv)
> {
> size_t *to_pull = (size_t *)priv;
> @@ -11042,6 +11188,7 @@ static struct {
> {"PROPERTIES", run_properties, 0},
> {"MANGLE", torture_mangle, 0},
> {"MANGLE1", run_mangle1, 0},
> + {"MANGLE-ILLEGAL", run_mangle_illegal, 0},
> {"W2K", run_w2ktest, 0},
> {"TRANS2SCAN", torture_trans2_scan, 0},
> {"NTTRANSSCAN", torture_nttrans_scan, 0},
> --
> 2.7.4
>
More information about the samba-technical
mailing list