Re: [PATCH] idmapd: rearm event handler after error in nfsdcb()

["J. Bruce Fields" <bfields@fieldses.org>]

On Thu, Sep 10, 2009 at 03:42:07PM -0400, Jeff Layton wrote:
> A couple of years ago, Bruce committed a patch to make knfsd send
> unsigned uid's and gid's to idmapd, rather than signed values. Part
> of that earlier discussion is here:
> 
> http://linux-nfs.org/pipermail/nfsv4/2007-December/007321.html
> 
> While this fixed the immediate problem, it doesn't appear that anything
> was ever done to make idmapd continue working when it gets a bogus
> upcall.

Thanks for following up on this!

> 
> idmapd uses libevent for its main event handling loop. When idmapd gets
> an upcall from knfsd it will service the request and then rearm the
> event by calling event_add on the event structure again.
> 
> When it hits an error though, it returns in most cases w/o rearming the
> event. That prevents idmapd from servicing any further requests from
> knfsd.
> 
> I've made another change too. If an error is encountered while reading
> the channel file, this patch has it close and reopen the file prior to
> rearming the event.
> 
> I've not been able to test this patch directly, but I have tested a
> backport of it to earlier idmapd code and verified that it did prevent
> idmapd from hanging when it got a badly formatted upcall from knfsd.
> 
> Signed-off-by: Jeff Layton <jlayton@redhat.com>
> ---
>  utils/idmapd/idmapd.c |   18 ++++++++++--------
>  1 files changed, 10 insertions(+), 8 deletions(-)
> 
> diff --git a/utils/idmapd/idmapd.c b/utils/idmapd/idmapd.c
> index 65a6a2a..573abaa 100644
> --- a/utils/idmapd/idmapd.c
> +++ b/utils/idmapd/idmapd.c
> @@ -139,6 +139,7 @@ static void nametoidres(struct idmap_msg *);
>  
>  static int nfsdopen(void);
>  static int nfsdopenone(struct idmap_client *);
> +static void nfsdreopen_one(struct idmap_client *);
>  static void nfsdreopen(void);
>  
>  size_t  strlcat(char *, const char *, size_t);
> @@ -502,7 +503,8 @@ nfsdcb(int fd, short which, void *data)
>  		xlog_warn("nfsdcb: read(%s) failed: errno %d (%s)",
>  			     ic->ic_path, len?errno:0, 
>  			     len?strerror(errno):"End of File");
> -		goto out;
> +		nfsdreopen_one(ic);
> +		return;

Why the "return"?  From your description above ("... close and reopen
the file prior to rearming the event"), I would have thought you'd want
the final event_add() that's done at "out".

--b.

>  	}
>  
>  	/* Get rid of newline and terminate buffer*/
> @@ -514,11 +516,11 @@ nfsdcb(int fd, short which, void *data)
>  	/* Authentication name -- ignored for now*/
>  	if (getfield(&bp, authbuf, sizeof(authbuf)) == -1) {
>  		xlog_warn("nfsdcb: bad authentication name in upcall\n");
> -		return;
> +		goto out;
>  	}
>  	if (getfield(&bp, typebuf, sizeof(typebuf)) == -1) {
>  		xlog_warn("nfsdcb: bad type in upcall\n");
> -		return;
> +		goto out;
>  	}
>  	if (verbose > 0)
>  		xlog_warn("nfsdcb: authbuf=%s authtype=%s",
> @@ -532,26 +534,26 @@ nfsdcb(int fd, short which, void *data)
>  		im.im_conv = IDMAP_CONV_NAMETOID;
>  		if (getfield(&bp, im.im_name, sizeof(im.im_name)) == -1) {
>  			xlog_warn("nfsdcb: bad name in upcall\n");
> -			return;
> +			goto out;
>  		}
>  		break;
>  	case IC_IDNAME:
>  		im.im_conv = IDMAP_CONV_IDTONAME;
>  		if (getfield(&bp, buf1, sizeof(buf1)) == -1) {
>  			xlog_warn("nfsdcb: bad id in upcall\n");
> -			return;
> +			goto out;
>  		}
>  		tmp = strtoul(buf1, (char **)NULL, 10);
>  		im.im_id = (u_int32_t)tmp;
>  		if ((tmp == ULONG_MAX && errno == ERANGE)
>  				|| (unsigned long)im.im_id != tmp) {
>  			xlog_warn("nfsdcb: id '%s' too big!\n", buf1);
> -			return;
> +			goto out;
>  		}
>  		break;
>  	default:
>  		xlog_warn("nfsdcb: Unknown which type %d", ic->ic_which);
> -		return;
> +		goto out;
>  	}
>  
>  	imconv(ic, &im);
> @@ -612,7 +614,7 @@ nfsdcb(int fd, short which, void *data)
>  		break;
>  	default:
>  		xlog_warn("nfsdcb: Unknown which type %d", ic->ic_which);
> -		return;
> +		goto out;
>  	}
>  
>  	bsiz = sizeof(buf) - bsiz;
> -- 
> 1.5.5.6
> 
> _______________________________________________
> NFSv4 mailing list
> NFSv4@linux-nfs.org
> http://linux-nfs.org/cgi-bin/mailman/listinfo/nfsv4