hi holger,
these patches are not the "final" patch. some things must be discussed before of yourse. i removed the out commented code from the patch. some comments like "// todo ..." are not removed. when you look at the file http://home.eversberg.eu/stage1.html, you will see the new version. whenever there is something to complain about, i will change it, until we have patches that all of us agree.
please ask if you don't fully understand the patch. especially for the BSC<->application interface, i was too lazy to describe all details. you will find mncc.c usefull to see how the interface is handled.
regards,
andres
-----Ursprüngliche Nachricht----- Von: openbsc-bounces@lists.gnumonks.org [mailto:openbsc-bounces@lists.gnumonks.org] Im Auftrag von Holger Freyther Gesendet: Sonntag, 17. Mai 2009 09:35 An: openbsc@lists.gnumonks.org Betreff: Re: patches for openbsc
On Friday 15 May 2009 12:37:50 Andreas.Eversberg wrote:
hi,
finally my patch is documented (almost). you will find the documentation of the patch:
http://home.eversberg.eu/stage1.html
the patch in plain text fromat:
http://home.eversberg.eu/stage1.patch
it also includes the changes i posted here before. if you have questions, ask.
Hi Andreas,
thank you for the patches. Let me start to review the patches one by one as replies to this one. One general thing with this and the previous patch. I'm quite picky when it comes to commented out code, and left overs from debugging.... please try to avoid leaving that in.
z.
On Sunday 17 May 2009 17:13:24 Andreas.Eversberg wrote:
hi holger,
these patches are not the "final" patch. some things must be discussed before of yourse. i removed the out commented code from the patch. some comments like "// todo ..." are not removed. when you look at the file http://home.eversberg.eu/stage1.html, you will see the new version. whenever there is something to complain about, i will change it, until we have patches that all of us agree.
please ask if you don't fully understand the patch. especially for the BSC<->application interface, i was too lazy to describe all details. you will find mncc.c usefull to see how the interface is handled.
Sorry,
we don't get anywhere with this. How should I comment on the stage1.html? Send you a diff against html? Currently it is way too much work to pick the good parts by hand and leave the other stuff out, I would really welcome patches one could just apply.
Examples:
1.) Installation of the lib...
NO! Not via an install-data-hook... If you want the lib to be installed change it away from a noinst lib. This should honor the --prefix one is giving when configuring... this is only leaving the includes...
And please don't mix changing the noinst to a proper library and adding new files... They have nothing to do with each other...
2.) ... Add mncc.c in one go and not by sprinkling changes around...
3.) Adding the timer. The change looks fine, we will need to test it on real hardware... BUT it is depending on your rename of the timer functions introduced later... maybe it is time for using git and git-format-patch? So yes, patch is okay, but we can not just apply it because of its dependencies (timer rename, new trau include, ...) It would also be appreciated if you can rediff this part as the debug statements have been added... And you have unrelated changes like the autorelease within this patch, something I have merged to openbsc by hand already...
And with the change of input/misdn.c you try to sneak in a mISDNuser dependency without changing the configure.in. I already asked why is this needed? What is the benefit?
4.) Skipped for now
5.) Okay, should be something like the new 3rd patch as 3rd) is depending on this one. All fine... but the title of the patch does not match the content. The renaming has nothing to do with the change of semantic of this function. So we need one patch for renaming the methods (to not pollute the global namespace...) + users of it and one for changing the semantic + user of it.
6.) Skipped for now
7.) Don't you miss a change to include/openbsc/gsm_data.h? Currently the bts is passed in terms of the data (or within a struct for data), can't this work for for you?
sorry, need to stop here right now, I see most of your work is in the NMCC change... so for now what about trying to get 1-7 resolved and progress on these items?
z.
On Mon, 18 May 2009 06:14:03 +0200 Holger Freyther zecke@selfish.org wrote:
we don't get anywhere with this. How should I comment on the stage1.html? Send you a diff against html? Currently it is way too much work to pick the good parts by hand and leave the other stuff out, I would really welcome patches one could just apply.
There's a wiki. Why don't you just write all the stuff into it? Way easier to work on.
cheers,
Dear Andreas,
first of all, as I indicated in the past, I am very thankful for the amount of work you are committning to OpenBSC. So please always keep this in mind!
Dear Holger,
we all bring different skills to this project. Andreas' strength is that he has a lot of knowledge of traditional ITU-T/ETSI telephony protocols, something that we clearly don't have. Apart from all his work, he actually also documented his work in detail - even if in a method quite uncommonly seen in FOSS projects for development discussion (colorful HTML on his website).
You on the other hand (like me) have extensive experience in participating in large FOSS projects that follow certain rules and 'best practise' when it comes to the structure of contributions.
Andreas is trying to help us with his code, so please be friendly and welcoming to him. I agree with you that the mode of submitting one large patch rather than an incremental per-feature patchset is not how contributions are normally acceptd. I also agree that cosmetic/syntax changes need to be kept separate from actual semantic changes of the code, or that autotools should be used the "right" way - but we can communicate that in a nicer way.
Andreas, I hope you will bear with us and go through the process of splitting up your patches. I know it is the least exciting part of anyone's work, but I believe it is an important skill to have when working within the FOSS community.
What I would like to see is independent and incremental patches in the following order:
* timer rename throughout the existing code * llist header changes (why were they made? c++?) * the transmit delay timer * introduction of trau interface * bsc_select_main(polling) * paging extension with cbfn BTS pointer * the actual MNCC interface * installation of libbsc (this should be last, since before the other changes it is of no use)
after applying each of those patches individually, the code should still compile and work. Each patch should be sent as an independent patch/diff file to the openbsc-devel list, including a short description on top of the patch what it does.
If submitted this way, I can agree to merge all the infrastructure work right away, i.e. the above list up to and including the 'paging extension'. This leaves Andreas' stage1 patch limited to the MNCC interface, which I first want to find some more time to review and play with it.
Is that something we can all agree to?
Thanks.
On Tuesday 19 May 2009 04:18:57 Harald Welte wrote:
Dear Andreas,
first of all, as I indicated in the past, I am very thankful for the amount of work you are committning to OpenBSC. So please always keep this in mind!
Dear Holger,
we all bring different skills to this project. Andreas' strength is that he has a lot of knowledge of traditional ITU-T/ETSI telephony protocols, something that we clearly don't have. Apart from all his work, he actually also documented his work in detail - even if in a method quite uncommonly seen in FOSS projects for development discussion (colorful HTML on his website).
You on the other hand (like me) have extensive experience in participating in large FOSS projects that follow certain rules and 'best practise' when it comes to the structure of contributions.
Andreas is trying to help us with his code, so please be friendly and welcoming to him. I agree with you that the mode of submitting one large patch rather than an incremental per-feature patchset is not how contributions are normally acceptd. I also agree that cosmetic/syntax changes need to be kept separate from actual semantic changes of the code, or that autotools should be used the "right" way - but we can communicate that in a nicer way.
Dear Andreas,
sorry about the tone. I'm very interested in getting your changes into the OpenBSC repository as they are great improvements. My mail was not meant to be rough but was meant as a way forward to get things merged before you end up with a huge pile of changes (this was probably the reason that harald and me started to merge bits and pieces by hand as well). I sincerely agree with Haralds proposed order of changes and please let us progress this way.
z.
PS: Sorry, I went to bed early here, I try to reach you tonight
Some more bla bla on why small chunks of changes.
First of all we have all limits, by having smaller changes we are better to judge this individual change, the impact and such without reaching the limit. Having small independent changes allows us to roll-out (revert) a particular change (e.g. when our judgment was wrong) without ending in a big manual merge catastrophe. And finally by having each change compilable we can fall back to things like git bisect.
-
Andreas.Eversberg -
codec -
Harald Welte -
Holger Freyther