Hi Jukka,
On Tue, Sep 16, 2014 at 04:20:19PM +0300, Jukka Rissanen wrote:
> Hi Alex,
>
> On ti, 2014-09-16 at 14:48 +0200, Alexander Aring wrote:
> > Hi Martin,
> >
> > On Tue, Sep 16, 2014 at 01:40:24PM +0100, Martin Townsend wrote:
> > ...
> > >
> > > Yes I see the problem now, maybe it's better to revert back to skb_inout, less chance of introducing bugs and then we have a well defined return value.
> > >
> >
> > No problem, for me it's okay, if this is okay for Jukka, we can change
> > it later to a better behaviour. Jukka please answer what you think about this.
> >
>
> What about doing things like this in your example?
>
ehm yes, the example is only there to describe the current situation.
> > I also did a small c example because this now:
> >
> > char *foo(char *buf)
> > {
> > char *new;
> >
> > if (some_error)
> > return NULL;
>
> In this case you should probably not return NULL but something like
> -EINVAL
>
> if (some_error) {
> free(buf);
> return -EINVAL;
> }
yes, that's the second choice, let do consume_skb/kfree_skb inside
lowpan_process_data function.
>
> >
> > if (some_error)
> > return NULL;
>
> Ditto
>
> >
> > new = expand(buf, 23);
> > if (!new)
> > return NULL;
>
> if (!new) {
> free(buf);
> return -ENOMEM;
> }
>
> >
> > free(buf);
> > buf = new;
> >
> > /* buf is now different than the parameter buf */
> > if (some_error)
> > return NULL;
>
> if (some_error) {
> free(buf);
> return -EFOOBAR;
> }
>
> >
> > return buf;
> > }
> >
> > int main(int argc, const char *argv[])
> > {
> > char *local_buf = malloc(42);
> > char *buf;
> >
> > buf = foo(local_buf);
> > if (!buf) {
> > /* BUG */
> > /* we don't know if local_buf is still valid. */
> > free(local_buf);
> > }
>
> if (IS_ERR_OR_NULL(buf)) {
> fail();
> } else
> free(buf);
>
> >
> > return 0;
> > }
> >
> > I think if you do buf = foo(buf) you can rescue it but this doesn't
> > look like a clean solution for me.
> >
> > - Alex
>
>
> In this simplified example, the subroutine frees the buf which does not
> look nice I have to admit.
>
I am also fine with this solution. Make something I will review it and
look if we run into trouble.
In my last mails stands, that we have two choices:
- make the skb_inout thingy
- handle error freeing into lowpan_process_data function.
You described the last one now. :-)
- Alex
--
To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
