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
--
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
