Re: [PATCH v4 bluetooth] 6lowpan: fix incorrect return values in lowpan_rcv

[Jukka Rissanen <jukka.rissanen@linux.intel.com>]

Hi Alex,

On ti, 2014-09-16 at 15:32 +0200, Alexander Aring wrote:
> 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. :-)

Great, your example clarified the issue nicely :)

I would vote for option 2) but if it makes the code too ugly then 1) is
ok too.


Cheers,
Jukka