Re: [PATCH 8/11] staging: lustre: obdclass: Use kzalloc and kfree

[Julia Lawall <julia.lawall@lip6.fr>]

On Fri, 1 May 2015, walter harms wrote:

> hi Julia,
> your patch seems fine.
> I tried to understand the code and it seems that much of it
> can be simplified by using already available functions.
> I have added some comments but i am not sure what to make of it.

Thanks for the review.  Comments below.

> >  
> >  	len = strlen(LUSTRE_MGC_OBDNAME) + strlen(libcfs_nid2str(nid)) + 1;
> > -	OBD_ALLOC(mgcname, len);
> > -	OBD_ALLOC(niduuid, len + 2);
> > +	mgcname = kzalloc(len, GFP_NOFS);
> > +	niduuid = kzalloc(len + 2, GFP_NOFS);
> >  	if (!mgcname || !niduuid) {
> >  		rc = -ENOMEM;
> >  		goto out_free;
> 
>  this can be simplified by using
>    kasprintf(&mgcname,"%s%s", LUSTRE_MGC_OBDNAME, libcfs_nid2str(nid));
> 
>  is guess the some is true for niduuid

Thanks for the suggestion.  I will look into that next.  It may be 
applicable elsewhere.

> >  	/* Save the obdname for cleaning the nid uuids, which are
> >  	   obdname_XX */
> >  	len = strlen(obd->obd_name) + 6;
> > -	OBD_ALLOC(niduuid, len);
> > +	niduuid = kzalloc(len, GFP_NOFS);
> >  	if (niduuid) {
> >  		strcpy(niduuid, obd->obd_name);
> >  		ptr = niduuid + strlen(niduuid);
> 
> 	i guess kstrdup() would be appropiate

OK, I will check on this too.

> > @@ -895,7 +887,7 @@ static int lmd_parse_mgssec(struct lustr
> >  	int     length;
> >  
> >  	if (lmd->lmd_mgssec != NULL) {
> > -		OBD_FREE(lmd->lmd_mgssec, strlen(lmd->lmd_mgssec) + 1);
> > +		kfree(lmd->lmd_mgssec);
> >  		lmd->lmd_mgssec = NULL;
> >  	}
> 
> is the check needed hier at all ? just
> kfree(lmd->lmd_mgssec);
> seems to do the same job.

I'm working on that right at the moment.  Patch shortly.

> 
> >  
> > @@ -905,7 +897,7 @@ static int lmd_parse_mgssec(struct lustr
> >  	else
> >  		length = tail - ptr;
> >  
> > -	OBD_ALLOC(lmd->lmd_mgssec, length + 1);
> > +	lmd->lmd_mgssec = kzalloc(length + 1, GFP_NOFS);
> >  	if (lmd->lmd_mgssec == NULL)
> >  		return -ENOMEM;
> >  
> 
> 	complicated why to say:
> 	lmd->lmd_mgssec=kstrndup(ptr, length,GFP_NOFS);

OK, I will look into it.

> > @@ -933,7 +925,7 @@ static int lmd_parse_string(char **handl
> >  	else
> >  		length = tail - ptr;
> >  
> > -	OBD_ALLOC(*handle, length + 1);
> > +	*handle = kzalloc(length + 1, GFP_NOFS);
> >  	if (*handle == NULL)
> >  		return -ENOMEM;
> >  
> 
> lmd_parse_string() seems more or less the same as lmd_parse_mgssec().
> perhaps this can be merged.

I will check.

> > @@ -971,7 +963,7 @@ static int lmd_parse_mgs(struct lustre_m
> >  		/* Multiple mgsnid= are taken to mean failover locations */
> >  		memcpy(mgsnid, lmd->lmd_mgs, oldlen);
> >  		mgsnid[oldlen - 1] = ':';
> > -		OBD_FREE(lmd->lmd_mgs, oldlen);
> > +		kfree(lmd->lmd_mgs);
> >  	}
> >  	memcpy(mgsnid + oldlen, *ptr, length);
> >  	mgsnid[oldlen + length] = '\0';
> 
> the code lmd_parse_mgs basicly does:
>   kasprintf( &lmd->lmd_mgs,"%s:%s",lmd->lmd_mgs,*ptr);

OK.

thanks,
julia