4:42 a.m.
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