Skip to content

SMB config for TimeMachine has typo #2952 #2951

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Feb 26, 2025
Merged

SMB config for TimeMachine has typo #2952 #2951

merged 1 commit into from
Feb 26, 2025

Conversation

@simon-77
Copy link
Contributor

@simon-77 simon-77 commented Feb 13, 2025 •
edited by phillxnet
Loading

There was a typo in the SMB config for time machine export:
The correct option is fruit:time machine

Fixes #2952

Dokumentation of SMB:
https://www.samba.org/samba/docs/current/man-html/vfs_fruit.8.html

Cheers
Simon

@phillxnet
Copy link
Member

phillxnet commented Feb 13, 2025

@simon-77 Nice find, and thanks for the fix.

Can you create a partner issue that this PR links to thought. We need it for our surfacing of changes within the Web-UI.
We can then hopefully get this one in ready for the pending Stable release.

This PR can then have a "Fixes #####" in and we have what is required for our GitHub auto changelog and our consequent rpm/Web-UI changelog.

@simon-77 simon-77 changed the title fix: typo in SMB TimeMachine config fix #2952 : typo in SMB TimeMachine config Feb 13, 2025
@simon-77 simon-77 changed the title fix #2952 : typo in SMB TimeMachine config Fixes #2952 typo in SMB TimeMachine config Feb 13, 2025
@simon-77
Copy link
Contributor Author

simon-77 commented Feb 13, 2025 •
edited
Loading

I have created the issue here:
#2952

But I don't know (or don't have the permissions to) link the two.

Changing the title of this PR did not do the trick ...

@FroggyFlox
Copy link
Member

FroggyFlox commented Feb 13, 2025

Changing the title of this PR did not do the trick ...

Unfortunately, I believe GitHub's automatic detection only works when the "Fixes ######" is in the main text, not the title.

Nice find indeed... That mistake must have been mine when I put that in. I wonder why it worked before, though. Do you know if they changed the spelling? I would be surprised if they did, though.

@simon-77
Copy link
Contributor Author

simon-77 commented Feb 13, 2025

I wonder why it worked before, though.

I currently have some issues setting up Time Machine on my MacBook to an SMB share. Sometimes it does work quiet well, sometimes it doesn't. Therefore I red some articles about the SMB config for TimeMachine, noticed the spelling mistake & changed it.
Honestly, I don't notice any difference ... but it still should be corrected.

@phillxnet phillxnet changed the title Fixes #2952 typo in SMB TimeMachine config SMB config for TimeMachine has typo #2952 Feb 13, 2025
@phillxnet
Copy link
Member

phillxnet commented Feb 13, 2025 •
edited
Loading

@simon-77 I've just tweaked the issue and pull request titles to be in-line which what we know works with our Changelog.
I've also popped in the Fixes line in the original text.

Our changelog parsing may well have been thrown as-was and is a little limited in its capabilities.
Hopefully this PR title should now look like previously closed one that presented OK within the Web-UI.

phillxnet
phillxnet approved these changes Feb 26, 2025
View reviewed changes
Copy link
Member

@phillxnet phillxnet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@simon-77 Nice find, and thanks for submitting this fix.

Before this change our time-machine tick in SMB share config resulted in the following Samaba configurations:

rleap15-6:~ # testparm 
Load smb config files from /etc/samba/smb.conf
Loaded services file OK.
Weak crypto is allowed by GnuTLS (e.g. NTLM as a compatibility fallback)

Server role: ROLE_STANDALONE

Press enter to see a dump of your service definitions

# Global parameters
[global]
        load printers = No
        log file = /var/log/samba/log.%m
        map to guest = Bad User
        printcap name = /dev/null
        idmap config * : backend = tdb
        cups options = raw


