Running "samba-tool domain backup offline" hangs almost immidiately for me (since its introduction in Samba 4.10): #samba-tool domain backup offline --targetdir . running backup on dirs: /var/db/samba4/private /var/db/samba4 /usr/local/etc Starting transaction on /var/db/samba4/private/secrets (...) What really hangs is a subprocess that samba-tool starts: /usr/local/bin/tdbbackup -s .copy.tdb /var/db/samba4/private/secrets.ldb -r The problem seems to lie in python/samba/tdb_util.py, where we find: tdbbackup_cmd = [toolpath, "-s", ".copy.tdb", file1] if readonly: tdbbackup_cmd.append("-r") Since readonly is required (otherwise the afore mentioned hanging happens), -r is added, resulting in the above command line. However, tdbbackup uses getopt to parse its arguments and getopt stops at the first token that doesn't start with -, i.e. "/var/db/samba4/private/secrets.ldb", so "-r" is never considered. tdbbackup then tries to open the databases read/write and hangs since Samba is using them. It works if I change the above code (in python/samba/tdb_util.py) to something like: if readonly: tdbbackup_cmd = [toolpath, "-r", "-s", ".copy.tdb", file1] else: tdbbackup_cmd = [toolpath, "-s", ".copy.tdb", file1]
Here is the reason why it does not fail on GNU/Linux: > By default, getopt() permutes the contents of argv as it scans, so that eventually all the > nonoptions are at the end. Two other modes are also implemented. If the first character of > optstring is '+' or the environment variable POSIXLY_CORRECT is set, then option processing > stops as soon as a nonoption argument is encountered. If the first character of optstring is > '-', then each nonoption argv-element is handled as if it were the argument of an option with > character code 1. (This is used by programs that were written to expect options and other > argv-elements in any order and that care about the ordering of the two.) The special argument > "--" forces an end of option-scanning regardless of the scanning mode. glibc is by default not POSIX compliant that is why it accepts the out of order args. This is a pure portability issue.
I have applied the patch downstream, but it has to be addressed upstream in a portable (POSIX compliant) way.
Thank you Andrea. do you want to write that as a patch?
(In reply to Douglas Bagnall from comment #3) https://github.com/freebsd/freebsd-ports/commit/1db23ed5ef53c509dd421db6ded1c2f528916361#diff-76f198c324707008df32690f8f8ca5404cd410680cf9f53f371668aa7bd810fa
Created attachment 18575 [details] Patch against master Here is a Git-formatted patch. Cherry-picking to previous branches is straight forward.
Created attachment 18576 [details] Patch against master
Comment on attachment 18576 [details] Patch against master The patch looks good! Can I ask for two more things: 1. add "Signed-off-by: Andrea Venturoli <ml@netfence.it>" at the bottom (`git commit --amend -s` will do it) 2. Read https://www.samba.org/samba/devel/copyright-policy.html and send in the declaration paperwork?
I have had a bit of a poke around to see if we do this in other places, but I haven't found anything. I think usually we're OK because a) we use lib/cmdline or python for parsing arguments, or b) we don't often call subprocesses from end-user tools, or c) we happen to get things in the right order.
(In reply to Douglas Bagnall from comment #7) I will amend the patch, I'd also would like to do this with my FreeBSD email address as an individual contributor. Does it make any difference?
Created attachment 18579 [details] Patch against master Updated patch.
CLA sent from michaelo AT FreeBSD.org
slow@ has confirmed my CLA today.
(In reply to Michael Osipov from comment #12) > slow@ has confirmed my CLA today. That's good, but I have got mixed up, sorry. We really need the CLA from Andrea, not you, to match the patch: > From: Andrea Venturoli <ml@netfence.it>
(In reply to Douglas Bagnall from comment #13) Sorry, what's a CLA? I'd be glad to see this patch committed (I'm already used and it was already committed downstream in FreeBSD).
(In reply to Douglas Bagnall from comment #13) Never mind, I will contribute as well, but left Andrea's notice because he's the author, not me.
(In reply to Andrea Venturoli from comment #14) It is a legal document that you provide you contribution under a certain license/conditions, see a few comments above. From your your mail account with the content provided and done.
(In reply to Michael Osipov from comment #16) CLA sent. Thanks.
Both CLAs have been provided. Is there anything we can do for this patch to land and being backported?
(In reply to Michael Osipov from comment #18) I'll push it through a gitlab CI pipeline, another team member will review it, we'll attach backport patches for the release branches here, and the release manager will apply those. The thing you can do is remind me if I forget, which so far you are doing well.
(In reply to Douglas Bagnall from comment #19) Thank you Douglas, appreciated!
This bug was referenced in samba master: 7e083a6b3a12933b79ef19ccbd4c13bfa0203498
Created attachment 18587 [details] patch for 4.20
Created attachment 18588 [details] patch for 4.21
Created attachment 18589 [details] patch for 4.22 The patches are all exactly the same, apart from the commit hashes.
Pushed to autobuild-v4-{22,21,20}-test.
This bug was referenced in samba v4-20-test: cabe5f89975ddd395abb7d77d89f18da9807762c
This bug was referenced in samba v4-21-test: 480f27625dd7eb8af499fe07e5f6d7c01d6bb60a
This bug was referenced in samba v4-22-test: a210c2b2bc53ebae5e0ee64eb51fcd117bd231ee
Closing out bug report. Thanks!
(In reply to Jule Anger from comment #29) Magic, thank you guys. I will try to provide more patches in the upcoming weeks and months.
This bug was referenced in samba v4-22-stable (Release samba-4.22.0): a210c2b2bc53ebae5e0ee64eb51fcd117bd231ee