Impressive. How did you find that one ?

(In reply to Jeremy Allison from comment #1)
I didn't, a customer did. :)

Damn, I really hate that module. Why do people use it :-(.

Looking at this, when the path isn't found by the initial stat("@GMT-TIME/complete/pathname") then it goes into the walk-the path lookup functions one component at a time.

So it starts with a lookup on "@GMT-TIME" without the rest of the path as context. As snap dirs can be anywhere, we don't know that the actual @GMT-TIME directory exists somewhere below this, so we just fail with NT_STATUS_OBJECT_PATH_NOT_FOUND.

We'll have to add case-seeking behavior into vfs_shadow_copy2 in order to fix this, so the initial stat of "@GMT-TIME/complete/pathname" always works even if the path on disk is "COMPLETE/.snapshots/"@GMT-TIME/pathname".

Only way to fix this I think.

Long term I think the snapdirseverywhere functionality needs to be split out into a separate vfs_snapshotseverywhere module and this insanity removed from shadow_copy2.

We can then clean up shadow_copy2 and possibly split it up further into modules that provide the different functionalities currently munged (badly) into the one module.

The reason:

get @GMT-2001.10.30-12.34.56/dir/FILE file

works is that the .shapshots directory is just above the last component, so we run into
the optimization for creating a new file (even though we're not).




 703                 if (errno == ENOENT) {
 704                         /* Optimization when creating a new file - only
 705                            the last component doesn't exist.
 706                            NOTE : check_parent_exists() doesn't preserve errno.
 707                         */
 708                         int saved_errno = errno;
 709                         status = check_parent_exists(ctx,
 710                                                 conn,
 711                                                 posix_pathnames,
 712                                                 smb_fname,
 713                                                 &dirpath,
 714                                                 &start);
 715                         errno = saved_errno;
 716                         if (!NT_STATUS_IS_OK(status)) {
 717                                 goto fail;
 718                         }
 719                 }

Note this will set start to the character after @GMT-2001.10.30-12.34.56/dir/, meaning start=FILE. We then go directly into get_real_filename() which will do the case-insensitive lookup and find "file".

The problem occurs when the .snapshots directory is elsewhere in the path.

(In reply to Jeremy Allison from comment #4)
Yup, I came to the same conclusion. I'll add a test, stay tuned...

I *think* I have a way to make the

@@ -908,8 +908,35 @@ static char *shadow_copy2_do_convert(TALLOC_CTX *mem_ctx,
               /*
+                * name here is the passed in path from the
+                * client. It may not be case canonicalized.
+                */
+               NTSTATUS status;
+               struct smb_filename *case_canon_fname = NULL;
+               uint32_t ucf_flags = UCF_ALWAYS_ALLOW_WCARD_LCOMP |
+                               UCF_PREP_CREATEFILE;
+
+               status = filename_convert(mem_ctx,
+                               handle->conn,
+                               name,
+                               ucf_flags,
+                               NULL,
+                               NULL,
+                               &case_canon_fname);
+               if (!NT_STATUS_IS_OK(status)) {
+                       errno = map_errno_from_nt_status(status);
+                       goto fail;
+               }
+               path = talloc_asprintf(mem_ctx,
+                               "%s/%s",
+                               handle->conn->connectpath,
+                               case_canon_fname->base_name);

fix work for the @GMT....\BASE\FILE case. But it's
ugly. Let me see if I can get it working tomorrow.

Thinking some more, I really don't want to change
anything outside of vfs_shadow_copy2.c for this if
I can help it :-).

As explained offline, anything that uses filename_convert() on the @GMT stripped path can't be used, as it will fail if a parent directory containing the file has been removed (but is of course present in the snapshot), eg

a
└── b
    └── .snapshots
        └── @GMT-2001.10.30-12.34.56
            └── c
                └── file

What seems to work is pimping shadow_copy2_get_real_filename() to do the work for us. Have working WIP patch, I'll followup per mail once I have something.

(In reply to Ralph Böhme from comment #9)

Oh, the missing parent problem is subtle. I hadn't thought of that. I'd love to see the WIP patch also if you have time !

Just passed CI: https://gitlab.com/samba-team/samba/-/merge_requests/1295

(In reply to Ralph Böhme from comment #11)

Oh that's clever :-). I hadn't thought of that kind of fix. Congratulations. I'll do a full review when you're ready.

(In reply to Jeremy Allison from comment #12)
Will do. It seems in the end this version looks quite neat:

https://gitlab.com/samba-team/devel/samba/-/commit/01bd0373f3c4e89c2b86a8f7766c1c6e5c6ca5a6

I'll let you know when I'm done.

Created attachment 15963 [details]
Patch for 4.11 and 4.12 backported from master

Fwiw, this is basically the original version based on @GMT before I had to rewrite the patchset on-top of the twrp stuff. :)

Comment on attachment 15963 [details]
Patch for 4.11 and 4.12 backported from master

LGTM.

Reassigning to Karolin for inclusion in 4.12.next, 4.11.next.

(In reply to Jeremy Allison from comment #16)
Pushed to autobuild-v4-{12,11}-test.

(In reply to Karolin Seeger from comment #17)
Pushed to both branches.
Closing out bug report.

Thanks!