On 04/30/2012 05:37 PM, Frank Maas wrote:
Dear Frans,
thanks a lot for your work, I would like to encourage you to revise the patch
in several ways. There are some soft factors like using tabs, following the
kernel coding style (e.g. not if and msgb_free) on the same line but also some
technical issues.
>From my point of view the root cause of this issue is that we are inconsistent
of what if the msgb is 'borrowed' or 'owned/transferred'. Whenever we reach a
_send method we actually transfer the ownership (as the data might be queued
or such).
The other technical part is to make sure we first establish a rule by
1.) adding doxygen/API documentation to the sendmsg function
2.) maybe look into introducing dummy annotations like __borrow, __takes that
we could autocheck in the future (e.g. by extending spaze/smack)
The issue with reviewing such a patch is the question if you have catched
everything. E.g. I think you missed need to convert (and what it calls):
static int gprs_ns_tx(struct gprs_nsvc *nsvc, struct msgb *msg)
{
...
default:
LOGP(DNS, LOGL_ERROR, "unsupported NS linklayer %u\n",
nsvc->ll);
msgb_free(msg);
ret = -EIO;
break;
}
return ret;
}
> As reported in ticket #55 SGSN can crash due to double free-ing. You
> can replace 'can' by 'will' in that last phrase. I had a sift through
> the code and tried to solve this by removing the free in gprs_ns.c.
> Whenever the calling function created the msgb-struct, I have made the
> function free it after its use. If the function got the msgb from a
> calling function, there will not be a free (hoping that will be done
> on the higher level).
>
> HTH/F
