lkml.org 
[lkml]   [2005]   [Oct]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
From
Date
SubjectRe: [PATCH] kNFSD - Allowing rpc.nfsd to setting of the port, transport and version the server will use
On Tuesday October 11, SteveD@redhat.com wrote:
> >
> > The 'port' bit I had trouble liking.
> > You write:
> >
> > family proto proto addr port
> >
> > to the 'ports' file.
> > 'family' and 'addr' are completely ignored.
> Well at this point they are not needed since we
> only support ipv4... and I can only assume when we do
> support other families, changing this interface will be
> the least of our problems... ;-)

Maybe, but either we make it ready for future needs now, or we only
support current needs and allow a clear upgrade path.
Ignoring fields which might later have a meaning is bad because it
means that user-space code which works now much break later for no
good reason.
My basic position wrt this is that if the fields are there, they
should be checked.

>
> > 'port' is effectively ignored (value is stored in a variable which
> > isn't used).
> I'm not sure I understand this... nfsd_port is set and used in
> nfsd_svc()
> @@ -104,11 +152,14 @@ nfsd_svc(unsigned short port, int nrserv
> nfsd_serv = svc_create(&nfsd_program, NFSD_BUFSIZE);
> if (nfsd_serv == NULL)
> goto out;
> + if (NFSCTL_UDPISSET(nfsd_portbits))
> + port = nfsd_port;
> error = svc_makesock(nfsd_serv, IPPROTO_UDP, port);
> if (error < 0)
> goto failure;
> #ifdef CONFIG_NFSD_TCP
> + if (NFSCTL_TCPISSET(nfsd_portbits))
> + port = nfsd_port;
> error = svc_makesock(nfsd_serv, IPPROTO_UDP, port);
> if (error < 0)
> goto failure;
>
> iff the nfsd_portbits are set... which is exactly the same concept
> used with the version bits... so I am a bit confused on what you
> want...

Uhmmmm.... that code fragment isn't in the patch you sent. I got:

> @@ -104,14 +152,17 @@ nfsd_svc(unsigned short port, int nrserv
> nfsd_serv = svc_create(&nfsd_program, NFSD_BUFSIZE);
> if (nfsd_serv == NULL)
> goto out;
> - error = svc_makesock(nfsd_serv, IPPROTO_UDP, port);
> - if (error < 0)
> - goto failure;
> -
> + if (!nfsd_portbits || NFSCTL_UDPISSET(nfsd_portbits)) {
> + error = svc_makesock(nfsd_serv, IPPROTO_UDP, port);
> + if (error < 0)
> + goto failure;
> + }
> #ifdef CONFIG_NFSD_TCP
> - error = svc_makesock(nfsd_serv, IPPROTO_TCP, port);
> - if (error < 0)
> - goto failure;
> + if (!nfsd_portbits || NFSCTL_TCPISSET(nfsd_portbits)) {
> + error = svc_makesock(nfsd_serv, IPPROTO_TCP, port);
> + if (error < 0)
> + goto failure;
> + }
> #endif
> do_gettimeofday(&nfssvc_boot); /* record boot time */
> } else

I guess if you merged these two, you might get something usable..

> >
> > That leaves 'proto' and 'proto'. One should be 'tcp' or 'notcp', the
> > other should be 'udp' or 'noudp'. Which is which? Udp comes first,
> > but it isn't at all obvious from the interface..
> Maybe being the author of these interfaces you have a better perspective
> than I, but I could say the same thing about all the interfaces under
> /proc/fs/nfsd... :-) So I do agree... its not that obvious, but I guess
> I didn't think it needed to be.. Who else do you see, other than
> rpc.nfsd, using this interface?

Ok, I guess I presented by difficulty with this rather badly. The
order does seem arbitrary and non-obvious, but you are right that
other things are no-obvious too...
The point I should have made was that it is non-extensible.
Suppose NFSv4.4 is defined to work over SCTP to even DCCP (which might
be ideal for NFS), how would we specify those?

>
> > I think you should write:
> > [+-]family proto addr port
> >
> > and every field must be checked and used.
> > So while we only support ipv4, the 'family' must by 'ipv4' or an error
> > is returned.
> > '+' adds an endpoint. '-' removes it.
> Understood... And I'll assume the default of both TCP and UDP
> listening on port 2048 will stay the same when nothing
> is specified...

Yes... but I'm not sure of the rest of the rules about defaults...
- should you need to explicitly remove the defaults if you don't want
them?
- should there be an easy way to revert-to-defaults? Maybe when the
last nfsd dies, all setting are forgotten (just like currently all
exports are removed).


>
> >
> > The old nfssvc syscall should add 'ipv4 udp * %port' and 'ipv4 tcp *
> > %port' if they don't already exist.
> Won't this break backwards compatibility with all the 2.4 kernels?
> The beauty of seq files is that you can change the interface
> and have no effect on kABI at all... which is really a good thing
> in my world... So why do we even care about the old syscall
> interface?

I don't see why it would break backwards compat. Isn't that exactly
what the nfssvc syscall does? You give it a port number, and it
starts the number of threads with two sockets, a udp bound to ADDR_ANY
and the given port, and a TCP bound the same way? What is what I
meant to say above.

>
> >
> > An alternate interface, which would be quite appealing, would be to
> > require the user-space program to create and bind a socket and then
> > communicate it to the kernel, possibly by writing a file-descriptor
> > number to a file in the nfsd filesystem.
> > 'nfsd' would check it is an appropriate type of socket, take
> > an extra reference, and use it.
> > This would probably be best done *after* the nfsd threads were
> > started, so there would need to be a way to start threads without
> > them automatically opening sockets. I'm not sure what the best
> > interface for that would be... Maybe establishing sockets before the
> > thread would be ok.
> Maybe I'm missing something here.... but I'm not quite sure how passing
> a fd to the kernel would help, other than (possibly) with error
> processing... The kernel will still need to know the port and proto so
> it can register them with the portmapper... plus permissions could
> become an problem especially with things like selinux running around..
>

I think the principle of "do as much as possible in user-space" is a
good one.
socket / bind / pmap_register
can all be done in user-space, so maybe they should be.
You could possible even argue that
listen / accept
the be done in user-space, and so should be, but I'm not sure I want
to push it that far.

The nice thing about just passing down a filedescriptor for a socket
is that it allows for any future additions of protocols.

I don't see how permissions could be a problem, but then I know very
little about selinux, so maybe there is something. Is there something
specific you foresee, or is it just general concern?

NeilBrown
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

\
 
 \ /
  Last update: 2005-10-12 06:25    [W:2.040 / U:0.004 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site