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.

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

libshare cleanup, NFS path escaping #13165

Closed
wants to merge 16 commits into from

Conversation

nabijaczleweli
Copy link
Contributor

@nabijaczleweli nabijaczleweli commented Feb 28, 2022

Motivation and Context

The way libshare handled dispatch was honestly so weird. Also, the libzfs protocol that handles every function as _nfs and _smb is kinda insane and won't scale so I replaced it.

Also, #13153 and #13324

Description

See individual commit messages.

Currently on top of #13259.

How Has This Been Tested?

These were made:

  1 # !!! DO NOT EDIT THIS FILE MANUALLY !!!
  2
  3 /mnt/filling/machine/1200-S121 *(sec=sys,rw,no_subtree_check,mountpoint,crossmnt,no_subtree_check,no_root_squash,async)
  4 /spac\040ed\040out *(sec=sys,rw,no_subtree_check,mountpoint,crossmnt)
  5 /spaced\040out *(sec=sys,rw,no_subtree_check,mountpoint,crossmnt)
  6 /hupa\011#hehe\040\134\134epsko\040\134053\040\040\015 *(sec=sys,rw,no_subtree_check,mountpoint,crossmnt)
  7 /hupa\011#hehe\040\134\134epsko\040\134053\040\040\015\040достопримечательности *(sec=sys,rw,no_subtree_check,mountpoint,crossmnt)

From these:

nabijaczleweli@tarta:~/store/code/zfs$ sudo cmd/zfs/zfs create 'tarta-zoot/spaced out' -o sharenfs=on
nabijaczleweli@tarta:~/store/code/zfs$ sudo cmd/zfs/zfs create 'tarta-zoot/spa ced out' -o sharenfs=on  -o mountpoint='/hupa    #hehe \\epsko \053  ^M'
nabijaczleweli@tarta:~/store/code/zfs$ sudo cmd/zfs/zfs create 'tarta-zoot/spa ced out  ' -o sharenfs=on  -o mountpoint='/hupa  #hehe \\epsko \053  ^M достопримечательности'

And then consequently removed as the datasets were deleted.

They were also exported fine. Well, mostly. Turns out exportfs recursively parses backslash escapes. – reported upstream here

I had a different way for parsing:

static int
nfs_process_exports(const char *exports, const char *mountpoint,
    boolean_t (*cbk)(void *userdata, char *line, boolean_t found_mountpoint),
    void *userdata)
{
	int error = SA_OK;
	boolean_t cont = B_TRUE;

	FILE *oldfp = fopen(exports, "re");
	if (oldfp != NULL) {
		char *buf = NULL, *sep, *dename = NULL;
		size_t buflen = 0, mplen = strlen(mountpoint);

		while (cont && getline(&buf, &buflen, oldfp) != -1) {
			if (buf[0] == '\n' || buf[0] == '#')
				continue;

			boolean_t found = B_FALSE;
			if ((sep = strpbrk(buf, " \t\n")) != NULL) {
				size_t rawlen = sep - buf;
				if (rawlen >= mplen) {
					char *nm = buf;
					if (memchr(buf, '\\', rawlen)) {
						nm = dename = realloc(dename, rawlen);
						if (!dename) {
							error = SA_NO_MEMORY;
							goto err;
						}
						size_t outlen = 0;
						for (char *c = buf, *oc = dename; c < buf + rawlen; ++c, ++outlen)
							if (*c == '\\' && c + 3 < buf + rawlen && isdigit(c[1]) && isdigit(c[2]) && isdigit(c[3])) {
								*oc = c[1] - '0';
								*oc = (*oc << 3) | (c[2] - '0');
								*oc = (*oc << 3) | (c[3] - '0');
								++oc;
								c += 3;
							} else
								*oc++ = *c;
						rawlen = outlen;
					}
					fprintf(stderr, "%zu == %zu; cmp(%.*s, %.*s) = %d\n", rawlen, mplen, (int)rawlen, nm, (int)mplen, mountpoint, strncmp(nm, mountpoint, mplen));
					found = rawlen == mplen && strncmp(nm, mountpoint, mplen) == 0;
				}
			}
			cont = cbk(userdata, buf, found);
		}
err:
		free(buf);
		free(dename);

		if (ferror(oldfp) != 0)
			error = ferror(oldfp);

		if (fclose(oldfp) != 0) {
			fprintf(stderr, "Unable to close file %s: %s\n",
			    exports, strerror(errno));
			error = error != SA_OK ? error : SA_SYSTEM_ERR;
		}
	}

	return (error);
}

