On Mon, Oct 29, 2012 at 11:36 AM, Leonardo Chiquitto
<leonardo.lists@xxxxxxxxx> wrote:
> On Mon, Oct 29, 2012 at 12:14 AM, Ian Kent <raven@xxxxxxxxxx> wrote:
>> On Mon, 2012-10-29 at 09:34 +0800, Ian Kent wrote:
>>> >
>>> > * Triggering the mounts simultaneously is required to reproduce the
>>> > problem, which
>>> > makes me think if something here (libtirpc for example) is not
>>> > really thread safe
>>> > or if the RPC clients must be destroyed after every use to avoid such issues.
>>>
>>> Don't think so, it's more likely I'm using a structure obtained from a
>>> non-reentrant library call.
>>
>> Yeah, getprotobyname(3) isn't re-entrant, that's likely the problem.
>
> Thanks, Ian. Indeed this is the problem.
>
> Instead of using the reentrant version, I think it's better to just get rid
> of getprotobyname(). The only field from the protoent structure that is
> used at the moment is the protocol ID.
>
> I think it's much easier to just use the ID directly:
>
> Index: autofs/include/rpc_subs.h
> ===================================================================
> --- autofs.orig/include/rpc_subs.h
> +++ autofs/include/rpc_subs.h
> @@ -54,7 +54,7 @@ struct conn_info {
> unsigned short port;
> unsigned long program;
> unsigned long version;
> - struct protoent *proto;
> + int proto;
> unsigned int send_sz;
> unsigned int recv_sz;
> struct timeval timeout;
> @@ -66,7 +66,7 @@ int rpc_udp_getclient(struct conn_info *
> void rpc_destroy_udp_client(struct conn_info *);
> int rpc_tcp_getclient(struct conn_info *, unsigned int, unsigned int);
> void rpc_destroy_tcp_client(struct conn_info *);
> -int rpc_portmap_getclient(struct conn_info *, const char *, struct
> sockaddr *, size_t, const char *, unsigned int);
> +int rpc_portmap_getclient(struct conn_info *, const char *, struct
> sockaddr *, size_t, int, unsigned int);
> int rpc_portmap_getport(struct conn_info *, struct pmap *, unsigned short *);
> int rpc_ping_proto(struct conn_info *);
> int rpc_ping(const char *, long, long, unsigned int);
> Index: autofs/modules/replicated.c
> ===================================================================
> --- autofs.orig/modules/replicated.c
> +++ autofs/modules/replicated.c
> @@ -423,7 +423,7 @@ void free_host_list(struct host **list)
>
> static unsigned int get_nfs_info(unsigned logopt, struct host *host,
> struct conn_info *pm_info, struct conn_info *rpc_info,
> - const char *proto, unsigned int version, int port)
> + int proto, unsigned int version, int port)
> {
> unsigned int random_selection = host->options & MOUNT_FLAG_RANDOM_SELECT;
> unsigned int use_weight_only = host->options & MOUNT_FLAG_USE_WEIGHT_ONLY;
> @@ -437,22 +437,18 @@ static unsigned int get_nfs_info(unsigne
> int status, count = 0;
>
> if (host->addr)
> - debug(logopt, "called with host %s(%s) proto %s version 0x%x",
> + debug(logopt, "called with host %s(%s) proto %d version 0x%x",
> host->name, get_addr_string(host->addr, buf, len),
> proto, version);
> else
> debug(logopt,
> - "called for host %s proto %s version 0x%x",
> + "called for host %s proto %d version 0x%x",
> host->name, proto, version);
>
> - rpc_info->proto = getprotobyname(proto);
> - if (!rpc_info->proto)
> - return 0;
> -
> + rpc_info->proto = proto;
> memset(&parms, 0, sizeof(struct pmap));
> -
> parms.pm_prog = NFS_PROGRAM;
> - parms.pm_prot = rpc_info->proto->p_proto;
> + parms.pm_prot = proto;
>
> if (!(version & NFS4_REQUESTED))
> goto v3_ver;
> @@ -483,7 +479,7 @@ static unsigned int get_nfs_info(unsigne
> }
> }
>
> - if (rpc_info->proto->p_proto == IPPROTO_UDP)
> + if (rpc_info->proto == IPPROTO_UDP)
> status = rpc_udp_getclient(rpc_info, NFS_PROGRAM, NFS4_VERSION);
> else
> status = rpc_tcp_getclient(rpc_info, NFS_PROGRAM, NFS4_VERSION);
> @@ -544,7 +540,7 @@ v3_ver:
> goto v2_ver;
> }
>
> - if (rpc_info->proto->p_proto == IPPROTO_UDP)
> + if (rpc_info->proto == IPPROTO_UDP)
> status = rpc_udp_getclient(rpc_info, NFS_PROGRAM, NFS3_VERSION);
> else
> status = rpc_tcp_getclient(rpc_info, NFS_PROGRAM, NFS3_VERSION);
> @@ -591,7 +587,7 @@ v2_ver:
> goto done_ver;
> }
>
> - if (rpc_info->proto->p_proto == IPPROTO_UDP)
> + if (rpc_info->proto == IPPROTO_UDP)
> status = rpc_udp_getclient(rpc_info, NFS_PROGRAM, NFS2_VERSION);
> else
> status = rpc_tcp_getclient(rpc_info, NFS_PROGRAM, NFS2_VERSION);
> @@ -622,7 +618,7 @@ v2_ver:
> }
>
> done_ver:
> - if (rpc_info->proto->p_proto == IPPROTO_UDP) {
> + if (rpc_info->proto == IPPROTO_UDP) {
> rpc_destroy_udp_client(rpc_info);
> rpc_destroy_udp_client(pm_info);
> } else {
> @@ -679,7 +675,7 @@ static int get_vers_and_cost(unsigned lo
>
> if (version & TCP_REQUESTED) {
> supported = get_nfs_info(logopt, host,
> - &pm_info, &rpc_info, "tcp", vers, port);
> + &pm_info, &rpc_info, IPPROTO_TCP, vers, port);
> if (IS_ERR(supported)) {
> if (ERR(supported) == EHOSTUNREACH ||
> ERR(supported) == ETIMEDOUT)
> @@ -692,7 +688,7 @@ static int get_vers_and_cost(unsigned lo
>
> if (version & UDP_REQUESTED) {
> supported = get_nfs_info(logopt, host,
> - &pm_info, &rpc_info, "udp", vers, port);
> + &pm_info, &rpc_info, IPPROTO_UDP, vers, port);
> if (IS_ERR(supported)) {
> if (!ret && ERR(supported) == ETIMEDOUT)
> return ret;
> @@ -713,7 +709,7 @@ static int get_supported_ver_and_cost(un
> socklen_t len = INET6_ADDRSTRLEN;
> char buf[len + 1];
> struct conn_info pm_info, rpc_info;
> - const char *proto;
> + int proto;
> unsigned int vers;
> struct timeval start, end;
> struct timezone tz;
> @@ -752,10 +748,10 @@ static int get_supported_ver_and_cost(un
> * So, we do the conversion here.
> */
> if (version & UDP_SELECTED_MASK) {
> - proto = "udp";
> + proto = IPPROTO_UDP;
> version >>= 8;
> } else
> - proto = "tcp";
> + proto = IPPROTO_TCP;
>
> switch (version) {
> case NFS2_SUPPORTED:
> @@ -772,9 +768,7 @@ static int get_supported_ver_and_cost(un
> return 0;
> }
>
> - rpc_info.proto = getprotobyname(proto);
> - if (!rpc_info.proto)
> - return 0;
> + rpc_info.proto = proto;
>
> if (port > 0)
> rpc_info.port = port;
> @@ -790,14 +784,14 @@ static int get_supported_ver_and_cost(un
>
> memset(&parms, 0, sizeof(struct pmap));
> parms.pm_prog = NFS_PROGRAM;
> - parms.pm_prot = rpc_info.proto->p_proto;
> + parms.pm_prot = rpc_info.proto;
> parms.pm_vers = vers;
> ret = rpc_portmap_getport(&pm_info, &parms, &rpc_info.port);
> if (ret < 0)
> goto done;
> }
>
> - if (rpc_info.proto->p_proto == IPPROTO_UDP)
> + if (rpc_info.proto == IPPROTO_UDP)
> status = rpc_udp_getclient(&rpc_info, NFS_PROGRAM, vers);
> else
> status = rpc_tcp_getclient(&rpc_info, NFS_PROGRAM, vers);
> @@ -819,7 +813,7 @@ static int get_supported_ver_and_cost(un
> }
> }
> done:
> - if (rpc_info.proto->p_proto == IPPROTO_UDP) {
> + if (rpc_info.proto == IPPROTO_UDP) {
> rpc_destroy_udp_client(&rpc_info);
> rpc_destroy_udp_client(&pm_info);
> } else {
> Index: autofs/lib/rpc_subs.c
> ===================================================================
> --- autofs.orig/lib/rpc_subs.c
> +++ autofs/lib/rpc_subs.c
> @@ -160,7 +160,7 @@ static int rpc_do_create_client(struct s
>
> *client = NULL;
>
> - proto = info->proto->p_proto;
> + proto = info->proto;
> if (proto == IPPROTO_UDP)
> type = SOCK_DGRAM;
> else
> @@ -191,7 +191,7 @@ static int rpc_do_create_client(struct s
> in4_raddr = (struct sockaddr_in *) addr;
> in4_raddr->sin_port = htons(info->port);
>
> - switch (info->proto->p_proto) {
> + switch (info->proto) {
> case IPPROTO_UDP:
> clnt = clntudp_bufcreate(in4_raddr,
> info->program, info->version,
> @@ -231,7 +231,7 @@ static int rpc_do_create_client(struct s
>
> *client = NULL;
>
> - proto = info->proto->p_proto;
> + proto = info->proto;
> if (proto == IPPROTO_UDP)
> type = SOCK_DGRAM;
> else
> @@ -282,11 +282,11 @@ static int rpc_do_create_client(struct s
> nb_addr.maxlen = nb_addr.len = slen;
> nb_addr.buf = addr;
>
> - if (info->proto->p_proto == IPPROTO_UDP)
> + if (info->proto == IPPROTO_UDP)
> clnt = clnt_dg_create(*fd, &nb_addr,
> info->program, info->version,
> info->send_sz, info->recv_sz);
> - else if (info->proto->p_proto == IPPROTO_TCP) {
> + else if (info->proto == IPPROTO_TCP) {
> ret = connect_nb(*fd, addr, slen, &info->timeout);
> if (ret < 0)
> return ret;
> @@ -345,7 +345,7 @@ static int create_client(struct conn_inf
> memset(&hints, 0, sizeof(hints));
> hints.ai_flags = AI_ADDRCONFIG;
> hints.ai_family = AF_UNSPEC;
> - if (info->proto->p_proto == IPPROTO_UDP)
> + if (info->proto == IPPROTO_UDP)
> hints.ai_socktype = SOCK_DGRAM;
> else
> hints.ai_socktype = SOCK_STREAM;
> @@ -360,7 +360,7 @@ static int create_client(struct conn_inf
>
> haddr = ai;
> while (haddr) {
> - if (haddr->ai_protocol != info->proto->p_proto) {
> + if (haddr->ai_protocol != info->proto) {
> haddr = haddr->ai_next;
> continue;
> }
> @@ -407,16 +407,11 @@ out_close:
> int rpc_udp_getclient(struct conn_info *info,
> unsigned int program, unsigned int version)
> {
> - struct protoent *pe_proto;
> CLIENT *client;
> int ret;
>
> if (!info->client) {
> - pe_proto = getprotobyname("udp");
> - if (!pe_proto)
> - return -ENOENT;
> -
> - info->proto = pe_proto;
> + info->proto = IPPROTO_UDP;
> info->timeout.tv_sec = RPC_TOUT_UDP;
> info->timeout.tv_usec = 0;
> info->send_sz = UDPMSGSIZE;
> @@ -448,16 +443,11 @@ void rpc_destroy_udp_client(struct conn_
> int rpc_tcp_getclient(struct conn_info *info,
> unsigned int program, unsigned int version)
> {
> - struct protoent *pe_proto;
> CLIENT *client;
> int ret;
>
> if (!info->client) {
> - pe_proto = getprotobyname("tcp");
> - if (!pe_proto)
> - return -ENOENT;
> -
> - info->proto = pe_proto;
> + info->proto = IPPROTO_TCP;
> info->timeout.tv_sec = RPC_TOUT_TCP;
> info->timeout.tv_usec = 0;
> info->send_sz = 0;
> @@ -503,23 +493,18 @@ void rpc_destroy_tcp_client(struct conn_
>
> int rpc_portmap_getclient(struct conn_info *info,
> const char *host, struct sockaddr *addr, size_t addr_len,
> - const char *proto, unsigned int option)
> + int proto, unsigned int option)
> {
> - struct protoent *pe_proto;
> CLIENT *client;
> int ret;
>
> - pe_proto = getprotobyname(proto);
> - if (!pe_proto)
> - return -ENOENT;
> -
> info->host = host;
> info->addr = addr;
> info->addr_len = addr_len;
> info->program = PMAPPROG;
> info->port = PMAPPORT;
> info->version = PMAPVERS;
> - info->proto = pe_proto;
> + info->proto = proto;
> info->send_sz = RPCSMALLMSGSIZE;
> info->recv_sz = RPCSMALLMSGSIZE;
> info->timeout.tv_sec = PMAP_TOUT_UDP;
> @@ -527,7 +512,7 @@ int rpc_portmap_getclient(struct conn_in
> info->close_option = option;
> info->client = NULL;
>
> - if (pe_proto->p_proto == IPPROTO_TCP)
> + if (info->proto == IPPROTO_TCP)
> info->timeout.tv_sec = PMAP_TOUT_TCP;
>
> ret = create_client(info, &client);
> @@ -545,7 +530,7 @@ int rpc_portmap_getport(struct conn_info
> struct conn_info pmap_info;
> CLIENT *client;
> enum clnt_stat status;
> - int proto = info->proto->p_proto;
> + int proto = info->proto;
> int ret;
>
> memset(&pmap_info, 0, sizeof(struct conn_info));
> @@ -623,13 +608,13 @@ int rpc_ping_proto(struct conn_info *inf
> {
> CLIENT *client;
> enum clnt_stat status;
> - int proto = info->proto->p_proto;
> + int proto = info->proto;
> int ret;
>
> if (info->client)
> client = info->client;
> else {
> - if (info->proto->p_proto == IPPROTO_UDP) {
> + if (info->proto == IPPROTO_UDP) {
> info->send_sz = UDPMSGSIZE;
> info->recv_sz = UDPMSGSIZE;
> }
> @@ -678,7 +663,7 @@ int rpc_ping_proto(struct conn_info *inf
>
> static unsigned int __rpc_ping(const char *host,
> unsigned long version,
> - char *proto,
> + int proto,
> long seconds, long micros,
> unsigned int option)
> {
> @@ -686,6 +671,7 @@ static unsigned int __rpc_ping(const cha
> struct conn_info info;
> struct pmap parms;
>
> + info.proto = proto;
> info.host = host;
> info.addr = NULL;
> info.addr_len = 0;
> @@ -700,13 +686,9 @@ static unsigned int __rpc_ping(const cha
>
> status = RPC_PING_FAIL;
>
> - info.proto = getprotobyname(proto);
> - if (!info.proto)
> - return status;
> -
> parms.pm_prog = NFS_PROGRAM;
> parms.pm_vers = version;
> - parms.pm_prot = info.proto->p_proto;
> + parms.pm_prot = info.proto;
> parms.pm_port = 0;
>
> status = rpc_portmap_getport(&info, &parms, &info.port);
> @@ -724,19 +706,19 @@ int rpc_ping(const char *host, long seco
> unsigned long vers2 = NFS2_VERSION;
> unsigned int status;
>
> - status = __rpc_ping(host, vers2, "udp", seconds, micros, option);
> + status = __rpc_ping(host, vers2, IPPROTO_UDP, seconds, micros, option);
> if (status > 0)
> return RPC_PING_V2 | RPC_PING_UDP;
>
> - status = __rpc_ping(host, vers3, "udp", seconds, micros, option);
> + status = __rpc_ping(host, vers3, IPPROTO_UDP, seconds, micros, option);
> if (status > 0)
> return RPC_PING_V3 | RPC_PING_UDP;
>
> - status = __rpc_ping(host, vers2, "tcp", seconds, micros, option);
> + status = __rpc_ping(host, vers2, IPPROTO_TCP, seconds, micros, option);
> if (status > 0)
> return RPC_PING_V2 | RPC_PING_TCP;
>
> - status = __rpc_ping(host, vers3, "tcp", seconds, micros, option);
> + status = __rpc_ping(host, vers3, IPPROTO_TCP, seconds, micros, option);
> if (status > 0)
> return RPC_PING_V3 | RPC_PING_TCP;
>
> @@ -759,11 +741,10 @@ int rpc_time(const char *host,
> double taken;
> struct timeval start, end;
> struct timezone tz;
> - char *proto = (ping_proto & RPC_PING_UDP) ? "udp" : "tcp";
> unsigned long vers = ping_vers;
>
> gettimeofday(&start, &tz);
> - status = __rpc_ping(host, vers, proto, seconds, micros, option);
> + status = __rpc_ping(host, vers, ping_proto, seconds, micros, option);
> gettimeofday(&end, &tz);
>
> if (status == RPC_PING_FAIL || status < 0)
> @@ -781,12 +762,12 @@ static int rpc_get_exports_proto(struct
> {
> CLIENT *client;
> enum clnt_stat status;
> - int proto = info->proto->p_proto;
> + int proto = info->proto;
> unsigned int option = info->close_option;
> int vers_entry;
> int ret;
>
> - if (info->proto->p_proto == IPPROTO_UDP) {
> + if (info->proto == IPPROTO_UDP) {
> info->send_sz = UDPMSGSIZE;
> info->recv_sz = UDPMSGSIZE;
> }
> @@ -893,11 +874,9 @@ exports rpc_get_exports(const char *host
> parms.pm_port = 0;
>
> /* Try UDP first */
> - info.proto = getprotobyname("udp");
> - if (!info.proto)
> - goto try_tcp;
> + info.proto = IPPROTO_UDP;
>
> - parms.pm_prot = info.proto->p_proto;
> + parms.pm_prot = info.proto;
>
> status = rpc_portmap_getport(&info, &parms, &info.port);
> if (status < 0)
> @@ -910,11 +889,9 @@ exports rpc_get_exports(const char *host
> return exportlist;
>
> try_tcp:
> - info.proto = getprotobyname("tcp");
> - if (!info.proto)
> - return NULL;
> + info.proto = IPPROTO_TCP;
>
> - parms.pm_prot = info.proto->p_proto;
> + parms.pm_prot = info.proto;
>
> status = rpc_portmap_getport(&info, &parms, &info.port);
> if (status < 0)
Hello Ian,
What about this patch, do you think it can be committed?
Thanks,
Leonardo
--
To unsubscribe from this list: send the line "unsubscribe autofs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
