Re: [Libtirpc-devel] [PATCH 1/3] Make it clear rpc_createerr is thread safe

[Ian Kent <ikent@redhat.com>]

On Thu, 2016-05-19 at 12:35 -0300, Paulo Andrade wrote:
>   Avoid hidding it under a macro, and also avoid multiple function
> calls when accessing structure fields.

I like this approach, the use of the macro was confusing to me.
Including that define must always be the case though.

If others agree with this there probably needs to be follow up series with
similar changes for the rest of the source.

I guess the main argument against it is that library users have come to expect
using the macro and could be confused by this change, the exact opposite to the
point of the change.

Thoughts anyone?

> 
> Signed-off-by: Paulo Andrade <pcpa@gnu.org>
> ---
>  src/clnt_vc.c | 17 +++++++++++------
>  1 file changed, 11 insertions(+), 6 deletions(-)
> 
> diff --git a/src/clnt_vc.c b/src/clnt_vc.c
> index a72f9f7..8af7ddd 100644
> --- a/src/clnt_vc.c
> +++ b/src/clnt_vc.c
> @@ -72,6 +72,8 @@
>  #define CMGROUP_MAX    16
>  #define SCM_CREDS      0x03            /* process creds (struct cmsgcred) */
>  
> +#undef rpc_createerr                   /* make it clear it is a thread safe
> variable */
> +
>  /*
>   * Credentials structure, used to verify the identity of a peer
>   * process that has sent us a message. This is allocated by the
> @@ -188,10 +190,11 @@ clnt_vc_create(fd, raddr, prog, vers, sendsz, recvsz)
>  	cl = (CLIENT *)mem_alloc(sizeof (*cl));
>  	ct = (struct ct_data *)mem_alloc(sizeof (*ct));
>  	if ((cl == (CLIENT *)NULL) || (ct == (struct ct_data *)NULL)) {
> +		struct rpc_createerr *ce = &get_rpc_createerr();
>  		(void) syslog(LOG_ERR, clnt_vc_errstr,
>  		    clnt_vc_str, __no_mem_str);
> -		rpc_createerr.cf_stat = RPC_SYSTEMERROR;
> -		rpc_createerr.cf_error.re_errno = errno;
> +		ce->cf_stat = RPC_SYSTEMERROR;
> +		ce->cf_error.re_errno = errno;
>  		goto err;
>  	}
>  	ct->ct_addr.buf = NULL;
> @@ -235,15 +238,17 @@ clnt_vc_create(fd, raddr, prog, vers, sendsz, recvsz)
>  	slen = sizeof ss;
>  	if (getpeername(fd, (struct sockaddr *)&ss, &slen) < 0) {
>  		if (errno != ENOTCONN) {
> -			rpc_createerr.cf_stat = RPC_SYSTEMERROR;
> -			rpc_createerr.cf_error.re_errno = errno;
> +			struct rpc_createerr *ce = &get_rpc_createerr();
> +			ce->cf_stat = RPC_SYSTEMERROR;
> +			ce->cf_error.re_errno = errno;
>  			mutex_unlock(&clnt_fd_lock);
>  			thr_sigsetmask(SIG_SETMASK, &(mask), NULL);
>  			goto err;
>  		}
>  		if (connect(fd, (struct sockaddr *)raddr->buf, raddr->len) <
> 0){
> -			rpc_createerr.cf_stat = RPC_SYSTEMERROR;
> -			rpc_createerr.cf_error.re_errno = errno;
> +			struct rpc_createerr *ce = &get_rpc_createerr();
> +			ce->cf_stat = RPC_SYSTEMERROR;
> +			ce->cf_error.re_errno = errno;
>  			mutex_unlock(&clnt_fd_lock);
>  			thr_sigsetmask(SIG_SETMASK, &(mask), NULL);
>  			goto err;