This is merely a historical archive of years 2008-2021, before the migration to mailman3.
A maintained and still updated list archive can be found at https://lists.osmocom.org/hyperkitty/list/osmocom-net-gprs@lists.osmocom.org/.
Harald Welte laforge at gnumonks.orgHi Saurabh, On Wed, Feb 24, 2016 at 06:52:58AM +0000, Saurabh Sharan wrote: > Subject: [PATCH] fix for encoding of padding bits 1) no actual commit log message beyond the subjcet line So you have a single-line commit message without further description. This may be acceptable in some cases where it is completly ovious (like a costmetic fix, or a == / != change, ...). But at least for the change you are proposing, it is not obvious to me what the problem was, and why. Sure, I see you are shifting it right one bit further. Probably due to the misgsing '0' ahead of the filler? A single sentence explaining that would be great. 2) encoding bugfix without a test caes especially with bugs affecting the encoding of messages, it would be very useful to have a small test case as part of the unit testing suite of osmo-pcu. The test should fail with the old behavior, and be fixed with the new behavior. By introducing the test case we make sure to avoid future regressions. The test case should be submitted as a separate patch/commit, independent of the actual bugfix. 3) cosmetics > + /*section 11 of 44.060 > + *The padding bits may be the 'null' string. Otherwise, the > + *padding bits starts with bit '0', followed by 'spare padding'. > + *< padding bits > ::= { null | 0 < spare padding > ! < Ignore : 1 bit** = < no string > > } ; > + */ > + guint8 fl = filler&(0xff>>(8-bits_to_handle + 1)); Several cosmetic issues with this: * no space between '/*' and start of comment * whitespace/tab doesn't align right, i.e. different lines have different amount of tabs/spaces ending up in misaligned display. The file src/csn1.cpp was originally taken from the wireshark project and is thus following a completely different coding style. They don't use tabs at all, but use spaces only to indent their lines. That's unlike the other source code files in the osmo-pcu, and we should probably convert it - but that's a separate discussion. In general it's best to follow the style of the surrounding code that you're editing. Try 'git log' when the patch is applied an you will see what I mean. * extra white spaces at the end of two lines in the comment, which is actually something even git is issuing a warning about when I try to apply it: ---- Applying: fix for encoding of padding bits .git/rebase-apply/patch:16: trailing whitespace. .git/rebase-apply/patch:17: trailing whitespace. warning: 2 lines add whitespace errors. ---- Now all of the above might see a bit excessive if after all we want to make such a minor change. But I think it's important to get the procedure/workflow right, particularly with further future patches in mind. Thanks for your understanding. Regards, Harald -- - Harald Welte <laforge at gnumonks.org> http://laforge.gnumonks.org/ ============================================================================ "Privacy in residential applications is a desirable marketing option." (ETSI EN 300 175-7 Ch. A6)