but it's more fragile (but, hardly), slower, and worse than just doing it this way. If we just agree on the escaped charset and stick to it, it won't be a problem.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Performance enhancement (non-breaking change which improves efficiency)
  • Code cleanup (non-breaking change which makes code smaller or more readable)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Library ABI change (libzfs, libzfs_core, libnvpair, libuutil and libzfsbootenv)
  • Documentation (a change to man pages or other documentation)

Checklist:

  • My code follows the OpenZFS code style requirements.
  • I have updated the documentation accordingly.
  • I have read the contributing document.
  • I have added tests to cover my changes. – TODO yeah. got carried away a bit, too
  • I have run the ZFS Test Suite with this change applied. – CI take my hand
  • All commit messages are properly formatted and contain Signed-off-by.

@nabijaczleweli nabijaczleweli changed the title libshare cleanup, path escaping libshare cleanup, NFS path escaping Feb 28, 2022
@behlendorf behlendorf added the Status: Code Review Needed Ready for review and testing label Feb 28, 2022
@behlendorf behlendorf added the Component: Share "zfs share" feature label Mar 4, 2022
@nabijaczleweli
Copy link
Contributor Author

nabijaczleweli commented Mar 4, 2022

During the porting process I've followed the (well, weak) style by adding ones that take a protocol list with _proto() at the end; however, they all do now, so in s?: libzfs share functions: _proto() -> nothing I've stripped it off again, and, looking at the total diff on GitHub, it ends up pretty slick, so I think we should squish it in (and just bump abiver as normal to prevent problems from same function names but different arguments).

Sum API changes are confined to libzfs.h and libshare.h.

@nabijaczleweli
Copy link
Contributor Author

nabijaczleweli commented Mar 5, 2022

Oh, it seems i hastily misread the manual; there's just no way to escape share paths on FreeBSD :) That being said, I think we should still try (which is the case in the current code) since the errors make much more sense:

nab@freebsd:~ % sudo mountd -rS /etc/zfs/exports
Mar  5 22:54:19 freebsd mountd[972]: bad exports list line '/testpool/spaced': symbolic link in export path or statfs failed

vs

nab@freebsd:~ % sudo mountd -rS /etc/zfs/exports
Mar  5 22:54:49 freebsd mountd[979]: bad exports list line '/testpool/spaced\040out': symbolic link in export path or statfs failed

and we're not gonna pollute other lines for shares with newlines in the path.

@nabijaczleweli
Copy link
Contributor Author

And on Illumos (this is Tribblix, uh, 5.11/-m25):
image
image

it..just doesn't work. zfs share shares via share(1M) and yields /literal path - nfs rw in /e/d/st (which is sharefs? apparently), but share(1M).. lists it as /literal? and mounting it uh, yeah lol
image

