Hello.
I've got rid of unnecessary cycle and to made difference between v2 and v3 more visible: v2 is basically just v3 with last bits of Kc zeroed. Also - small readability improvements.
Also I've added test suite with test vectors from original python implementation.
Hi Max,
There are definitely things weird in that patch :
- You double declare the same function - Why replace #defined constant by hardcoded numbers ? - Speaking about readability :
kc[6] = 4 * (kc[6] >> 2);
isn't the most obvious way to do kc[6] &= 0xfc;
Cheers,
Sylvain
On Mon, Nov 18, 2013 at 6:47 PM, ☎ Max.Suraev@fairwaves.ru wrote:
Hello.
I've got rid of unnecessary cycle and to made difference between v2 and v3 more visible: v2 is basically just v3 with last bits of Kc zeroed. Also - small readability improvements.
Also I've added test suite with test vectors from original python implementation.
-- best regards, Max, http://fairwaves.ru
19.11.2013 09:17, Sylvain Munaut пишет:
Hi Max,
There are definitely things weird in that patch :
- You double declare the same function
Which one?
- Why replace #defined constant by hardcoded numbers ?
To make code look more similar to comp128v1 and other code which uses Kc and rand: we use hardcoded values everywhere.
- Speaking about readability :
kc[6] = 4 * (kc[6] >> 2);
isn't the most obvious way to do kc[6] &= 0xfc;
Added, thank you.
Hi,
☎ wrote:
I've got rid of unnecessary cycle and to made difference between v2 and v3 more visible: v2 is basically just v3 with last bits of Kc zeroed. Also - small readability improvements.
Also I've added test suite with test vectors from original python implementation.
Cool!
+++ b/include/osmocom/gsm/comp128v23.h @@ -13,11 +13,11 @@
- Performs the COMP128 version 2 and 3 algorithm (used as A3/A8)
- ki : uint8_t [16]
- srand : uint8_t [16]
*/
- version : uint8_t (2 or 3)
- sres : uint8_t [4]
- kc : uint8_t [8]
- returns 1 if not version 2 or 3 specified
-int comp128v23(const uint8_t *ki, const uint8_t *rand, uint8_t version, uint8_t *sres, uint8_t *kc);
Since this is a public API I don't know if the old function can be removed.
+int comp128v3(const uint8_t *ki, const uint8_t *rand, uint8_t *sres, uint8_t *kc); +int comp128v3(const uint8_t *ki, const uint8_t *rand, uint8_t *sres, uint8_t *kc);
Copypaste error here. I think it's nice to add two new functions, but probably the old one needs to stay there.
+++ b/src/gsm/auth_comp128v23.c @@ -31,7 +31,7 @@ static int c128v2_gen_vec(struct osmo_auth_vector *vec, struct osmo_sub_auth_data *aud, const uint8_t *_rand) {
- comp128v23(aud->u.gsm.ki, _rand, 2, vec->sres, vec->kc);
comp128v2(aud->u.gsm.ki, _rand, vec->sres, vec->kc); vec->auth_types = OSMO_AUTH_TYPE_GSM;
return 0;
I like this change, thanks to the two new functions.
//Peter
19.11.2013 12:16, Peter Stuge пишет:
Since this is a public API I don't know if the old function can be removed.
I thought public api is in auth_comp128* e. g. osmo_auth_gen_vec() - see tests/comp128_test.c for usage examples. And functions like comp128v*() are internal implementation details.
+int comp128v3(const uint8_t *ki, const uint8_t *rand, uint8_t *sres, uint8_t *kc); +int comp128v3(const uint8_t *ki, const uint8_t *rand, uint8_t *sres, uint8_t *kc);
Copypaste error here. I think it's nice to add two new functions, but probably the old one needs to stay there.
Fixed, thank you.
☎ wrote:
Since this is a public API I don't know if the old function can be removed.
I thought public api is in auth_comp128* e. g. osmo_auth_gen_vec() - see tests/comp128_test.c for usage examples. And functions like comp128v*() are internal implementation details.
Using osmo_auth_*() is the recommended API but comp128v*() are also in the installed header files and thus also part of the public API.
//Peter
19.11.2013 12:42, Peter Stuge пишет:
☎ wrote:
Since this is a public API I don't know if the old function can be removed.
I thought public api is in auth_comp128* e. g. osmo_auth_gen_vec() - see tests/comp128_test.c for usage examples. And functions like comp128v*() are internal implementation details.
Using osmo_auth_*() is the recommended API but comp128v*() are also in the installed header files and thus also part of the public API.
Is there any reason not to deprecate it?
☎ wrote:
Using osmo_auth_*() is the recommended API but comp128v*() are also in the installed header files and thus also part of the public API.
Is there any reason not to deprecate it?
Even if it is deprecated it would have to stay, otherwise the libosmocore ABI breaks without a very good reason.
//Peter
That function hasn't been there long enough for anyone to use it.
There also wasn't any tagged version so far with it, so no ABI guarantee.
On Tue, Nov 19, 2013 at 1:29 PM, Peter Stuge peter@stuge.se wrote:
☎ wrote:
Using osmo_auth_*() is the recommended API but comp128v*() are also in the installed header files and thus also part of the public API.
Is there any reason not to deprecate it?
Even if it is deprecated it would have to stay, otherwise the libosmocore ABI breaks without a very good reason.
//Peter
It might make sense to add __attribute__ ((deprecated)) to comp128 function to discourage it's use.
19.11.2013 14:10, Sylvain Munaut пишет:
That function hasn't been there long enough for anyone to use it.
There also wasn't any tagged version so far with it, so no ABI guarantee.
On Tue, Nov 19, 2013 at 1:29 PM, Peter Stuge peter@stuge.se wrote:
☎ wrote:
Using osmo_auth_*() is the recommended API but comp128v*() are also in the installed header files and thus also part of the public API.
Is there any reason not to deprecate it?
Even if it is deprecated it would have to stay, otherwise the libosmocore ABI breaks without a very good reason.
//Peter
Hi Max,
I would like to merge your patch, but:
On Tue, Nov 19, 2013 at 12:25:52PM +0100, ☎ wrote:
+int +comp128v2(const uint8_t *ki, const uint8_t *rand, uint8_t *sres, uint8_t *kc) +{
- int r = comp128v3(ki, rand, sres, kc);
- kc[7] = 0; /* 10 last bits of Kc forced to 0 */
- kc[6] &= 0xfc;
- return r;
+}
this is space-indented, not tab-indented.
+static struct osmo_sub_auth_data test_aux2 = {
- .type = OSMO_AUTH_TYPE_GSM,
- .algo = OSMO_AUTH_ALG_COMP128v2,
- .u.gsm = {
- .ki = { 0xAA, 0xAA, 0xAA, 0xAA, 0xAA, 0xAA, 0xAA, 0xAA, 0xAA, 0xAA, 0xAA, 0xAA, 0xAA, 0xAA, 0xAA, 0xAA },
those lienes are too long for 80-character wide terminals
- uint8_t buf[12];
- osmo_hexparse(res, buf, 12);
- if (0 != memcmp(buf, vec->sres, 4)) {
- printf("%d FAIL SRES:\n", rc);
there's again mixed space and tab indentation.
+void test_comp128v3(char * rand, char * res) {
we put the curly braces at the beginning of the line, not at the end of the line. And again the functions are space indented.
Furthermore, your patch does not apply on top of master.
It's sad to see that valuable contributions are lost due to basic coding style issues not being observed. We had this back in April with your KASUMI related patches, and it was never fixed. Please take the time to fix those issues, thanks.
Regards, Harald
On Fri, Nov 22, 2013 at 10:44 AM, Harald Welte laforge@gnumonks.org wrote:
Hi Max,
I would like to merge your patch, but:
On Tue, Nov 19, 2013 at 12:25:52PM +0100, ☎ wrote:
+int +comp128v2(const uint8_t *ki, const uint8_t *rand, uint8_t *sres, uint8_t *kc) +{
- int r = comp128v3(ki, rand, sres, kc);
- kc[7] = 0; /* 10 last bits of Kc forced to 0 */
- kc[6] &= 0xfc;
- return r;
+}
this is space-indented, not tab-indented.
+static struct osmo_sub_auth_data test_aux2 = {
.type = OSMO_AUTH_TYPE_GSM,.algo = OSMO_AUTH_ALG_COMP128v2,.u.gsm = {.ki = { 0xAA, 0xAA, 0xAA, 0xAA, 0xAA, 0xAA, 0xAA, 0xAA, 0xAA, 0xAA, 0xAA, 0xAA, 0xAA, 0xAA, 0xAA, 0xAA },those lienes are too long for 80-character wide terminals
- uint8_t buf[12];
- osmo_hexparse(res, buf, 12);
- if (0 != memcmp(buf, vec->sres, 4)) {
printf("%d FAIL SRES:\n", rc);there's again mixed space and tab indentation.
+void test_comp128v3(char * rand, char * res) {
we put the curly braces at the beginning of the line, not at the end of the line. And again the functions are space indented.
Furthermore, your patch does not apply on top of master.
It's sad to see that valuable contributions are lost due to basic coding style issues not being observed. We had this back in April with your KASUMI related patches, and it was never fixed. Please take the time to fix those issues, thanks.
I should probably keep my mouth shut here, but I've always wondered why....
Why not just fix it since this is a community effort? It takes longer to complain about it and reject it than it does to perform the actual work. (Sans the merge problem on Master).
Anyway, please don't take it as a personal attack because its not. Lots of folks do it and I'm interested in understanding why they do it.
Jeff
22.11.2013 17:59, Jeffrey Walton пишет:
Why not just fix it since this is a community effort? It takes longer to complain about it and reject it than it does to perform the actual work. (Sans the merge problem on Master).
I second that. I mean I do understand that nobody wants to do the boring work which could be offloaded to others, and rightfully so. But that's also discouraging - and that's why those patches are stalled since spring: I use patched version locally and keep working on my research. Because it's research project, not some product there's no maintenance burden for me in this setup, while with pushing stuff upstream - there is. Hence the efforts to mainline things are not as ideal as they could be.
Of course I would try to fix the patches anyway, so that they will please upstream, but not today - Friday evening, and so on :)
I hope I do not sound complaining - just would like to clarify the difference.
☎ wrote:
Why not just fix it since this is a community effort? It takes longer to complain about it and reject it than it does to perform the actual work. (Sans the merge problem on Master).
I second that.
Complaining and rejecting might take longer but also sends a clear message that actually the development in this project has to follow style.
Trust me, wiping the floor after other people, as Harald expressed it, is not sustainable and becomes much more frustrating for everyone.
I mean I do understand that nobody wants to do the boring work
Right, so it seems reasonable that style is the responsibility of the original patch author, don't you think?
Pro tip: Use the correct style already when creating the patch, that way there is no extra step of boring reformatting/cleanup work.
But that's also discouraging - and that's why those patches are stalled since spring: I use patched version locally
To me it says that you were too unattentive or too lazy to follow the coding style when originally creating those changes, and that now you don't care to fix the changes so that others can benefit from them.
There are lots of valid reasons for not immediately using the correct style, but never fixing old commits makes me go: Sad face. :\
Because it's research project .. no maintenance burden for me in this setup, while with pushing stuff upstream - there is.
Probably the upstream code has helped you save some time in the research project, so I think it is a fair tradeoff for you to spend time also giving back to the codebase - and you do!
I think you're doing a great job, you're certainly contributing to the project, it's just unfortunate for everybody that you haven't tidied those old patches. If the project was developing at a higher pace they could have bitrotted already. :\
Hence the efforts to mainline things are not as ideal as they could be.
In the end that's always up to each individual contributor.
Of course I would try to fix the patches anyway, so that they will please upstream, but not today - Friday evening, and so on :)
For me it's not about pleasing upstream, but about recognizing the importance of following style in order to increase maintainability of the source code as a whole, and to decrease load on maintainers, so that development progress is as smooth as possible.
I hope I do not sound complaining
I don't think you sound like you are complaining but I do think that the responsibility to format contributions according to project style lies firmly with each individual contributor and never anywhere else.
Kind regards
//Peter
Hi Jeffery,
On Fri, Nov 22, 2013 at 11:59:28AM -0500, Jeffrey Walton wrote:
Why not just fix it since this is a community effort? It takes longer to complain about it and reject it than it does to perform the actual work. (Sans the merge problem on Master).
well, number one reason is 'because this is the culture'. All (at least most) of the projects I have been involved in have a coding style. It is the duty of every developer to follow that. If I want to get something merged to wireshark, I have to use their coding style, not my personal one, no matter if I like it or not.
If an occasional mistake happens, then that is excusable. But if I always post my patches without caring about the rules (including coding style), then it measn that I am perceived as being ignorant towards the project.
Having a unified coding style is important for many software developers.
Why am I not fixing it up? Because * I might break the code doing so (and I might not have the same test setup and familiarity with that paticular code / functionalty as the original subitter) * maintainers are typically overloaded in all projects. For efficient collaborative development it is important that the work for the maintainers is reduced, as they are single-point-of-failures. * maintainers are developers. They primarily like to write code, and like to be efficient/productive. 'wiping the floor' after other developers/contributors who have been too lazy to care about the rules of the project is tiresome, annoying, and generally not very productive use of their time
It is not only the content of your work that matters, but also the style. Many people also consider it as disrespectful, if contributors are not willing to align with a projects style / rules / architecture.
As a maintainer [and I'm not saying that I have been maintaining openbsc or related projects during the last year, I'm just generally speaking], I want contributors to learn how to write patches that I can simply apply without having to spend time on it except reading through the code. If I always fix up everyones patches, I will do that for the years to come and will not do anything else (more productive).
Regards, Harald
On Fri, Nov 22, 2013 at 12:35 PM, Harald Welte laforge@gnumonks.org wrote:
Hi Jeffery,
On Fri, Nov 22, 2013 at 11:59:28AM -0500, Jeffrey Walton wrote:
Why not just fix it since this is a community effort? It takes longer to complain about it and reject it than it does to perform the actual work. (Sans the merge problem on Master).
well, number one reason is 'because this is the culture'. All (at least most) of the projects I have been involved in have a coding style. It is the duty of every developer to follow that. If I want to get something merged to wireshark, I have to use their coding style, not my personal one, no matter if I like it or not.
If an occasional mistake happens, then that is excusable. But if I always post my patches without caring about the rules (including coding style), then it measn that I am perceived as being ignorant towards the project.
Having a unified coding style is important for many software developers.
Why am I not fixing it up? Because
- I might break the code doing so (and I might not have the same test setup and familiarity with that paticular code / functionalty as the original subitter)
- maintainers are typically overloaded in all projects. For efficient collaborative development it is important that the work for the maintainers is reduced, as they are single-point-of-failures.
- maintainers are developers. They primarily like to write code, and like to be efficient/productive. 'wiping the floor' after other developers/contributors who have been too lazy to care about the rules of the project is tiresome, annoying, and generally not very productive use of their time
It is not only the content of your work that matters, but also the style. Many people also consider it as disrespectful, if contributors are not willing to align with a projects style / rules / architecture.
As a maintainer [and I'm not saying that I have been maintaining openbsc or related projects during the last year, I'm just generally speaking], I want contributors to learn how to write patches that I can simply apply without having to spend time on it except reading through the code. If I always fix up everyones patches, I will do that for the years to come and will not do anything else (more productive).
Thanks Harald. I think there are a lot of good points here. If these things matter that much, why not use a commit hook that enforces policy by applying formatting without prejudice?
With a commit hook, you don't waste your time or get frustrated with committers, and developers don't waste their time learning yet another set of standards and idiosyncrasies. The time could then be better spent on things like writing code or reviewing proposed functionality.
Jeff
Jeffrey Walton wrote:
Thanks Harald. I think there are a lot of good points here. If these things matter that much, why not use a commit hook that enforces policy by applying formatting without prejudice?
Because we'd like to have developers share a cohesive culture around the code and code style rather than have developers that are unable to do better than agree to disagree about something so simple as style, and resign to have a computer fix up the differences.
If not even agreement about style can be accomplished then it's not very likely that much other agreement can be had either.
(Unfortunately I know that all too well from experience.)
developers don't waste their time learning yet another set of standards and idiosyncrasies.
I think that statement is quite intolerant of you. Learning a culture by slowly immersing oneself in it is never a waste of time, I think it enriches life in more ways than we can know.
If developers do not want to work together then they shouldn't pretend; it would be much better to go separate ways.
//Peter
All fixed. Let me know if I've overlooked something.
22.11.2013 16:44, Harald Welte пишет:
Hi Max,
I would like to merge your patch, but:
On Tue, Nov 19, 2013 at 12:25:52PM +0100, ☎ wrote:
+int +comp128v2(const uint8_t *ki, const uint8_t *rand, uint8_t *sres, uint8_t *kc) +{
- int r = comp128v3(ki, rand, sres, kc);
- kc[7] = 0; /* 10 last bits of Kc forced to 0 */
- kc[6] &= 0xfc;
- return r;
+}
this is space-indented, not tab-indented.
+static struct osmo_sub_auth_data test_aux2 = {
- .type = OSMO_AUTH_TYPE_GSM,
- .algo = OSMO_AUTH_ALG_COMP128v2,
- .u.gsm = {
- .ki = { 0xAA, 0xAA, 0xAA, 0xAA, 0xAA, 0xAA, 0xAA, 0xAA, 0xAA, 0xAA, 0xAA, 0xAA, 0xAA, 0xAA, 0xAA, 0xAA },
those lienes are too long for 80-character wide terminals
- uint8_t buf[12];
- osmo_hexparse(res, buf, 12);
- if (0 != memcmp(buf, vec->sres, 4)) {
- printf("%d FAIL SRES:\n", rc);
there's again mixed space and tab indentation.
+void test_comp128v3(char * rand, char * res) {
we put the curly braces at the beginning of the line, not at the end of the line. And again the functions are space indented.
Furthermore, your patch does not apply on top of master.
It's sad to see that valuable contributions are lost due to basic coding style issues not being observed. We had this back in April with your KASUMI related patches, and it was never fixed. Please take the time to fix those issues, thanks.
Regards, Harald
Hi Max,
On Mon, Nov 25, 2013 at 12:20:33PM +0100, ☎ wrote:
All fixed. Let me know if I've overlooked something.
thanks. it looks fine to me, and if it was applying to libosmocore/master, I would have merged it today :)
Just for sake of completeness, the linux kernel checkpatch.pl script finds the following issues (which I would ignore, but just as a general rule, it is a good way to catch coding style issues):
------------ WARNING: line over 80 characters #45: FILE: include/osmocom/gsm/comp128v23.h:20: +int comp128v2(const uint8_t *ki, const uint8_t *rand, uint8_t *sres, uint8_t *kc);
WARNING: line over 80 characters #46: FILE: include/osmocom/gsm/comp128v23.h:21: +int comp128v3(const uint8_t *ki, const uint8_t *rand, uint8_t *sres, uint8_t *kc);
WARNING: please, no spaces at the start of a line #225: FILE: tests/comp128/comp128_test.c:18: + }$
WARNING: please, no spaces at the start of a line #234: FILE: tests/comp128/comp128_test.c:27: + }$
ERROR: "foo * bar" should be "foo *bar" #237: FILE: tests/comp128/comp128_test.c:30: +void print_check(int rc, char * res, struct osmo_auth_vector *vec)
ERROR: trailing statements should be on next line #251: FILE: tests/comp128/comp128_test.c:44: + else printf("%d OK\n", rc);
ERROR: else should follow close brace '}' #251: FILE: tests/comp128/comp128_test.c:44: + } + else printf("%d OK\n", rc);
ERROR: "foo * bar" should be "foo *bar" #254: FILE: tests/comp128/comp128_test.c:47: +void test_comp128v3(char * rand, char * res)
ERROR: "foo * bar" should be "foo *bar" #254: FILE: tests/comp128/comp128_test.c:47: +void test_comp128v3(char * rand, char * res)
ERROR: "foo * bar" should be "foo *bar" #264: FILE: tests/comp128/comp128_test.c:57: +void test_comp128v2(char * rand, char * res)
ERROR: "foo * bar" should be "foo *bar" #264: FILE: tests/comp128/comp128_test.c:57: +void test_comp128v2(char * rand, char * res) ------------
Regards,
01.12.2013 14:39, Harald Welte пишет:
Hi Max,
On Mon, Nov 25, 2013 at 12:20:33PM +0100, ☎ wrote:
All fixed. Let me know if I've overlooked something.
thanks. it looks fine to me, and if it was applying to libosmocore/master, I would have merged it today :)
Hmm... it applies with patch --dry-run -p1 < 0001-Refactor-COMP128v23-implementation-and-add-test-suit-v5.patch just fine. What exactly is wrong and how do I fix it? I've made a patch using "git format-patch" - is there some better way?
Just for sake of completeness, the linux kernel checkpatch.pl script finds the following issues (which I would ignore, but just as a general rule, it is a good way to catch coding style issues):
WARNING: line over 80 characters #45: FILE: include/osmocom/gsm/comp128v23.h:20: +int comp128v2(const uint8_t *ki, const uint8_t *rand, uint8_t *sres, uint8_t *kc);
WARNING: line over 80 characters #46: FILE: include/osmocom/gsm/comp128v23.h:21: +int comp128v3(const uint8_t *ki, const uint8_t *rand, uint8_t *sres, uint8_t *kc);
With all due respect - those are shorter than the line they are replacing which is already committed to the git.
The rest I have corrected.
Hi Max,
On Mon, Dec 02, 2013 at 11:36:08AM +0100, ☎ wrote:
Hmm... it applies with patch --dry-run -p1 < 0001-Refactor-COMP128v23-implementation-and-add-test-suit-v5.patch just fine. What exactly is wrong and how do I fix it? I've made a patch using "git format-patch" - is there some better way?
my apologies. It was a broken setup on my side. 'git format-patch' (on top of master branch) is the best way, of course.
With all due respect - those are shorter than the line they are replacing which is already committed to the git.
Yes. The original code is not perfect, and sometimes we bend the rules accidentially or intentionally. Also, the code normally doesn't go through the kernels checkpatch.pl - we simply write it with the coding style in mind. Also, as holger has indicated, those are warnings and not errors.
Thanks for taking the extra time to clean it up, even if you are personally not convinced it would be neccessary. Please continue to do so with any other patches that you may have pending. We do want to get things merged and avoid fragmentation.
Regards, Harald
baseband-devel@lists.osmocom.org
-
Harald Welte -
Holger Hans Peter Freyther -
Jeffrey Walton -
Peter Stuge -
Sylvain Munaut -
☎