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

Rewrite of nfs.c to keep options per host separated. #2790

Closed
wants to merge 2 commits into from

Conversation

FransUrbo
Copy link
Contributor

Example: Setting sharenfs to

  "rw=@192.168.69.8,no_subtree_check,no_root_squash,ro=@192.168.69.45,no_subtree_check,root_squash"

will create the option string

  sec=sys,ro,no_subtree_check,no_root_squash,mountpoint,no_subtree_check,no_root_squash,no_subtree_check,root_squash

which is then run on each host!. Part of this is "default" options.

Instead, go through the option string, look for hosts and then make sure options for each host is kept apart by using a linked list (maintained by the ZFS list_* functions).
Also, the default options should only be added to the list of options if setting sharenfs to on.

Closes: #2773

@behlendorf
Copy link
Contributor

Attempting to translate the Solaris syntax is really becoming a pain. While looking at this I noticed that the FreeBSD implementation doesn't even try, they require FreeBSD style options. So in some sense we've already lost the battle since by translating we're going to be at best compatibility with Illumos. We may want to reconsider this policy.

Leaving that matter aside this patch doesn't look quite right to me. It does address the case in #2773 but it appears to break other cases which end users may be depending on. For example, the updated code doesn't appear to allow you to list multiple colon seperated hosts. For example, rw=192.168.1.1:192.168.1.2. Plus it appears to break the duplicate detection code.

@FransUrbo
Copy link
Contributor Author

the updated code doesn't appear to allow you to list multiple colon seperated hosts

Didn't even know you where allowed to do that!

Is there documentation somewhere on how to create a export string? Neither exports(5), exportfs(8), rpc.mountd(8) nor nfs(5) explains how this is setup.

