Re: [PATCH 1/2] Teach clients to map numeric strings into valid uids and gids.

[Trond Myklebust <trond.myklebust@fys.uio.no>]

On Tue, 2010-08-17 at 18:07 -0400, Steve Dickson wrote:
> 
> On 08/17/2010 05:47 PM, Trond Myklebust wrote:
> > On Tue, 2010-08-17 at 17:35 -0400, Steve Dickson wrote:
> >>
> >> On 08/17/2010 04:27 PM, Trond Myklebust wrote:
> >>> On Tue, 2010-08-17 at 15:38 -0400, Steve Dickson wrote:
> >>>> When a server can not map a uid or gid they can send
> >>>> an numeric string (one that does not include the @domain)
> >>>> of the given id. When this occurs the client will can use
> >>>> that numeric representation iff that id exists on the client.
> >>>>
> >>>> Signed-off-by: Steve Dickson <steved@redhat.com>
> >>>> ---
> >>>>  utils/idmapd/idmapd.c |   38 ++++++++++++++++++++++++++++++++++++++
> >>>>  1 files changed, 38 insertions(+), 0 deletions(-)
> >>>>
> >>>> diff --git a/utils/idmapd/idmapd.c b/utils/idmapd/idmapd.c
> >>>> index 9ecab66..d6799c9 100644
> >>>> --- a/utils/idmapd/idmapd.c
> >>>> +++ b/utils/idmapd/idmapd.c
> >>>> @@ -826,6 +826,21 @@ idtonameres(struct idmap_msg *im)
> >>>>  		im->im_status = IDMAP_STATUS_SUCCESS;
> >>>>  }
> >>>>  
> >>>> +static int
> >>>> +alldigits(char *name)
> >>>> +{
> >>>> +	char *p = name;
> >>>> +
> >>>> +	if (name == NULL)
> >>>> +		return 0;
> >>>> +
> >>>> +	while (*p != '\0') {
> >>>> +		if (!isdigit(*p++))
> >>>> +			return 0;
> >>>> +	}
> >>>> +	return 1;
> >>>> +}
> >>>> +
> >>>>  static void
> >>>>  nametoidres(struct idmap_msg *im)
> >>>>  {
> >>>> @@ -843,6 +858,18 @@ nametoidres(struct idmap_msg *im)
> >>>>  		ret = nfs4_name_to_uid(im->im_name, &uid);
> >>>>  		im->im_id = (u_int32_t) uid;
> >>>>  		if (ret) {
> >>>> +			/*
> >>>> +			 * When a server sends numeric string that's
> >>>> +			 * lacking the @domain part, and the uid exists
> >>>> +			 * use the numeric string as the uid. 
> >>>> +			 */
> >>>> +			if (alldigits(im->im_name)) {
> >>>> +				sscanf(im->im_name, "%u", &uid);
> >>>> +				if (getpwuid(uid) != NULL) {
> >>>> +					im->im_id = uid;	
> >>>> +					break;
> >>>> +				}
> >>>> +			}
> >>>>  			im->im_status = IDMAP_STATUS_LOOKUPFAIL;
> >>>>  			im->im_id = nobodyuid;
> >>>>  		}
> >>>> @@ -851,6 +878,17 @@ nametoidres(struct idmap_msg *im)
> >>>>  		ret = nfs4_name_to_gid(im->im_name, &gid);
> >>>>  		im->im_id = (u_int32_t) gid;
> >>>>  		if (ret) {
> >>>> +			/*
> >>>> +			 * Do the same type of mapping of a numeric string
> >>>> +			 * sent as the gid.
> >>>> +			 */
> >>>> +			if (alldigits(im->im_name)) {
> >>>> +				sscanf(im->im_name, "%u", &gid);
> >>>> +				if (getgrgid(gid) != NULL) {
> >>>> +					im->im_id = gid;
> >>>> +					break;
> >>>> +				}
> >>>> +			}
> >>>>  			im->im_status = IDMAP_STATUS_LOOKUPFAIL;
> >>>>  			im->im_id = nobodygid;
> >>>>  		}
> >>>
> >>> Wouldn't it make more sense to put this into libnfsidmap instead?
> >> I did think of that but since the nobody ids are being set here I 
> >> figured I made the most sense to do here... plus this is pretty 
> >> simple code why hide a way in the library and make it more 
> >> difficult to debug... 
> > 
> > It means that Bryan is going to have to duplicate it all in the new
> > highly scalable idmapper.
> Hmm... I guess I didn't realize there was a "highly scalable idmapper" on
> drawing board... Cool... Any details... Did I miss some email?? 

Not yet. He just finished the first draft today.

Bryan is basically reusing the keyring based design for the dns resolver
and applying it to idmapping.

> > 
> > Ditto for the nobody ids.
> > 
> > What's the point of having the library when it doesn't do the full NFSv4
> > idmapping for you?
> At this point we don't do the nobody id mapping in the library... I'm just
> going with what is there now.. I do see and understand your point about doing 
> all id mapping in the library... and with the new scalable idmapper,  move the 
> code... I strongly suggest you do... 

Ok...

> But at this point in time we have people doing the v4 transitioning now and
> I can get the fix out much quicker, making their transition less painful,
> if I put these few lines of code in nfs-utils verses the library... 

I don't see why getting it into nfs-utils is faster. Is CITI being slow
taking patches for libnfsidmap?