Hello Harald, We had tried the option of sending one patch through git send-email. This was our first attempt so wanted to confirm whether we followed the process correctly. Please note that I am unable to see this patch(mail below) in Archive maintained at http://lists.osmocom.org/pipermail/osmocom-net-gprs/
Regards Saurabh
-----Original Message----- From: Saurabh Sharan [mailto:saurabh.sharan@radisys.com] Sent: Tuesday, February 23, 2016 4:29 PM To: osmocom-net-gprs@lists.osmocom.org Cc: Saurabh Sharan Saurabh.Sharan@radisys.com Subject: [PATCH] fix for encoding of padding bits
--- src/csn1.cpp | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/src/csn1.cpp b/src/csn1.cpp index 54cc411..9262c81 100644 --- a/src/csn1.cpp +++ b/src/csn1.cpp @@ -2400,7 +2400,12 @@ gint16 csnStreamEncoder(csnStream_t* ar, const CSN_DESCR* pDescr, bitvec *vector guint8 bits_to_handle = remaining_bits_len%8; if (bits_to_handle > 0) { - guint8 fl = filler&(0xff>>(8-bits_to_handle)); + /*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)); bitvec_write_field(vector, writeIndex, fl, bits_to_handle); LOGPC(DCSN1, LOGL_NOTICE, "%u|", fl); remaining_bits_len -= bits_to_handle; -- 1.7.9.5
Hi Saurabh,
On Wed, Feb 24, 2016 at 06:52:58AM +0000, Saurabh Sharan wrote:
We had tried the option of sending one patch through git send-email. This was our first attempt so wanted to confirm whether we followed the process correctly. Please note that I am unable to see this patch(mail below) in Archive maintained at http://lists.osmocom.org/pipermail/osmocom-net-gprs/
Indeed, the mail did not arrive here. Please check your git send-email configuration. By default, like it is customary on Unix/Linux systems, git just hands over the mail to your local sysem-wide MTA installation. (sendmail, exim, qmail, ...). So that MTA might have accepted the mail, but might itself not be configured to forward the mail.
However, git offers plenty of other config options, such as directly speaking SMTP to an remote e-mail server, etc.
You can test this setup by setting the '--to' address to your own e-mail addreess. If the mails arrive there, chances are high they would also make it to the list.
Regards, Harald
Hello All, Will it be fine for you to review the patch (csn1.cpp 1code line modified with 5 comment lines) sent as part of the earlier mail. We are working on identifying for any issues in send mail. According to your suggestion I tried git send-email to my personal email which is working fine.
Regards Saurabh
-----Original Message----- From: Harald Welte [mailto:laforge@gnumonks.org] Sent: Wednesday, February 24, 2016 2:15 PM To: Saurabh Sharan Saurabh.Sharan@radisys.com Cc: osmocom-net-gprs@lists.osmocom.org Subject: Re: [PATCH] fix for encoding of padding bits
Hi Saurabh,
On Wed, Feb 24, 2016 at 06:52:58AM +0000, Saurabh Sharan wrote:
We had tried the option of sending one patch through git send-email. This was our first attempt so wanted to confirm whether we followed the process correctly. Please note that I am unable to see this patch(mail below) in Archive maintained at http://lists.osmocom.org/pipermail/osmocom-net-gprs/
Indeed, the mail did not arrive here. Please check your git send-email configuration. By default, like it is customary on Unix/Linux systems, git just hands over the mail to your local sysem-wide MTA installation. (sendmail, exim, qmail, ...). So that MTA might have accepted the mail, but might itself not be configured to forward the mail.
However, git offers plenty of other config options, such as directly speaking SMTP to an remote e-mail server, etc.
You can test this setup by setting the '--to' address to your own e-mail addreess. If the mails arrive there, chances are high they would also make it to the list.
Regards, Harald
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
osmocom-net-gprs@lists.osmocom.org
-
Harald Welte -
Saurabh Sharan