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
Add support for a virtual hostname to nfsd #180
Conversation
Specifically, this allows (via "-V vhostname") telling nfsd what principal to use, instead of the hostname. This is used at iXsystems for fail-over in HA systems. Reviewed by: macklem Sponsored by: iXsystems Inc. Differential Revision: https://reviews.freebsd.org/D19191 (cherry picked from commit 9fb40dda0a553c6e9243d2f483623672cc5c9bfa) Conflict etc/rc.d/nfsd resolved by sef
etc/rc.d/nfsd
Outdated
|
||
load_rc_config $name | ||
start_precmd="nfsd_precmd" | ||
sig_stop="USR1" | ||
|
||
nfsd_precmd() | ||
{ | ||
local _vhost |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am confused where/why is this var used?
Other than that looks OK to me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
D'oh. Hold-over from one of the interim versions of the startup script. Removed, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me except the few comments.
usr.sbin/nfsd/nfsd.c
Outdated
"dsserverN:/dsserverN-mounted-on-dir] [-m mirrorlevel]\n"; | ||
" [-p/--pnfs dsserver0:/dsserver0-mounted-on-dir,...,\n" | ||
" [-V virtual_hostname]\n" | ||
" dsserverN:/dsserverN-mounted-on-dir] [-m mirrorlevel]\n"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems this line was continuation of the previous. You inserted the new option at wrong place.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wrapped it so that it would not end up too long, just as first line ("dsserverN:") did.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not against wrapping. I am telling that third line is a continuation of the first.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I'm going to fault my dyslexia there. It took me more readings to see that I'd put the new line in the wrong place, hopefully better now.
if (vhost == NULL) | ||
gethostname(hostname, sizeof (hostname)); | ||
else | ||
strlcpy(hostname, vhost, sizeof (hostname)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a minor, style(9) does not use space after sizeof.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't kernel, and I used the same spacing as the line that I replaced.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't mind much about it. Though style(9) applies to user space too, when possible.
usr.sbin/nfsd/nfsd.c
Outdated
getopt_usage = | ||
"usage:\n" | ||
" nfsd [-ardtue] [-h bindip]\n" | ||
" [-n numservers] [--minthreads #] [--maxthreads #]\n" | ||
" [-p/--pnfs dsserver0:/dsserver0-mounted-on-dir,...," | ||
"dsserverN:/dsserverN-mounted-on-dir] [-m mirrorlevel]\n"; | ||
" [-V virtual_hostname]\n" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You need to move the semicolon now too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed, and did a buildworld just to be sure :).
new% ./nfsd -:
nfsd: invalid option -- :
usage:
nfsd [-ardtue] [-h bindip]
[-n numservers] [--minthreads #] [--maxthreads #]
[-p/--pnfs dsserver0:/dsserver0-mounted-on-dir,...,dsserverN:/dsserverN-mounted-on-dir] [-m mirrorlevel]
[-V virtual_hostname]
old% /usr/sbin/nfsd -:
nfsd: invalid option -- :
usage:
nfsd [-ardtue] [-h bindip]
[-n numservers] [--minthreads #] [--maxthreads #]
[-p/--pnfs dsserver0:/dsserver0-mounted-on-dir,...,dsserverN:/dsserverN-mounted-on-dir] [-m mirrorlevel]
Specifically, this allows (via "-V vhostname") telling nfsd what principal
to use, instead of the hostname. This is used at iXsystems for fail-over in
HA systems.
Reviewed by: macklem
Sponsored by: iXsystems Inc.
Differential Revision: https://reviews.freebsd.org/D19191
(cherry picked from commit 9fb40dda0a553c6e9243d2f483623672cc5c9bfa)
Conflict etc/rc.d/nfsd resolved by sef
This is part of merging our changes in; the intent here is to support both methods until we change our RC scripts to support the command-line option.