It (exports(5) DOES talk about colons, but only in relation to IPv6 hosts:

EXAMPLE
       /foo            2001:db8:9:e54::/64(rw) 192.0.2.0/24(rw)

Actually, this is on Linux. So this isn't a Linux option then…?

I'm not against ditching Solaris options support, I just feel that we should really, really try.. With this patch, I've at least tried to make it easier… ?=

@behlendorf
Copy link
Contributor

The expected Solaris/Illumos syntax can be found in the share(1M) and share_nfs(1M) man pages. If you take a look at example 2 in share(1M) you'll see how ro, rw, and multiple hosts can be specified.

http://www.lehman.cuny.edu/cgi-bin/man-cgi?share+1
http://www.lehman.cuny.edu/cgi-bin/man-cgi?share_nfs+1

The Linux NFS syntax for this is very different. The code does a reasonable job today translating but this is one case it doesn't try to handle. The whole thing is further complicated by the fact that there are illumos nfs options which have no Linux equivalent and visa-versa.

It's very tempting to just leave it as is for now and make plans to just cut over at some point to Linux options. As I mentioned above, this appears to be what FreeBSD did.

so that it can be (re)used in other parts of libshare.
Example: Setting sharenfs to

  "rw=@192.168.69.8,no_subtree_check,no_root_squash,ro=@192.168.69.45,no_subtree_check,root_squash"

will create the option string

  sec=sys,ro,no_subtree_check,no_root_squash,mountpoint,no_subtree_check,no_root_squash,no_subtree_check,root_squash

which is then run on _each host_!. Part of this is "default" options.

Instead, go through the option string, look for hosts and then make
sure options for each host is kept apart by using a linked list
(maintained by the ZFS list_* functions). Also, the default options
should only be added to the list of options if setting sharenfs to
"on" (and then to the '*' "host").

Closes: openzfs#2773
@FransUrbo
Copy link
Contributor Author

@behlendorf Do you think you can spend an hour or two to look this over? It's now been running in the Debian GNU/Linux packages for almost a year and except for a minor problem in the beginning, I've heard nothing about this so I'm pretty sure it works just fine.

Copy link
Contributor

@behlendorf behlendorf left a comment

Choose a reason for hiding this comment

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

These changes need to be rebased on master so they apply cleanly. The should be easy since there's been very little churn in this file.

Since this change was originally proposed the ZFS Test Suite has been merged and gets run on every PR. In order to get test coverage on these modifications to the share support the zfs share/unshare tests need be enabled in the linux.run file and updated so most or all of them pass. If a few test cases are problematic we can leave them disabled for now. This would still be a huge step in the right direction. We can install whatever packages are needed in to the test VMs to support this.

Once we have automated test coverage and the tests are passing I'll be comfortable merging this change. It with also make it easier to improve this code in the future.

#include <unistd.h>
#include <libzfs.h>
#include <libshare.h>
#include "libshare_impl.h"

#if !defined(offsetof)
#define offsetof(s, m) ((size_t)(&(((s *)0)->m)))
#endif
Copy link
Contributor

Choose a reason for hiding this comment

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

Why was this needed, offsetof is part of stddef.h
http://man7.org/linux/man-pages/man3/offsetof.3.html

/* NOTE: share path is stored elsewhere */
typedef struct nfs_share_list_s {
char host[255];
char opts[255];
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's add #define's for these buffer sizes. It may make sense to move those defines and this structure to lib/libshare/nfs.h which would be analogous to lib/libshare/smb.h.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a NFS limitation, so I don't see the point in having this as a define..

https://linux.die.net/man/5/nfs

namlen=n

The maximum length of a pathname component on this mount. If this option is not specified, the maximum length is negotiated with the server. In most cases, this maximum length is 255 characters.

*plinux_hostspec = strdup(solaris_hostspec);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

[cstyle] The { } are expected for the else block when need by the if portion.


#ifdef DEBUG
fprintf(stderr, "sharing %s with opts %s\n", hostpath, opts);
fprintf(stderr, "sharing %s with opts %s\n",
hostpath, (opts ? opts : "NULL"));
Copy link
Contributor

Choose a reason for hiding this comment

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

[cstyle] 4 space indent to continue a line.

for (i = 0; i < 5; i++)
fprintf(stderr, "%s ", argv[i]);
fprintf(stderr, "\n");
#endif
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we still need these DEBUG blocks. If we really need to preserve this debugging I suggest updating the code to use dprintf(). Then this debugging can be enabled at run time rather than build time like this, ZFS_DEBUG="on" zfs share ....

if (shareopts == NULL)
return (SA_OK);

rc = get_linux_shareopts(shareopts, &linux_opts);
linux_opts = calloc(sizeof (nfs_share_list_t), 1);
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs to be checked for a NULL return.

for (i = 0; i < 3; i++)
fprintf(stderr, "%s ", argv[i]);
fprintf(stderr, "\n");
#endif
Copy link
Contributor

Choose a reason for hiding this comment

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

Drop or convert to dprintf()


#ifdef DEBUG
fprintf(stderr, "nfs_disable_share: %s\n", impl_share->sharepath);
#endif
Copy link
Contributor

Choose a reason for hiding this comment

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

Drop or convert to dprintf()

if (shareopts == NULL)
return (SA_OK);

linux_opts = calloc(sizeof (nfs_share_list_t), 1);
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs to be checked for a NULL return.

@@ -440,6 +522,8 @@ nfs_validate_shareopts(const char *shareopts)
char *linux_opts;
int rc;

linux_opts = calloc(sizeof (nfs_share_list_t), 1);
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs to be checked for a NULL return.

@behlendorf
Copy link
Contributor

Closing for now to reduce the number of open active PRs. We can absolutely consider merging this functionality once the patch is refreshed against master and we have some share test cases to validate this works.

@behlendorf behlendorf closed this Oct 1, 2016
@dipack95
Copy link

@behlendorf @FransUrbo Issue #2773 still plagues us, and I noticed this patch attempts to fix that issue. Is there an ETA on this being merged in the project? I can help if required.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Share "zfs share" feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot specify different NFS share options for multiple hosts
3 participants