[PATCH] smbd: Fix snapshot query on shares with DFS enabled
Uri Simchoni
uri at samba.org
Wed Aug 17 07:12:45 UTC 2016
On 08/17/2016 01:29 AM, Jeremy Allison wrote:
> On Tue, Aug 16, 2016 at 03:24:48PM -0700, Jeremy Allison wrote:
>> On Tue, Aug 16, 2016 at 08:40:15AM -0700, Jeremy Allison wrote:
>>> On Tue, Aug 16, 2016 at 08:05:55AM +0300, Uri Simchoni wrote:
>>>> On 08/16/2016 02:44 AM, Jeremy Allison wrote:
>>>>>>
>>>>>> I will try to take a closer look tomorrow, maybe that can be easily
>>>>>> handled in the shadowcopy2 module. The backport should probably wait
>>>>>> until this is resolved
>>>>>
>>>>> We should probably add a smbclient-based torture test
>>>>> for both file and directory inside source3/script/tests/test_shadow_copy.sh
>>>>> once this gets fixed to make sure we don't regress.
>>>>>
>>>> Looking again at the shadow_copy2 test, it's a great step forward vs no
>>>> test, but it is lacking in some areas.
>>>>
>>>> The current test list previous versions of a file, which queries
>>>> snapshot file information.
>>>> a. Not sure it's complete enough - maybe successfully reading the file
>>>> will be safer
>>>> b. It does not handle directories at all
>>>> c. It only runs on SMB1, and with respect to paths, SMB2 may behave
>>>> differently. Strangely, the test seems to pass on SMB3 (adding -m SMB3),
>>>> although AFAICT smbclient uses SMB1 calls to list snapshots (so it
>>>> should have yielded 0 snapshots). I'll have to look into it.
>>>>
>>>> The SMB3 test must, IMHO, access snapshot files using TWrp create
>>>> context (I don't mind doing it once I have time and it shouldn't stop
>>>> any bugfixes if it's been reasonably verified that they don't break
>>>> things, so it's a note to self).
>>>
>>> For the current change, it would be enough to test over SMB2 using
>>> path with /@GMT-YYYY... appended to the end, as that's exactly
>>> what the server now does.
>>>
>>> Further tests can add access to the Twrp context, but I don't
>>> think they're a blocker for adding tests (one for file, one
>>> for directory) for this specific case.
>>
>> OK, I now have working SMB2 FSCTL_GET_SHADOW_COPY_DATA client
>> code, but for SMB2 it seems that Twrp contexts are required
>> to get at the shadow copies on Windows (I can list 'em, but
>> doing path/@GMT-YYYY etc. fails with OBJECT_PATH_NOT_FOUND).
>>
>> Shall I add code into the SMB2 client open that converts
>> a /@GMT-YYYY... trailing path into a Twrp token ?
>>
>> What do you think ?
>
> FYI. Here is the libsmb client code that plumbs SMB2
> FSCTL_GET_SHADOW_COPY_DATA into cli_shadow_copy_data()
> if you want to play with it.
>
> Jeremy.
>
I've played with it some (haven't carefully reviewed it). Adding a patch
that runs shadow-copy2 tests using both SMB3 and NT1, and a test-patch
that puts the @GMT at the end of the path.
The SMB3 variant fails without your snapshot listing code, but passes
with it, no matter where the @GMT is, so I don't understand why you said
that path/@GMT-xxx fails (tested both path\@GMT and path/@GMT).
IMHO smbclient should place a TWrp in its allinfo query - that's what's
going to work against any SMB2 server. Perhaps we can also add smbclient
functionality to access previous versions (somewhat like cd - smbclient
keeps the dir locally and prepends it to paths. In the same way the TWrp
can be added to any open).
For the time being, I think we should apply your shadow_copy_data()
patches and not my patch that puts the @GMT at the end. That would make
"allinfo" properly list previous versions against all (reasonably
recent) versions of samba in SMB2 mode, at the expense of providing a
not-quite-realistic test for vfs_shadow_copy2.
Uri.
-------------- next part --------------
From f611432ad5ece9e564ab7a833a807ff12d695d71 Mon Sep 17 00:00:00 2001
From: Uri Simchoni <uri at samba.org>
Date: Tue, 16 Aug 2016 07:19:04 +0300
Subject: [PATCH 1/2] s2-selftest: run shadow_copy2 test both in NT1 and SMB3
modes
Signed-off-by: Uri Simchoni <uri at samba.org>
---
source3/script/tests/test_shadow_copy.sh | 4 ++--
source3/selftest/tests.py | 3 ++-
2 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/source3/script/tests/test_shadow_copy.sh b/source3/script/tests/test_shadow_copy.sh
index 45d9cf1..2fb1ef5 100755
--- a/source3/script/tests/test_shadow_copy.sh
+++ b/source3/script/tests/test_shadow_copy.sh
@@ -5,7 +5,7 @@
if [ $# -lt 7 ]; then
cat <<EOF
-Usage: test_shadow_copy SERVER SERVER_IP DOMAIN USERNAME PASSWORD WORKDIR SMBCLIENT
+Usage: test_shadow_copy SERVER SERVER_IP DOMAIN USERNAME PASSWORD WORKDIR SMBCLIENT PARAMS
EOF
exit 1;
fi
@@ -18,8 +18,8 @@ PASSWORD=${5}
WORKDIR=${6}
SMBCLIENT=${7}
shift 7
-SMBCLIENT="$VALGRIND ${SMBCLIENT}"
ADDARGS="$*"
+SMBCLIENT="$VALGRIND ${SMBCLIENT} ${ADDARGS}"
incdir=`dirname $0`/../../../testprogs/blackbox
. $incdir/subunit.sh
diff --git a/source3/selftest/tests.py b/source3/selftest/tests.py
index 0a0cb08..291aaa9 100755
--- a/source3/selftest/tests.py
+++ b/source3/selftest/tests.py
@@ -187,7 +187,8 @@ for env in ["fileserver"]:
plantestsuite("samba3.blackbox.dfree_quota (%s)" % env, env, [os.path.join(samba3srcdir, "script/tests/test_dfree_quota.sh"), '$SERVER', '$DOMAIN', '$USERNAME', '$PASSWORD', '$LOCAL_PATH', smbclient3, smbcquotas, smbcacls])
plantestsuite("samba3.blackbox.valid_users (%s)" % env, env, [os.path.join(samba3srcdir, "script/tests/test_valid_users.sh"), '$SERVER', '$SERVER_IP', '$DOMAIN', '$USERNAME', '$PASSWORD', '$PREFIX', smbclient3])
plantestsuite("samba3.blackbox.offline (%s)" % env, env, [os.path.join(samba3srcdir, "script/tests/test_offline.sh"), '$SERVER', '$SERVER_IP', '$DOMAIN', '$USERNAME', '$PASSWORD', '$LOCAL_PATH/offline', smbclient3])
- plantestsuite("samba3.blackbox.shadow_copy2 (%s)" % env, env, [os.path.join(samba3srcdir, "script/tests/test_shadow_copy.sh"), '$SERVER', '$SERVER_IP', '$DOMAIN', '$USERNAME', '$PASSWORD', '$LOCAL_PATH/shadow', smbclient3])
+ plantestsuite("samba3.blackbox.shadow_copy2 NT1 (%s)" % env, env, [os.path.join(samba3srcdir, "script/tests/test_shadow_copy.sh"), '$SERVER', '$SERVER_IP', '$DOMAIN', '$USERNAME', '$PASSWORD', '$LOCAL_PATH/shadow', smbclient3, '-m', 'NT1'])
+ plantestsuite("samba3.blackbox.shadow_copy2 SMB3 (%s)" % env, env, [os.path.join(samba3srcdir, "script/tests/test_shadow_copy.sh"), '$SERVER', '$SERVER_IP', '$DOMAIN', '$USERNAME', '$PASSWORD', '$LOCAL_PATH/shadow', smbclient3, '-m', 'SMB3'])
plantestsuite("samba3.blackbox.smbclient.forceuser_validusers (%s)" % env, env, [os.path.join(samba3srcdir, "script/tests/test_forceuser_validusers.sh"), '$SERVER', '$DOMAIN', '$USERNAME', '$PASSWORD', '$LOCAL_PATH', smbclient3])
plantestsuite("samba3.blackbox.smbget (%s)" % env, env, [os.path.join(samba3srcdir, "script/tests/test_smbget.sh"), '$SERVER', '$SERVER_IP', '$DOMAIN', 'smbget_user', '$PASSWORD', '$LOCAL_PATH/smbget', smbget])
plantestsuite("samba3.blackbox.netshareenum (%s)" % env, env, [os.path.join(samba3srcdir, "script/tests/test_shareenum.sh"), '$SERVER', '$USERNAME', '$PASSWORD', rpcclient])
--
2.5.5
From 50a0c4378c5b80b2ad5bdd8969be3a18c9981c38 Mon Sep 17 00:00:00 2001
From: Uri Simchoni <uri at samba.org>
Date: Wed, 17 Aug 2016 09:34:59 +0300
Subject: [PATCH 2/2] s3-smbclient: adjust 'allinfo' for self-testing
THIS IS A HACK - IMHO it doesn't belong in the code base
When doing 'allinfo', and retrieving info about previous
versions, put the snapshot label (@GMT-xxx) at the end of
the path if the connection is SMB2.
The proper way to do it is to use the TWrp create context.
Signed-off-by: Uri Simchoni <uri at samba.org>
---
source3/client/client.c | 15 +++++++++++++--
1 file changed, 13 insertions(+), 2 deletions(-)
diff --git a/source3/client/client.c b/source3/client/client.c
index 45dc11c..8ab73a2 100644
--- a/source3/client/client.c
+++ b/source3/client/client.c
@@ -1793,8 +1793,19 @@ static int do_allinfo(const char *name)
char *snap_name;
d_printf("%s\n", snapshots[i]);
- snap_name = talloc_asprintf(talloc_tos(), "%s%s",
- snapshots[i], name);
+
+ if (smbXcli_conn_protocol(cli->conn) >= PROTOCOL_SMB2_02) {
+ /* This is really a hack for unit-testing
+ * samba VFS modules. In the general case,
+ * a TWrp create context should be used.
+ */
+ snap_name = talloc_asprintf(talloc_tos(), "%s\\%s",
+ name, snapshots[i]);
+ } else {
+ snap_name = talloc_asprintf(talloc_tos(), "%s%s",
+ snapshots[i], name);
+ }
+
status = cli_qpathinfo3(cli, snap_name, &b_time, &a_time,
&m_time, &c_time, &size,
NULL, NULL);
--
2.5.5
More information about the samba-technical
mailing list