Change sharesmb to use REGISTRY shares for better control by user/admin #1476
Conversation
|
Sorry! I completely lost track of this! |
|
That's ok. No one seems to care or want SMBFS support anyway, so |
|
I am looking forward to the |
|
@FransUrbo Well here's something to think about. I've been wondering for a while now if the current libshare nfs/smb/iscsi approach is the right thing to do on Linux. The major reason we've done it this way is because that's what was available on Illumos. However, the Illumos/FreeBSD environment is really quite different than the Linux one. They have a single implementation of all these services and that implementation is under their direct control. That makes it far easier for them to integrate with it. Since we don't have that luxury we really need something more flexible and customizable. It occurred to me that thing might just be the new zfs event daemon (ZED). An initial implementation of the ZED is planned to be merged for the 0.6.3 tag. It will basically be able to consume zevents generated by the filesystem and execute arbitrary scripts. For things like nfs/smb/iscsi I can image the tools generating share/unshare events to be handled by the event daemon. For example, when a filesystem is mounted a share zevent is generated which includes the relevant properties for the dataset. The ZED receives the event and fork/execs a helper script which then checks the various properties and decides if it should be shared. This has the advantage of moving all bits involved in sharing something to an easily customizable script in user space. End users would then be empowered to be able to easily add support for their use case and we could ship canned script for the most common needs. The major wrinkle I see is that as originally conceived the ZED was entirely asynchronous. For things like unshare that may not be OK since we'll need to unshare it prior to unmounting it unless we make things like lazy unmounts the default. Anyway, I'd be interested in your thoughts on this! |
|
@behlendorf Something needs to be done to libshare on Linux, that's for sure (see #1484). If that means moving it to a separate or not, I'm not sure. I would need to look at ZED first, but it's asynchronousness doesn't make it sound like a good contender (i'm not to fond of lazy mounts/unmounts - sounds way to risky)... But getting libshare away from the filesystem seems to be the majorities opinion (all those who have said something negative about smbfs/iscsi support in ZoL have given this as their major reason/flaw). And I somewhat agree... In the sense that having that support in ONE command is the second major thing I love and attracted me to ZFS in the first place. That does not mean that it must be in, or near, it's core... Is the Illumos libshare completely unfit for a Linux port? @101100 Then you should really, REALLY, have taken that pull and run with it so that we (read: I) could have some more eyes on it before it's merged. It could also have meant that it got merged quicker.... Just say'n! |
|
@FransUrbo We basically have the vast majority of Illumos libshare now on Linux. It's just that on Illumos they have a single tightly integrated nfs, smb, and iscsi server which good stable programmatic interfaces. Where as under Linux there are three nfsd implementations, one smbd implementation, and three iscsi implementations. Most of which are still under some level of development, they may or may not be installed, and even if they are you've not guaranteed a particular version. So I think the key to doing something useful here is to make whatever we do uberflexible. So the question is if you could start from scratch what would you do? |
|
I honestly don't know... I kind'a like the current way. It's extremely simple - just add a new source file in lib/libshare and of you go with a new share type/implementation... My plan was to also add AFP support, but that stopped because of netatalks inability to accept simple 'add/remove/modify share' commands externally (i.e., it only accepts modifications on the config file and then requiring a restart/reload). I was even looking at adding that kind of support to netatalk, but I got sidetracked with other things... It's just that the libshare core needs reworked (because of the overhead). Yes, adding new options to an existing share type needs a little more work, but with the pull, I try to somewhat rectify that (with the nfs.c:foreach_nfs_shareopt() to libshare.c:foreach_shareopt() rename) - it makes parsing options much simpler. This might need more work to make it even simpler, but for my two share types, adding or removing an options is uber simple now. I haven't wanted to touch nfs.c (it's a little hard to read, maybe a touch up of that can simplify things?). Removing libshare al together sounds like a good base plan though - keep the filesystem stuff in the filesystem and add everything else to 'external software' (i.e. the ZED daemon or whatever). Maybe go through the design of that once more, taking stuff like this (sharing etc) into account? Do you have a git repo or something for ZED so I can have a look at it? |
|
Ah, I was late to the game. I wouldn't mind reviewing this code, but I don't have experience with the ZFS project, so I don't know the standards for the project. I piped up because of the mention that this was not important. I'm happy with ZFS but found the lack of guest access annoying for my purposes... @behlendorf |
|
@101100 Simply testing it for starters would be plenty... |
|
@101100 that is, do a pull/cherry-pick of the pull request, recompiling (don't forget ./autogen.sh !!) and reinstalling. Then just run it a few days and report what you find... That would go a very long way! |
|
I just did a rebase against master (and modified the last commit message). Maybe I should rebase it into one commit? |
|
@FransUrbo
|
Building from a git repo is quite different. So if you haven't done that before, it's probably better to just wait. It will hopefully be available sooner than later :). If you make debs from the git repos, I can't guarantee that just replacing the existing with the newly built works perfectly. I don't know if that have actually been tested. |
|
Rebase against latest master, including fixing C style stuff.... Also rebased as ONE commit. |
if /usr/bin/net doesn't exist or isn't executable. Just create a dummy...
Put in /usr/share/initramfs-tools/ or /etc/initramfs-tools/. + With pull request openzfs#1476 (not yet merged) comes a verbose warning if /usr/bin/net doesn't exist or isn't executable. Just create a dummy... + If user haven't specified pool, but bootfs then use the first part of the bootfs path as pool name. + Add support for booting of a ZFS snapshot. Do this by cloning the snapshot into a dataset. If this dataset already exists, destroy it. !! WARNING: Untested !!
Put in /usr/share/initramfs-tools/ or /etc/initramfs-tools/. + With pull request openzfs#1476 (not yet merged) comes a verbose warning if /usr/bin/net doesn't exist or isn't executable. Just create a dummy... + If user haven't specified pool, but bootfs then use the first part of the bootfs path as pool name. + Add support for booting of a ZFS snapshot. Do this by cloning the snapshot into a dataset. If this dataset already exists, destroy it. !! WARNING: Untested !!
Put in /usr/share/initramfs-tools/ or /etc/initramfs-tools/. + With pull request openzfs#1476 (not yet merged) comes a verbose warning if /usr/bin/net doesn't exist or isn't executable. Just create a dummy... + If user haven't specified pool, but bootfs then use the first part of the bootfs path as pool name. + Add support for booting of a ZFS snapshot. Do this by cloning the snapshot into a dataset. If this dataset already exists, destroy it. !! WARNING: Untested !!
* Put in /usr/share/initramfs-tools/ or /etc/initramfs-tools/. * With pull request openzfs#1476 (not yet merged) comes a verbose warning if /usr/bin/net doesn't exist or isn't executable. Just create a dummy... * If user haven't specified pool, but bootfs then use the first part of the bootfs path as pool name. * Add support for booting of a ZFS snapshot. Do this by cloning the snapshot into a dataset. If this the resulting dataset already exists, destroy it. Then set mountpoint=/ on that dataset and mount it on root.
* Put in /usr/share/initramfs-tools/ or /etc/initramfs-tools/. * With pull request openzfs#1476 (not yet merged) comes a verbose warning if /usr/bin/net doesn't exist or isn't executable. Just create a dummy... * If user haven't specified pool, but bootfs then use the first part of the bootfs path as pool name. * Add support for booting of a ZFS snapshot. Do this by cloning the snapshot into a dataset. If this the resulting dataset already exists, destroy it. Then set mountpoint=/ on that dataset and mount it on root.
* Put in /usr/share/initramfs-tools/ or /etc/initramfs-tools/. * With pull request openzfs#1476 (not yet merged) comes a verbose warning if /usr/bin/net doesn't exist or isn't executable. Just create a dummy... * If user haven't specified pool, but bootfs then use the first part of the bootfs path as pool name. * Add support for booting of a ZFS snapshot. Do this by cloning the snapshot into a dataset. If this the resulting dataset already exists, destroy it. Then set mountpoint=/ on that dataset and mount it on root. + If snapshot does not exist, use base dataset (the part before '@') as boot filesystem instead.
* Put in /usr/share/initramfs-tools/ or /etc/initramfs-tools/. * With pull request openzfs#1476 (not yet merged) comes a verbose warning if /usr/bin/net doesn't exist or isn't executable. Just create a dummy... * If user haven't specified pool, but bootfs then use the first part of the bootfs path as pool name. * Add support for booting of a ZFS snapshot. Do this by cloning the snapshot into a dataset. If this the resulting dataset already exists, destroy it. Then set mountpoint=/ on that dataset and mount it on root. + If snapshot does not exist, use base dataset (the part before '@') as boot filesystem instead. * Add support for more kernel command line arguments (ideas taken from the Fedora/Redhat dracut script): + (zfs_force|zfs.force|zfsforce)=(on|yes|1) (ignore case). + root=zfs:AUTO => try to find rootfs automatically. + root=zfs:<dataset> * Remove the existing '-f' option to 'zpool import', instead let this be controlled on the kernel command line. * Do not force-set mountpoint=/ - should (??) not be nessesary. * Support mounting a rootfs with mountpoint=legacy
* Put in /usr/share/initramfs-tools/ or /etc/initramfs-tools/. * With pull request openzfs#1476 (not yet merged) comes a verbose warning if /usr/bin/net doesn't exist or isn't executable. Just create a dummy... * If user haven't specified pool, but bootfs then use the first part of the bootfs path as pool name. * Add support for booting of a ZFS snapshot. Do this by cloning the snapshot into a dataset. If this the resulting dataset already exists, destroy it. Then set mountpoint=/ on that dataset and mount it on root. + If snapshot does not exist, use base dataset (the part before '@') as boot filesystem instead. * Add support for more kernel command line arguments (ideas taken from the Fedora/Redhat dracut script): + (zfs_force|zfs.force|zfsforce)=(on|yes|1) (ignore case). + root=zfs:AUTO => try to find rootfs automatically. + root=zfs:<dataset> * Remove the existing '-f' option to 'zpool import', instead let this be controlled on the kernel command line. * Do not force-set mountpoint=/ - should (??) not be nessesary. * Support mounting a rootfs with mountpoint=legacy * Support both RPM based and DEB based system by removing the logic from the dracut script and instead use the logic in scripts/zfs-initramfs/scripts/zfs for both Debian GNU/Linux (etc) and RedHat/Fedora (etc).
* Put in /usr/share/initramfs-tools/ or /etc/initramfs-tools/. * With pull request openzfs#1476 (not yet merged) comes a verbose warning if /usr/bin/net doesn't exist or isn't executable. Just create a dummy... * If user haven't specified pool, but bootfs then use the first part of the bootfs path as pool name. * Add support for booting of a ZFS snapshot. Do this by cloning the snapshot into a dataset. If this the resulting dataset already exists, destroy it. Then set mountpoint=/ on that dataset and mount it on root. + If snapshot does not exist, use base dataset (the part before '@') as boot filesystem instead. * Add support for more kernel command line arguments (ideas taken from the Fedora/Redhat dracut script): + (zfs_force|zfs.force|zfsforce)=(on|yes|1) (ignore case). + root=zfs:AUTO => try to find rootfs automatically. + root=zfs:<dataset> * Remove the existing '-f' option to 'zpool import', instead let this be controlled on the kernel command line. * Do not force-set mountpoint=/ - should (??) not be nessesary. * Support mounting a rootfs with mountpoint=legacy * Support both RPM based and DEB based system by removing the logic from the dracut script and instead use the logic in scripts/zfs-initramfs/scripts/zfs for both Debian GNU/Linux (etc) and RedHat/Fedora (etc).
* Put in /usr/share/initramfs-tools/ or /etc/initramfs-tools/. * With pull request openzfs#1476 (not yet merged) comes a verbose warning if /usr/bin/net doesn't exist or isn't executable. Just create a dummy... * If user haven't specified pool, but bootfs then use the first part of the bootfs path as pool name. * Add support for booting of a ZFS snapshot. Do this by cloning the snapshot into a dataset. If this the resulting dataset already exists, destroy it. Then set mountpoint=/ on that dataset and mount it on root. + If snapshot does not exist, use base dataset (the part before '@') as boot filesystem instead. * Add support for more kernel command line arguments (ideas taken from the Fedora/Redhat dracut script): + (zfs_force|zfs.force|zfsforce)=(on|yes|1) (ignore case). + root=zfs:AUTO => try to find rootfs automatically. + root=zfs:<dataset> * Remove the existing '-f' option to 'zpool import', instead let this be controlled on the kernel command line. * Do not force-set mountpoint=/ - should (??) not be nessesary. * Support mounting a rootfs with mountpoint=legacy * Support both RPM based and DEB based system by removing the logic from the dracut script and instead use the logic in scripts/zfs-initramfs/scripts/zfs for both Debian GNU/Linux (etc) and RedHat/Fedora (etc).
* Put in /usr/share/initramfs-tools/ or /etc/initramfs-tools/. * With pull request openzfs#1476 (not yet merged) comes a verbose warning if /usr/bin/net doesn't exist or isn't executable. Just create a dummy... * If user haven't specified pool, but bootfs then use the first part of the bootfs path as pool name. * Add support for booting of a ZFS snapshot. Do this by cloning the snapshot into a dataset. If this the resulting dataset already exists, destroy it. Then set mountpoint=/ on that dataset and mount it on root. + If snapshot does not exist, use base dataset (the part before '@') as boot filesystem instead. * Add support for more kernel command line arguments (ideas taken from the Fedora/Redhat dracut script): + (zfs_force|zfs.force|zfsforce)=(on|yes|1) (ignore case). + root=zfs:AUTO => try to find rootfs automatically. + root=zfs:<dataset> + root=ZFS=<dataset> * Remove the existing '-f' option to 'zpool import', instead let this be controlled on the kernel command line. * Do not force-set mountpoint=/ - should (??) not be nessesary. * Support mounting a rootfs with mountpoint=legacy * Support both RPM based and DEB based system by removing the logic from the dracut script and instead use the logic in scripts/zfs-initramfs/scripts/zfs for both Debian GNU/Linux (etc) and RedHat/Fedora (etc). * Only run the local-* script(s) if/when it/they exists. * Don't auto import pools when loading module - keep better control of the imports. * If called with only 'rpool' (or only 'zfs-bootfs'), then fake it by setting 'root=zfs:AUTO' (to avoid duplication of code etc) so that it will be auto detected later. * Rewrite the auto detection of bootfs in the auto detector. * Only try to import pool if it haven't already been imported (in the auto detector).
|
@FransUrbo Can you refresh this against master and repush it so we can get a clean test run. This is something we should be able to get in the next tag. |
f154f43 to
89a2e18
Compare
|
Apparently this patch has increased our dependency on the |
I have no problem with point three, I just think point two is .. "better" (and a good compromise) for the user/admin.
|
89a2e18 to
8e1264c
Compare
* Add support for options to 'sharesmb'. Fixes: openzfs#1182 + Move nfs.c:foreach_nfs_shareopt() to libshare.c:foreach_shareopt() so that it can be (re)used in smb.c (and later iscsi.c). + Call net(8) with the guest_ok option. + Rewrite the sharesmb part of the zfs(8) manpage. + Add more examples + Inform about the (new) options to sharesmb. + Add STDERR_VERBOSE to libzfs_run_process() so that we can catch any errors from net(8). + Don't call smb_retrieve_shares() after enabling a share. No point, it's done when really needed anyway. + Support 'y', 'yes' and 'true' for guest_ok. * If sharesmb=on, just exit cleanly directly at the top of get_smb_shareopts_cb(). * Remove param 2 and 3 from smb_enable_share_one(). That is availible in the first param - the impl_share struct. * Extra debugging added in smb_enable_share_one() - print all our options just before we call net(8). * Add support for optional, extra script /sbin/zfs_share_smbfs to run after shareing is done. + Add an example zfs_share_smbfs script * The option sharesmb should not be inherited by childs. Samba is perfectly able to show child file systems... * Use dataset name in comment, not the smb share name. * Use REGISTRY SHARES instead of USER SHARES + These are a lot more configurable (more possibility to customize the share). + Remove the 'acl' option and replace it with 'writeable' instead. + On some machines, there is a "global" share. This isn't really a share, but the possibility to modify global options. So first get a line of _real_ shares using 'net conf listshares' and _then_ cyckle through them retrieving info with 'net conf showshare <share>'. + Add an example on how to modify a share after ZFS share and what man pages to use for more info. * Speedups (less share retreivals) and make sure the 'net' command is executable. + Since smb_shares is global, we only retreive shares if it's NULL in smb_retrieve_shares(). It should limit the number of times we fork() and read shares (which takes time). + If the net command don't exist (is executable), don't even register the smb fstype in libshare_smb_init(). + access() fix - check if NET_CMD_PATH is _executable_ not just 'exists'. * Run cstyle.pl to fix any code style problems. * Output error if 'net' can't talk to samba. * Improve error message if 'net' isn't executable. * Move the checks from smb_available() to libshare_smb_init() to avoid doing them multiple times. * Convert homemade linked list to the ZFS versions list_* functions (which uses a link_t). * Script to list SMB shares added. * Comment out a call to zfs_unshare_smb(). Don't seem to be needed. This because zfs_unshare_smb() is called twice if setting 'sharesmb=off' - once 'somewhere else' and once here (this one AFTER the share have been unshared). That makes this one fail! Don't know if this is correct, but it works to comment out this...
|
A compromise would be to only output the error/warning messages if DEBUG is set... Pushed this new version. |
Initramfs scripts for ZoL. * Put in /usr/share/initramfs-tools/ or /etc/initramfs-tools/. * With pull request openzfs#1476 (not yet merged) comes a verbose warning if /usr/bin/net doesn't exist or isn't executable. Just create a dummy... * If user haven't specified pool, but bootfs then use the first part of the bootfs path as pool name. * Add support for booting of a ZFS snapshot. Do this by cloning the snapshot into a dataset. If this the resulting dataset already exists, destroy it. Then set mountpoint=/ on that dataset and mount it on root. + If snapshot does not exist, use base dataset (the part before '@') as boot filesystem instead. * Add support for more kernel command line arguments (ideas taken from the Fedora/Redhat dracut script): + (zfs_force|zfs.force|zfsforce)=(on|yes|1) (ignore case). + root=zfs:AUTO => try to find rootfs automatically. + root=zfs:<dataset> + root=ZFS=<dataset> * Remove the existing '-f' option to 'zpool import', instead let this be controlled on the kernel command line. * Do not force-set mountpoint=/ - should (??) not be nessesary. * Support mounting a rootfs with mountpoint=legacy * Support both RPM based and DEB based system by removing the logic from the dracut script and instead use the logic in scripts/zfs-initramfs/scripts/zfs for both Debian GNU/Linux (etc) and RedHat/Fedora (etc). * Only run the local-* script(s) if/when it/they exists. * Don't auto import pools when loading module - keep better control of the imports. * If called with only 'rpool' (or only 'zfs-bootfs'), then fake it by setting 'root=zfs:AUTO' (to avoid duplication of code etc) so that it will be auto detected later. + If pool is specified and we're auto discovering, don't go through ALL pools, only the one we specified. * Rewrite the auto detection of bootfs in the auto detector. * Only try to import pool if it haven't already been imported (in the auto detector). * Add support for only having 'root=pool/dataset' option. * Remove the use of setting 'ZFS_RPOOL=rpool'. No longer nessesary. * Check if /scripts/local-* is either file or directory - should be dir, but just to be safe check both. * Make wait_for_udev wait for 10 seconds. * Add a configurable ZFS_INITRD_POST_MODPROBE_SLEEP (set in /etc/default/zfs) after the modprobe. * Merge openzfs#2196. Remove lines that contain only a hyphen (match '^-$' instead of '-'). * Add a 'default' file to go in '/etc/default/zfs'. * Support mounting additional filesystems (such as /var and /usr etc) in the initrd using ZFS_INITRD_ADDITIONAL_DATASETS set in /etc/defaults/zfs. * Add exceptions to pool imports. + Both for init and initrd script. * Make sure that the zpool.cache file is actually copied onto the initrd!! * Include /etc/modprobe.d/{zfs,spl}.conf in the initrd if they exist. * Export all pools found after the modprobe - zfs_autoimport_disable=1' don't seem to be working in >= 0.6.3. + This snippet will go away as soon as I can figure out why it doesn't, it's ugly!! * Only try /dev/disk/by-* in the initrd if USE_DISK_BY_ID is set. + Try this first, then fallback to using the cache file if that exist.
|
My main concern here is that for As for how to handle the case where a share command is run and the utility isn't available. That really needs to be a hard error since it obviously can't succeed. |
Should be a fair compromise. |
|
Part of the reason this change has languished so long is because it contains a large number of small/medium size changes. That makes it hard to review and merge. If this change had been broken in to a stack of smaller patches I'm sure at least parts of it would have been merged by now. For example, I think refactoring |
Changing it to use Registry Shares instead allows ALL options allowed in smb.conf to be used in the share (even if sharesmb only does the basic, initial sharing). |
|
@behlendorf Add tag 'Share' on this one? |
|
Bumping to 0.6.5. @FransUrbo if you could refresh it based on the above comments we can revisit it for the next tag. |
|
Na, let's just ignore it. We can wait a year or two for zed to get the functionality instead. |
|
Soo... I've came across that and i see that we are almost in 2023 and we still can't set sharesmb to some options? |
|
No :(. @behlendorf couldn't be bothered to give me a decent review for (two?) years and then IMO came up with some BS review just because it wasn't perfect-perfect-perfect. It's good that PR quality is a priority, but in this case it was at least working perfectly fine in production. Both by me and others. So I simply gave up and left the project. He want to do it perfect, his way? Yeah, we've been waiting for almost ten years for that now and still nothing.. ! I still run my patch(es) in production and every time I upgrade my system I update the patches. However, I haven't had time or interest to do that in a few years - "if it works, don't mess with it" :D. |
Most notably:
A registry share is much more configurable than a user share, that basically only accepts path, comment and guest_ok. A registry share supports every option that one would have in smb.conf(5).
The only change, for the user/admin, is that the 'acl' option is no more. Instead there is the 'writeable' boolean.
The commit (162e908) about reopening the sharetab is to fix a problem I've seen quite a lot, but not been able to pinpoint. But it seems like when libzfs opens the sharetab, and libshare replaces it with an updated one, libzfs doesn't see the change/new file. So instead just reopen it before we try to read from it.