[PATCH] fix for encoding of padding bits

Harald Welte laforge at gnumonks.org
Wed Feb 24 12:01:20 UTC 2016


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



More information about the osmocom-net-gprs mailing list