On Fri, Jul 18, 2008 at 7:21 PM, J. Bruce Fields <bfields@xxxxxxxxxxxx> wrote:
> On Mon, Jun 30, 2008 at 06:45:45PM -0400, Chuck Lever wrote:
>> Create a separate server-level interface for unregistering RPC services.
>>
>> The mechanics of and the API for registering and unregistering RPC
>> services will diverge further as support for IPv6 is added.
>>
>> Signed-off-by: Chuck Lever <chuck.lever@xxxxxxxxxx>
>> ---
>>
>> net/sunrpc/svc.c | 71 +++++++++++++++++++++++++++++++++++++++++++++---------
>> 1 files changed, 59 insertions(+), 12 deletions(-)
>>
>>
>> diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c
>> index d0e7865..a41b163 100644
>> --- a/net/sunrpc/svc.c
>> +++ b/net/sunrpc/svc.c
>> @@ -27,6 +27,8 @@
>>
>> #define RPCDBG_FACILITY RPCDBG_SVCDSP
>>
>> +static void svc_unregister(const struct svc_serv *serv);
>> +
>> #define svc_serv_is_pooled(serv) ((serv)->sv_function)
>>
>> /*
>> @@ -426,9 +428,8 @@ __svc_create(struct svc_program *prog, unsigned int bufsize, int npools,
>> spin_lock_init(&pool->sp_lock);
>> }
>>
>> -
>> /* Remove any stale portmap registrations */
>> - svc_register(serv, 0, 0);
>> + svc_unregister(serv);
>>
>> return serv;
>> }
>> @@ -496,8 +497,7 @@ svc_destroy(struct svc_serv *serv)
>> if (svc_serv_is_pooled(serv))
>> svc_pool_map_put();
>>
>> - /* Unregister service with the portmapper */
>> - svc_register(serv, 0, 0);
>> + svc_unregister(serv);
>> kfree(serv->sv_pools);
>> kfree(serv);
>> }
>> @@ -758,12 +758,10 @@ int
>> svc_register(struct svc_serv *serv, int proto, unsigned short port)
>> {
>> struct svc_program *progp;
>> - unsigned long flags;
>> unsigned int i;
>> int error = 0, dummy;
>>
>> - if (!port)
>> - clear_thread_flag(TIF_SIGPENDING);
>> + BUG_ON(proto == 0 && port == 0);
>>
>> for (progp = serv->sv_program; progp; progp = progp->pg_next) {
>> for (i = 0; i < progp->pg_nvers; i++) {
>> @@ -791,13 +789,62 @@ svc_register(struct svc_serv *serv, int proto, unsigned short port)
>> }
>> }
>>
>> - if (!port) {
>> - spin_lock_irqsave(¤t->sighand->siglock, flags);
>> - recalc_sigpending();
>> - spin_unlock_irqrestore(¤t->sighand->siglock, flags);
>> + return error;
>> +}
>
> The "port" in the (port && !dummy) check in this loop should also go.
If this patch were by itself, yes. But all this is cleaned out in the
next subsequent patch. I don't think it makes a difference here,
unless you think there is a good possibility these patches will be
separated.
>
>> +
>> +/*
>> + * The local rpcbind daemon listens on either only IPv6 or only
>> + * IPv4. The kernel can't tell how it's configured.
>> + *
>> + * However, AF_INET addresses are mapped to AF_INET6 in IPv6-only
>> + * configurations, so even an unregistration request on AF_INET
>> + * will get to a local rpcbind daemon listening only on AF_INET6.
>> + *
>> + * So we always unregister via AF_INET (the loopback address is
>> + * fairly unambiguous anyway).
>> + *
>> + * At this point we don't need rpcbind version 4 for unregistration:
>> + * A v2 UNSET request will clear all transports (netids), addresses,
>> + * and address families for [program, version].
>> + *
>> + * This should allow automatic support for both an all-IPv4 and
>> + * an all-IPv6 configuration.
>> + */
>> +static void __svc_unregister(struct svc_program *program, u32 version)
>> +{
>> + int error, boolean;
>> +
>> + error = rpcb_register(program->pg_prog, version, 0, 0, &boolean);
>> + dprintk("svc: svc_unregister(%sv%u), error %d, %s\n",
>> + program->pg_name, version, error,
>> + (boolean ? "succeeded" : "failed"));
>> +}
>> +
>> +/*
>> + * All transport protocols and ports for this service are removed from
>> + * the local rpcbind database. The result of unregistration is reported
>> + * via dprintk for those who want verification of the result, but is
>> + * otherwise not important.
>> + */
>> +static void svc_unregister(const struct svc_serv *serv)
>> +{
>> + struct svc_program *program;
>> + unsigned long flags;
>> + u32 version;
>
> It may just be brain-damage from too many years of mathematics, but
> let's leave this as "i" as before: its scope is only a few lines, its
> meaning is obvious from use, and this is what CodingStyle asks for
> anyway.
It may seem like a small thing, but I must disagree here. I assume
you are quibbling with the new name only and not the type change.
My reading of CodingStyle Chapter 4 is that "i" is appropriate instead
of "tmp" or "x" or "index" -- in other words where you need a generic
iterator. It doesn't require the name "i" for _all_ loop iterators.
I certainly wouldn't use "i" if I were iterating over characters or
addresses.
In mathematics (as you well know), "i, j, k" are used as subscripts or
for sequences or summations; often they refer to _every_ possible
value. We don't have any of that here. We are passing in RPC version
numbers. These may not even be in sequence: mountd has versions 1,
3, and 4, but not 2, nor 5 and above.
Any modern structured programming text recommends that we should name
the variable something that reflects its use. "i" is really quite
generic; "version" is "short and to the point," as Chapter 4
recommends.
[ "vers" is perhaps more concise, but I think nothing but ambiguity is
gained from dropping the last three letters. "lovers" could easily be
"low version" or "star-crossed lovers", for example].
Over the past several kernel releases I've included patches that
change variables storing RPC version numbers to "u32 version" wherever
they are used. I really don't see the need to be different here, and
I'd rather be consistent with nearly every other usage. If you're
storing an RPC version number, it is a u32 field or variable called
"version." The type and the name match what is in the RFCs.
>
>> +
>> + clear_thread_flag(TIF_SIGPENDING);
>> +
>> + for (program = serv->sv_program; program; program = program->pg_next) {
>> + for (version = 0; version < program->pg_nvers; version++) {
>> + if (program->pg_vers[version] == NULL)
>> + continue;
>> + __svc_unregister(program, version);
>
> Isn't there a change in behavior from the omitted vs_hidden check?
> I assume it's harmless to unregister something that was never
> registered (if that's indeed what this does), but it seems better to
> skip it.
svc_unregister() is used in svc_create() before registering a new
service, and in svc_destroy() when unregistering a service being shut
down.
It's advisable to do this now even for so-called hidden services
because of the ability for rpcbind to advertise RPC services at
particular addresses. The kernel registers an RPC service for the ANY
address, so all addresses for that service that are already registered
should be removed first.
Perhaps for hidden services, svc_unregister() should warn loudly or
fail immediately as a safety precaution, as these services should not
have been registered already, and if they are, we may be colliding
with something in user space.
> Needs a comment in the changelog in any case.
OK.
>
>> + }
>> }
>>
>> - return error;
>> + spin_lock_irqsave(¤t->sighand->siglock, flags);
>> + recalc_sigpending();
>> + spin_unlock_irqrestore(¤t->sighand->siglock, flags);
>> }
>>
>> /*
--
Chuck Lever
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