I was hoping to rig comprehensive tests for all platforms (now that I've, uh, enabled us actually.. running them? on all platforms), but, well.

andrewc12 pushed a commit to andrewc12/openzfs that referenced this pull request Sep 23, 2022
This makes it so we don't leak a consistent 64 bytes anymore,
makes the searches simpler and faster, removes /all allocations/
from the driver (quite trivially, since they were absolutely needless),
and makes libshare thread-safe (except, maybe, linux/smb, but that only
does pointer-width loads/stores so it's also mostly fine, except for
leaking smb_shares)

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz>
Closes openzfs#13165
andrewc12 pushed a commit to andrewc12/openzfs that referenced this pull request Sep 23, 2022
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz>
Closes openzfs#13165
andrewc12 pushed a commit to andrewc12/openzfs that referenced this pull request Sep 23, 2022
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz>
Closes openzfs#13165
andrewc12 pushed a commit to andrewc12/openzfs that referenced this pull request Sep 23, 2022
With the additional benefit of removing all the _all() functions and
treating a NULL list as "all" ‒ the remaining all function is for all
/datasets/, which is consistent with the rest of the API

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz>
Closes openzfs#13165
andrewc12 pushed a commit to andrewc12/openzfs that referenced this pull request Sep 23, 2022
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz>
Closes openzfs#13165
andrewc12 pushed a commit to andrewc12/openzfs that referenced this pull request Sep 23, 2022
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz>
Closes openzfs#13165
andrewc12 pushed a commit to andrewc12/openzfs that referenced this pull request Sep 23, 2022
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz>
Closes openzfs#13165
Closes openzfs#13153
andrewc12 pushed a commit to andrewc12/openzfs that referenced this pull request Sep 23, 2022
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz>
Closes openzfs#13165
andrewc12 pushed a commit to andrewc12/openzfs that referenced this pull request Sep 23, 2022
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz>
Closes openzfs#13165
andrewc12 pushed a commit to andrewc12/openzfs that referenced this pull request Sep 23, 2022
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz>
Closes openzfs#13165
andrewc12 pushed a commit to andrewc12/openzfs that referenced this pull request Sep 23, 2022
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz>
Closes openzfs#13165
Closes openzfs#13324
andrewc12 pushed a commit to andrewc12/openzfs that referenced this pull request Sep 23, 2022
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz>
Closes openzfs#13165
andrewc12 pushed a commit to andrewc12/openzfs that referenced this pull request Sep 23, 2022
This renders it thread-safe

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz>
Closes openzfs#13165
andrewc12 pushed a commit to andrewc12/openzfs that referenced this pull request Sep 23, 2022
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz>
Closes openzfs#13165
andrewc12 pushed a commit to andrewc12/openzfs that referenced this pull request Sep 23, 2022
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz>
Closes openzfs#13165
andrewc12 pushed a commit to andrewc12/openzfs that referenced this pull request Sep 23, 2022
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz>
Closes openzfs#13165
andrewc12 pushed a commit to andrewc12/openzfs that referenced this pull request Sep 23, 2022
This makes it so we don't leak a consistent 64 bytes anymore,
makes the searches simpler and faster, removes /all allocations/
from the driver (quite trivially, since they were absolutely needless),
and makes libshare thread-safe (except, maybe, linux/smb, but that only
does pointer-width loads/stores so it's also mostly fine, except for
leaking smb_shares)

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz>
Closes openzfs#13165
andrewc12 pushed a commit to andrewc12/openzfs that referenced this pull request Sep 23, 2022
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz>
Closes openzfs#13165
andrewc12 pushed a commit to andrewc12/openzfs that referenced this pull request Sep 23, 2022
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz>
Closes openzfs#13165
andrewc12 pushed a commit to andrewc12/openzfs that referenced this pull request Sep 23, 2022
With the additional benefit of removing all the _all() functions and
treating a NULL list as "all" ‒ the remaining all function is for all
/datasets/, which is consistent with the rest of the API

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz>
Closes openzfs#13165
andrewc12 pushed a commit to andrewc12/openzfs that referenced this pull request Sep 23, 2022
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz>
Closes openzfs#13165
andrewc12 pushed a commit to andrewc12/openzfs that referenced this pull request Sep 23, 2022
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz>
Closes openzfs#13165
andrewc12 pushed a commit to andrewc12/openzfs that referenced this pull request Sep 23, 2022
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz>
Closes openzfs#13165
Closes openzfs#13153
andrewc12 pushed a commit to andrewc12/openzfs that referenced this pull request Sep 23, 2022
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz>
Closes openzfs#13165
andrewc12 pushed a commit to andrewc12/openzfs that referenced this pull request Sep 23, 2022
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz>
Closes openzfs#13165
andrewc12 pushed a commit to andrewc12/openzfs that referenced this pull request Sep 23, 2022
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz>
Closes openzfs#13165
andrewc12 pushed a commit to andrewc12/openzfs that referenced this pull request Sep 23, 2022
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz>
Closes openzfs#13165
Closes openzfs#13324
andrewc12 pushed a commit to andrewc12/openzfs that referenced this pull request Sep 23, 2022
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz>
Closes openzfs#13165
andrewc12 pushed a commit to andrewc12/openzfs that referenced this pull request Sep 23, 2022
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz>
Closes openzfs#13165
Closes openzfs#13324
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Share "zfs share" feature Status: Accepted Ready to integrate (reviewed, tested)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants