On Fri, 2013-01-04 at 15:25 -0200, Leonardo Chiquitto wrote:
> 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?
Yep, I think this is the best thing to do. I can't find the original
message which has the patch and this message has all sorts of wrap going
on. Could you send the patch again please.
>
> 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
--
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