[smb-tm-share]
        admin users = radmin
        comment = b'Samba-Export'
        path = /mnt2/smb-tm-share
        read only = No
        root preexec = sh -c "cd /opt/rockstor/ && poetry run mnt-share smb-tm-share"
        root preexec close = Yes
        vfs objects = catia fruit streams_xattr
        fruit:resource = file
        fruit:locking = none
        fruit:encoding = private
        fruit:delete_empty_adfiles = yes
        fruit:wipe_intentionally_left_blank_rfork = yes
        fruit:posix_rename = no
        fruit:veto_appledouble = no
        fruit:metadata = stream
        fruit:timemachine = yes

So our prior fruit:timemachine = yes looks to be accepted/acceptable.

From an rpm built and installed on Leap 15.6 from this pull requests branch, the same Time Machine tick resulting in:

rleap15-6:~ # testparm 
Load smb config files from /etc/samba/smb.conf
Loaded services file OK.
Weak crypto is allowed by GnuTLS (e.g. NTLM as a compatibility fallback)

Server role: ROLE_STANDALONE

Press enter to see a dump of your service definitions

# Global parameters
[global]
        load printers = No
        log file = /var/log/samba/log.%m
        map to guest = Bad User
        printcap name = /dev/null
        idmap config * : backend = tdb
        cups options = raw


[smb-tm-share]
        comment = b'Samba-Export'
        path = /mnt2/smb-tm-share
        read only = No
        root preexec = sh -c "cd /opt/rockstor/ && poetry run mnt-share smb-tm-share"
        root preexec close = Yes
        vfs objects = catia fruit streams_xattr
        fruit:resource = file
        fruit:locking = none
        fruit:encoding = private
        fruit:delete_empty_adfiles = yes
        fruit:wipe_intentionally_left_blank_rfork = yes
        fruit:posix_rename = no
        fruit:veto_appledouble = no
        fruit:metadata = stream
        fruit:time machine = yes

Again no indication of a non valid config, and we have the doc-consistent:

  • fruit:time machine = yes

So this change looks to be legitimate, but I'm unsure if it actually makes any changes.
@FroggyFlox what is your opinion on this change? As per @simon-77 's comment, it is consistent with the sited doc references. Maybe we are fending off a future deprecated form here!

The following has mention of both variants: https://developer.apple.com/forums/thread/666293

Approving and will merge if no objections soon.

@FroggyFlox
Copy link
Member

FroggyFlox commented Feb 26, 2025

I agree... @simon-77's change is consistent with upstream docs so we should use that.

@phillxnet
Copy link
Member

phillxnet commented Feb 26, 2025

I've also, since review, found 2018 & 2021 upstream bugreports containing the space as their config, with no comment on that config entry:

We also have, from our original 2018 issue for TM over SMB from #1910 the following upstream reference in a comment of mine: #1910 (comment)

That bug report is from 2016/2017. So this looks like testparm either doesn't 'sense' this anomaly or it isn't an anomaly. But what-ever; this was missed in my review of the code. We should look to what exactly testparm covers as we do use it within our own code to review configurations for their validity.

rockstor-core/src/rockstor/system/samba.py

Lines 42 to 47 in cfffde7

def test_parm(config="/etc/samba/smb.conf"):
cmd = [TESTPARM, "-s", config]
o, e, rc = run_command(cmd, throw=False)
if rc != 0:
raise Exception("Syntax error while checking the temporary samba config file")
return True

which is used during samba config refresh:

rockstor-core/src/rockstor/system/samba.py

Line 95 in cfffde7

def refresh_smb_config(exports):

and when writing out the global config:

rockstor-core/src/rockstor/system/samba.py

Lines 112 to 113 in cfffde7

# write out new [global] section and re-write the existing rockstor section.
def update_global_config(smb_config=None, ad_config=None):

@phillxnet phillxnet merged commit 9994322 into rockstor:testing Feb 26, 2025
@phillxnet phillxnet mentioned this pull request Feb 26, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@phillxnet phillxnet phillxnet approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@simon-77 @phillxnet @FroggyFlox