Re: [PATCH 0/6] PPP Patches v3

[Marcel Holtmann <marcel@holtmann.org>]

[-- Attachment #1: Type: text/plain, Size: 3176 bytes --]

Hi Kristen,

> > > > > > Changes since v2 - corrected whitespace. 
> > > > > 
> > > > > all the patches have been applied.
> > > > > 
> > > > > So form now on please send smaller patches as soon as you have them
> > > > > ready and tested. If you need comments or have questions, you can use
> > > > > the mailing list as well.
> > > > 
> > > > some additional comments from my side.
> > > > 
> > > > If you have variables that are only valid inside the scope of a
> > > > statement, then I prefer if you only declare them there. This makes the
> > > > code a bit simpler to read. Check the patch I just pushed to give you an
> > > > example.
> > > > 
> > > > Also please find access to a 32-bit system if you are running 64-bit or
> > > > vice versa. Our build system is strict and will turn turn all casting
> > > > mistakes in an error.
> > > > 
> > > > Please be present on #ofono IRC as much as possible so people can report
> > > > such things. Remember that your code is now part of the tree and it can
> > > > break the build.
> > >
> > > I just started reviewing the changes you made and the first 
> > > patch I hit I discovered that you broke something.  Would you like to
> > > revert the patches and let me review them first, or should I just send 
> > > a patch to the mailing list to fix what you broke?
> > 
> > send patches to the mailing list to fix them. The PPP code is in
> > development so it is just fine. I prefer to keep reverts for code that
> > is suppose to be stable.
> > 
> > If I did break something, then I am worried that the code gets too
> > complex. Such little changes should not have broken anything. Except I
> > made a stupid typo or thinking mistake (which happens of course). If it
> > fundamentally changes the code flow, then we do have a bigger problem.
>
> I wanted this code to at least be able to send a packet over the wire,
> and with your changes we can't do that anymore.  The problem is that
> your little cosmetic change really did change the flow, simply because
> you didn't think through what you were doing.  Of course it is up to
> you if you want to have this code be completely non-functional rather
> than simply doing the revert.  I'll be glad to submit a patch later
> which fixes your fix and adds some comments so you know what is 
> actually happening.

I am not doing the revert since I do wanna figure out what broke it. And
actually I wanna have that recorded in the tree.

So Denis and I looked through my changes and he figured out what might
have broken it. So your attempt at doing code flow optimization with
g_list_length() throw me on the wrong error path. Please don't do this
since all the GLib functions will do proper empty list checking.

I fixed this now in the tree and it should restore it to a working code
base. I prefer if you don't do things like GList *list = NULL. These
initialization of variables lead to some complex code flow. And if you
do something wrong, the compiler will never warn.

Also in general it is preferred to do list == NULL checking instead of
using g_list_legnth() functions.

Regards

Marcel