Update patch set 3

Patch Set 3: (10 comments) Patch-set: 3 Attention: {"person_ident":"Gerrit User 1000010 \u003c1000010@035e6965-6537-41bd-912c-053f3cf69326\u003e","operation":"REMOVE","reason":"\u003cGERRIT_ACCOUNT_1000010\u003e replied on the change"} Attention: {"person_ident":"Gerrit User 1000074 \u003c1000074@035e6965-6537-41bd-912c-053f3cf69326\u003e","operation":"ADD","reason":"\u003cGERRIT_ACCOUNT_1000010\u003e replied on the change"}
2024-02-14 19:01:47 +00:00 · 2024-02-14 19:01:47 +00:00 · ee8b085a2e
parent 47d5ce218b
commit ee8b085a2e
1 changed files with 186 additions and 0 deletions
--- a/186
+++ b/186
@ -17,6 +17,24 @@
      "revId": "dacf17334861dd8d9531c4030742084b58d2aad4",
      "serverId": "035e6965-6537-41bd-912c-053f3cf69326"
    },
+    {
+      "unresolved": true,
+      "key": {
+        "uuid": "79fc8376_065406dc",
+        "filename": "include/osmocom/sigtran/osmo_ss7.h",
+        "patchSetId": 3
+      },
+      "lineNbr": 293,
+      "author": {
+        "id": 1000010
+      },
+      "writtenOn": "2024-02-14T19:01:47Z",
+      "side": 1,
+      "message": "Ack, I will drop the `osmo_` prefix and move to `ss7_internal.h`.\nWe can always make it public, if needed.",
+      "parentUuid": "1dea7cd6_cc14eb33",
+      "revId": "dacf17334861dd8d9531c4030742084b58d2aad4",
+      "serverId": "035e6965-6537-41bd-912c-053f3cf69326"
+    },
    {
      "unresolved": true,
      "key": {
@ -52,6 +70,24 @@
      "revId": "dacf17334861dd8d9531c4030742084b58d2aad4",
      "serverId": "035e6965-6537-41bd-912c-053f3cf69326"
    },
+    {
+      "unresolved": true,
+      "key": {
+        "uuid": "b4eb789f_92e333a2",
+        "filename": "include/osmocom/sigtran/osmo_ss7.h",
+        "patchSetId": 3
+      },
+      "lineNbr": 349,
+      "author": {
+        "id": 1000010
+      },
+      "writtenOn": "2024-02-14T19:01:47Z",
+      "side": 1,
+      "message": "I added this because internally this function looks for a matching ASP:\n\n```\n/* Check if the candicate we have here has any suitable\n * ASP */\nif (osmo_ss7_asp_find_by_proto2(as, trans_proto, proto)) \n        return as;\n```\n\nand because in theory there can be both M3UA/TCP and M3UA/SCTP ASPs in the list. Looks like I got this wrong and in this context *any* ASP is suitable for us, regardless of the transport protocol.\n\nI will revert this chunk and change the internal logic to look for an ASP with matching ASP protocol, but any transport protocol (be it TCP or SCTP).",
+      "parentUuid": "5ebb016a_373031ba",
+      "revId": "dacf17334861dd8d9531c4030742084b58d2aad4",
+      "serverId": "035e6965-6537-41bd-912c-053f3cf69326"
+    },
    {
      "unresolved": true,
      "key": {
@ -87,6 +123,24 @@
      "revId": "dacf17334861dd8d9531c4030742084b58d2aad4",
      "serverId": "035e6965-6537-41bd-912c-053f3cf69326"
    },
+    {
+      "unresolved": true,
+      "key": {
+        "uuid": "fb9fab4c_5a76f7fb",
+        "filename": "include/osmocom/sigtran/osmo_ss7.h",
+        "patchSetId": 3
+      },
+      "lineNbr": 616,
+      "author": {
+        "id": 1000010
+      },
+      "writtenOn": "2024-02-14T19:01:47Z",
+      "side": 1,
+      "message": "Ok, I will revert my changes to the `simple_{server,client}` API and let it use the default transport, determined by `ss7_default_trans_proto_for_asp_proto()`.\n\nThis also means reverting changes to `examples/sccp_demo_user.c`, because it\u0027s using this API.",
+      "parentUuid": "ec121186_f4970e6e",
+      "revId": "dacf17334861dd8d9531c4030742084b58d2aad4",
+      "serverId": "035e6965-6537-41bd-912c-053f3cf69326"
+    },
    {
      "unresolved": true,
      "key": {
@ -140,6 +194,24 @@
      "revId": "dacf17334861dd8d9531c4030742084b58d2aad4",
      "serverId": "035e6965-6537-41bd-912c-053f3cf69326"
    },
+    {
+      "unresolved": true,
+      "key": {
+        "uuid": "6c10896e_cbfb827a",
+        "filename": "src/osmo_ss7_asp.c",
+        "patchSetId": 3
+      },
+      "lineNbr": 137,
+      "author": {
+        "id": 1000010
+      },
+      "writtenOn": "2024-02-14T19:01:47Z",
+      "side": 1,
+      "message": "I can do `ipproto \u003c\u003c 16`, which would allow for values up to 65535.\n\n\u003e I don\u0027t really see why we should do this kind of heuristics just to save us 10 lines of code checks.\n\nI can also implement this without mux-ing, using several if-statements.\nBut personally I find the current approach more readable.",
+      "parentUuid": "6183e0c1_8b6d0fd3",
+      "revId": "dacf17334861dd8d9531c4030742084b58d2aad4",
+      "serverId": "035e6965-6537-41bd-912c-053f3cf69326"
+    },
    {
      "unresolved": true,
      "key": {
@ -157,6 +229,24 @@
      "revId": "dacf17334861dd8d9531c4030742084b58d2aad4",
      "serverId": "035e6965-6537-41bd-912c-053f3cf69326"
    },
+    {
+      "unresolved": true,
+      "key": {
+        "uuid": "f3d29fae_8ffc61d5",
+        "filename": "src/osmo_ss7_asp.c",
+        "patchSetId": 3
+      },
+      "lineNbr": 532,
+      "author": {
+        "id": 1000010
+      },
+      "writtenOn": "2024-02-14T19:01:47Z",
+      "side": 1,
+      "message": "Ack, I\u0027ll then reword as follows:\n\n```\nASP protocol \u0027%s\u0027 with transport protocol %d is not supported\n```",
+      "parentUuid": "98844271_a11de137",
+      "revId": "dacf17334861dd8d9531c4030742084b58d2aad4",
+      "serverId": "035e6965-6537-41bd-912c-053f3cf69326"
+    },
    {
      "unresolved": true,
      "key": {
@ -174,6 +264,24 @@
      "revId": "dacf17334861dd8d9531c4030742084b58d2aad4",
      "serverId": "035e6965-6537-41bd-912c-053f3cf69326"
    },
+    {
+      "unresolved": false,
+      "key": {
+        "uuid": "c6350764_0fe9d1a3",
+        "filename": "src/osmo_ss7_asp.c",
+        "patchSetId": 3
+      },
+      "lineNbr": 887,
+      "author": {
+        "id": 1000010
+      },
+      "writtenOn": "2024-02-14T19:01:47Z",
+      "side": 1,
+      "message": "I haven\u0027t marked the related thread as resolved either, so let\u0027s not create even more.",
+      "parentUuid": "76daaf4e_92ee772e",
+      "revId": "dacf17334861dd8d9531c4030742084b58d2aad4",
+      "serverId": "035e6965-6537-41bd-912c-053f3cf69326"
+    },
    {
      "unresolved": true,
      "key": {
@ -245,6 +353,24 @@
      "revId": "dacf17334861dd8d9531c4030742084b58d2aad4",
      "serverId": "035e6965-6537-41bd-912c-053f3cf69326"
    },
+    {
+      "unresolved": true,
+      "key": {
+        "uuid": "fccd3b75_26c84b50",
+        "filename": "src/osmo_ss7_vty.c",
+        "patchSetId": 3
+      },
+      "lineNbr": 65,
+      "author": {
+        "id": 1000010
+      },
+      "writtenOn": "2024-02-14T19:01:47Z",
+      "side": 1,
+      "message": "I will see what I can do here, but I am really not getting what\u0027s so special about using this syntax over [currently broken] `[(sctp|tcp)]` syntax as long as it works and looks the same in the interactive VTY.",
+      "parentUuid": "549309ec_ee84beca",
+      "revId": "dacf17334861dd8d9531c4030742084b58d2aad4",
+      "serverId": "035e6965-6537-41bd-912c-053f3cf69326"
+    },
    {
      "unresolved": true,
      "key": {
@ -298,6 +424,24 @@
      "revId": "dacf17334861dd8d9531c4030742084b58d2aad4",
      "serverId": "035e6965-6537-41bd-912c-053f3cf69326"
    },
+    {
+      "unresolved": true,
+      "key": {
+        "uuid": "a16f1926_267bc604",
+        "filename": "src/osmo_ss7_vty.c",
+        "patchSetId": 3
+      },
+      "lineNbr": 995,
+      "author": {
+        "id": 1000010
+      },
+      "writtenOn": "2024-02-14T19:01:47Z",
+      "side": 1,
+      "message": "\u003e Why don\u0027t have \"transport-protocol (sctp|tcp)\" command around here?\n\u003e Is it really needed to set the transport type already in the \"asp NAME ...\" command?\n\n@laforge@osmocom.org already answered above, acknowledging.\n\n\u003e You are missing a tcp-role (client|server) command btw.\n\nDo we really want separate `proto-role` param for each transport protocol? For now it\u0027s going to be only two, but who knows about the future. I would go for a common parameter because the role is not specific to the transport protocol. And I don\u0027t see why would anyone want to have SCTP specific parameters in an ASP using TCP as the transport, for instance.\n\n\u003e Regarding tcp-role (client|server): I think the sctp-role (client|server) also works with the TCP transport.\n\nAck, it works for both transports.\n\n\u003e So we could just make the tcp-role an alias. Or introduce a new transport-role with a compatibility alias to sctp-role?\n\nAs I wrote above, I would prefer a generic role selection command. So this means adding a new `transport-role` command and turning `sctp-role` into alias. I will proceed with that if you both agree on that.",
+      "parentUuid": "43cf723e_891d8fca",
+      "revId": "dacf17334861dd8d9531c4030742084b58d2aad4",
+      "serverId": "035e6965-6537-41bd-912c-053f3cf69326"
+    },
    {
      "unresolved": true,
      "key": {
@ -368,6 +512,24 @@
      "revId": "dacf17334861dd8d9531c4030742084b58d2aad4",
      "serverId": "035e6965-6537-41bd-912c-053f3cf69326"
    },
+    {
+      "unresolved": false,
+      "key": {
+        "uuid": "7fcc3745_793c6ada",
+        "filename": "tests/vty/ss7_asp_test.vty",
+        "patchSetId": 3
+      },
+      "lineNbr": 58,
+      "author": {
+        "id": 1000010
+      },
+      "writtenOn": "2024-02-14T19:01:47Z",
+      "side": 1,
+      "message": "Please, let\u0027s take a look at the VTY transcript tests:\n\nhttps://cgit.osmocom.org/libosmocore/tree/tests/vty/vty_transcript_test.vty\n\nAs can be seen (compare `multi1` and `multi2`), in the interactive VTY the help strings look **identical** for both variants.\n\nLet\u0027s please stop discussing this. As I said in other comment, using `([sctp]|[tcp])` is workaround to avoid hitting an assert in libosmovty. It does look weird in the code, but works and looks identical in the VTY.",
+      "parentUuid": "32c77c78_579936fe",
+      "revId": "dacf17334861dd8d9531c4030742084b58d2aad4",
+      "serverId": "035e6965-6537-41bd-912c-053f3cf69326"
+    },
    {
      "unresolved": true,
      "key": {
@ -391,6 +553,30 @@
      "revId": "dacf17334861dd8d9531c4030742084b58d2aad4",
      "serverId": "035e6965-6537-41bd-912c-053f3cf69326"
    },
+    {
+      "unresolved": true,
+      "key": {
+        "uuid": "ad52c524_082893f1",
+        "filename": "tests/vty/ss7_asp_test.vty",
+        "patchSetId": 3
+      },
+      "lineNbr": 408,
+      "author": {
+        "id": 1000010
+      },
+      "writtenOn": "2024-02-14T19:01:47Z",
+      "side": 1,
+      "message": "It\u0027s not really changing the existing config files. This is a VTY transcript test, which demonstrates that `show running-config` is explicitly printing the transport protocol regardless of the default value. I will implement the logic to not print the transport protocol if it matches the default (as you and Pau requested in other comment https://gerrit.osmocom.org/c/libosmo-sccp/+/35796/comment/e558b88d_31c7d1be/).",
+      "parentUuid": "d3211568_5d7c7292",
+      "range": {
+        "startLine": 408,
+        "startChar": 29,
+        "endLine": 408,
+        "endChar": 33
+      },
+      "revId": "dacf17334861dd8d9531c4030742084b58d2aad4",
+      "serverId": "035e6965-6537-41bd-912c-053f3cf69326"
+    },
    {
      "unresolved": true,
      "key": {