On Wednesday November 8, [email protected] wrote:
> On 11/8/06, Neil Brown <[email protected]> wrote:
> > On Wednesday November 1, [email protected] wrote:
> > > - ret = svc_makesock(serv, IPPROTO_TCP, nfs_callback_set_tcpport);
> > > + ret = svc_makesock_noregister(serv, IPPROTO_TCP, nfs_callback_set_tcpport);
> > > if (ret < 0)
> > > goto out_destroy;
> > > - if (!list_empty(&serv->sv_permsocks)) {
> > > - svsk = list_entry(serv->sv_permsocks.next,
> > > - struct svc_sock, sk_list);
> > > - nfs_callback_tcpport = ntohs(inet_sk(svsk->sk_sk)->sport);
> > > - dprintk ("Callback port = 0x%x\n", nfs_callback_tcpport);
> > > - } else
> > > - BUG();
> > > + if (list_empty(&serv->sv_tempsocks)) {
> > > + ret = -ENOMEM;
> > > + goto out_destroy;
> > > + }
> > > + svsk = list_entry(serv->sv_tempsocks.next, struct svc_sock, sk_list);
> > > + nfs_callback_tcpport = ntohs(inet_sk(svsk->sk_sk)->sport);
> > > + dprintk("Callback port = 0x%x\n", nfs_callback_tcpport);
> >
> > This is somewhat ugly (both old and new code).
> > Can we just get svc_makesock* to take an 'int *' rather than an 'int'
> > so that it can return a dynamically allocated port number?
>
> Well, yes, but I was told Linus frowns on that way of returning a
> result. Maybe svc_makesock can return a value such that, if negative,
> it reflects an error, and if positive, is the assigned port?
Let him frown??
-ve error or +ve port should be ok though. Return the port in host
byte order, otherwise the return type will be hard to declare
properly.
>
> > Also I'm not very comfortable with the fact that sockets that aren't
> > registered become ipso-facto temporary sockets. They might get closed
> > due to age.
>
> I'm not sure I understand.
>
You implement svc_makesock_noregister by passing a '0' all the way
down to svc_setup_socket as it's pmap_register argument.
This argument does two things.
1/ it chooses whether to register the port with portmap or not.
2/ it chooses whether the socket should go on sv_permsocks or
sv_tempsocks.
Sockets listed on sv_tempsocks a liable to be closed without notice,
either because there are too many of them or because one has not seen
any traffic for 6 minutes.
I don't think that you want the socket on which you are listening for
callbacks to be closed if there are no callbacks for 6 minutes.
So currently 'pmap_register' means two different things and we need to
split it so that we can mean one (don't register) without meaning the
other (socket is temporary).
Make sense?
> > Maybe we should pull the call to svc_register out of svc_setup_socket
> > and change the pmap_register flag to is_permanent or similar.
>
> That doesn't bother me.
That was just an implementation suggestion.
NeilBrown
-------------------------------------------------------------------------
Using Tomcat but need to do more? Need to support web services, security?
Get stuff done quickly with pre-integrated technology to make your job easier
Download IBM WebSphere Application Server v.1.0.1 based on Apache Geronimo
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=120709&bid=263057&dat=121642
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs
On Thu, Nov 09, 2006 at 03:15:19PM +1100, Neil Brown wrote:
> So currently 'pmap_register' means two different things and we need to
> split it so that we can mean one (don't register) without meaning the
> other (socket is temporary).
I very much agree with that. I stumbled across this as well
a while ago.
Olaf
--
Walks like a duck. Quacks like a duck. Must be a chicken.
-------------------------------------------------------------------------
Using Tomcat but need to do more? Need to support web services, security?
Get stuff done quickly with pre-integrated technology to make your job easier
Download IBM WebSphere Application Server v.1.0.1 based on Apache Geronimo
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=120709&bid=263057&dat=121642
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs