Skip to content
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

zfs: patch client path used in smb share #92378

Merged
merged 3 commits into from Jun 21, 2021
Merged

zfs: patch client path used in smb share #92378

merged 3 commits into from Jun 21, 2021

Conversation

jnetod
Copy link
Contributor

@jnetod jnetod commented Jul 5, 2020 •
edited

fix zfs smb share integration

# zfs-sharesmb.nix

{ pkgs, ... }:

let usersharePath = "/var/lib/samba/usershares";
in {
  services = {
    samba = {
      enable = true;
      extraConfig = ''
        registry shares = yes        
        usershare path = ${usersharePath}
        usershare max shares = 100
        usershare allow guests = yes
        usershare owner only = no
      '';
    };
  };

  systemd.services.samba-smbd.serviceConfig.ExecStartPre =
    "${pkgs.coreutils}/bin/mkdir -m +t -p ${usersharePath}";
}

# zfs set sharesmb=on tank/smb_share
# smbclient -NL localhost
# net usershare info

before:

# zfs share tank/smb_share
cannot share tank/smb_share: smb add share failed

now works as intended.

Motivation for this change
Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@SuperSandro2000
Copy link
Member

Please squash the commits together.

@SuperSandro2000
Copy link
Member

SuperSandro2000 commented Nov 29, 2020 •
edited

Please rebase this.

Auto-merging pkgs/os-specific/linux/zfs/default.nix
Automatic merge went well; stopped before committing as requested
error: --- ThrownError ----------------------------------------------------------------------------------------------------------------------------------------------------------------- nixdeepin was a work in progress and it has been canceled and removed https://github.com/NixOS/nixpkgs/issues/94870
(use '--show-trace' to show detailed location information)
nix --experimental-features nix-command eval --json --impure --expr (import /nix/store/lfdsijd3gkgsxdm3p8y5258zc151kqid-nixpkgs-review-2.5.0/lib/python3.8/site-packages/nixpkgs_review/nix/evalAttrs.nix /tmp/tmpl18bb84c) failed to run, /tmp/tmpl18bb84c was stored inspection
https://github.com/NixOS/nixpkgs/pull/92378 failed to build

@SuperSandro2000 SuperSandro2000 marked this pull request as draft November 29, 2020 01:34
@stale
Copy link

stale bot commented Jun 3, 2021

I marked this as stale due to inactivity. → More info

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jun 3, 2021
@jnetod
zfs: patch client path used in smb share
e16e517 
fix zfs smb share integration

before:

# zfs share tank/smb_share
cannot share tank/smb_share: smb add share failed

now works as intended.
@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jun 9, 2021
@jnetod jnetod marked this pull request as ready for review June 9, 2021 03:10
@Mic92
Copy link
Member

Mic92 commented Jun 9, 2021

Does this require a samba configuration? Do you have a setup to test this?

@jnetod
Copy link
Contributor Author

jnetod commented Jun 11, 2021

Does this require a samba configuration? Do you have a setup to test this?

Sure! I've updated my comment, check it out.

Mic92
Mic92 reviewed Jun 11, 2021
View reviewed changes
nixos/tests/zfs.nix Outdated Show resolved Hide resolved
@ofborg ofborg bot requested a review from Mic92 June 11, 2021 06:35
jnetod
jnetod commented Jun 18, 2021
View reviewed changes
nixos/tests/zfs.nix Outdated Show resolved Hide resolved
@Mic92 @jnetod
Update nixos/tests/zfs.nix
a4cb90b 
Co-authored-by: jnetod <49963580+jnetod@users.noreply.github.com>
@Mic92
Copy link
Member

Mic92 commented Jun 18, 2021

@GrahamcOfBorg test zfs.stable

@Mic92 Mic92 merged commit 7c2d156 into NixOS:master Jun 21, 2021
@Izorkin
Copy link
Contributor

Izorkin commented Apr 26, 2022

After that, PR significantly increased the size of the iso image. The image now contains the samba package, which is not needed there.
сс @jnetod @SuperSandro2000 @Mic92

@Izorkin
Copy link
Contributor

Izorkin commented Apr 26, 2022

Before PR:

nix path-info -Sh ./result
/nix/store/4il7r8fgh26brziawhcw0knj1h4ww1lc-zfs-user-2.1.4       278.6M

After PR:

nix path-info -Sh ./result
/nix/store/yw1by07874clir1jqlnvc9xz7k27q3vh-zfs-user-2.1.4       358.6M

@SuperSandro2000
Copy link
Member

If it really bothers you, please create a PR which moves samba support behind a boolean flag and disables it for the iso.

@Izorkin Izorkin mentioned this pull request Apr 26, 2022
Open
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jonringer jonringer Awaiting requested review from jonringer

@globin globin Awaiting requested review from globin

@hmenke hmenke Awaiting requested review from hmenke

@fpletz fpletz Awaiting requested review from fpletz

@wizeman wizeman Awaiting requested review from wizeman

@jcumming jcumming Awaiting requested review from jcumming

@Mic92 Mic92 Awaiting requested review from Mic92

Assignees
No one assigned
Labels
6.topic: nixos 10.rebuild-darwin: 1-10 10.rebuild-darwin: 1 10.rebuild-linux: 11-100 12. first-time contribution
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@jnetod @SuperSandro2000 @Mic92 @Izorkin @Janik-Haag