Change in osmo-sgsn[master]: Store GSN address in libosmocore struct
Harald Welte has posted comments on this change. ( https://gerrit.osmocom.org/12352 ) Change subject: Store GSN address in libosmocore struct .. Patch Set 2: Code-Review-1 I'm also surprised that there is work on gtphub, as I don't recall any related issue being relevant at this point. Let's please abandon it. -- To view, visit https://gerrit.osmocom.org/12352 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-sgsn Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I6ed32a91483dc608c47df77869033a6e891e9e6a Gerrit-Change-Number: 12352 Gerrit-PatchSet: 2 Gerrit-Owner: Max Gerrit-Reviewer: Harald Welte Gerrit-Reviewer: Holger Freyther Gerrit-Reviewer: Jenkins Builder (102) Gerrit-Reviewer: Max Gerrit-Reviewer: Neels Hofmeyr Gerrit-Reviewer: Pau Espin Pedrol Gerrit-Reviewer: Stefan Sperling Gerrit-Comment-Date: Mon, 07 Jan 2019 15:47:35 + Gerrit-HasComments: No Gerrit-HasLabels: Yes
Change in osmo-sgsn[master]: Store GSN address in libosmocore struct
Neels Hofmeyr has posted comments on this change. ( https://gerrit.osmocom.org/12352 ) Change subject: Store GSN address in libosmocore struct .. Patch Set 2: (1 comment) https://gerrit.osmocom.org/#/c/12352/2/src/gprs/gtphub.c File src/gprs/gtphub.c: https://gerrit.osmocom.org/#/c/12352/2/src/gprs/gtphub.c@196 PS2, Line 196: gsna->length = 4; if using the new struct, should also populate the type member throughout the code. Alternatively spread API doc everywhere that the field is ignored, but that's ugly IMHO. That kind of makes it not worthwhile bothering, I guess. -- To view, visit https://gerrit.osmocom.org/12352 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-sgsn Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I6ed32a91483dc608c47df77869033a6e891e9e6a Gerrit-Change-Number: 12352 Gerrit-PatchSet: 2 Gerrit-Owner: Max Gerrit-Reviewer: Harald Welte Gerrit-Reviewer: Holger Freyther Gerrit-Reviewer: Jenkins Builder (102) Gerrit-Reviewer: Max Gerrit-Reviewer: Neels Hofmeyr Gerrit-Reviewer: Pau Espin Pedrol Gerrit-Reviewer: Stefan Sperling Gerrit-Comment-Date: Sat, 05 Jan 2019 03:59:12 + Gerrit-HasComments: Yes Gerrit-HasLabels: No
Change in osmo-sgsn[master]: Store GSN address in libosmocore struct
Neels Hofmeyr has posted comments on this change. ( https://gerrit.osmocom.org/12352 ) Change subject: Store GSN address in libosmocore struct .. Patch Set 2: I'd say, ok, fine, and since we're in the sgsn in a sub-area called gtphub, I don't even see how an ABI change is a problem. But I am puzzled why the gtphub code is coming up at this point in time. How are we currently interested in that, how did you even end up spotting the gsn_addr? -- To view, visit https://gerrit.osmocom.org/12352 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-sgsn Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I6ed32a91483dc608c47df77869033a6e891e9e6a Gerrit-Change-Number: 12352 Gerrit-PatchSet: 2 Gerrit-Owner: Max Gerrit-Reviewer: Harald Welte Gerrit-Reviewer: Holger Freyther Gerrit-Reviewer: Jenkins Builder (102) Gerrit-Reviewer: Max Gerrit-Reviewer: Neels Hofmeyr Gerrit-Reviewer: Pau Espin Pedrol Gerrit-Reviewer: Stefan Sperling Gerrit-Comment-Date: Sat, 05 Jan 2019 03:56:14 + Gerrit-HasComments: No Gerrit-HasLabels: No
Change in osmo-sgsn[master]: Store GSN address in libosmocore struct
Holger Freyther has posted comments on this change. ( https://gerrit.osmocom.org/12352 ) Change subject: Store GSN address in libosmocore struct .. Patch Set 2: (1 comment) https://gerrit.osmocom.org/#/c/12352/2/include/osmocom/sgsn/gtphub.h File include/osmocom/sgsn/gtphub.h: https://gerrit.osmocom.org/#/c/12352/2/include/osmocom/sgsn/gtphub.h@377 PS2, Line 377: struct osmo_gsn_address addr; > You are breaking ABI here, length of struct is different. […] But why would one want to have two representations of a GSN address? When to use which? If osmo_gsn_address is a superset why wouldn't we want to use it everywhere? -- To view, visit https://gerrit.osmocom.org/12352 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-sgsn Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I6ed32a91483dc608c47df77869033a6e891e9e6a Gerrit-Change-Number: 12352 Gerrit-PatchSet: 2 Gerrit-Owner: Max Gerrit-Reviewer: Harald Welte Gerrit-Reviewer: Holger Freyther Gerrit-Reviewer: Jenkins Builder (102) Gerrit-Reviewer: Max Gerrit-Reviewer: Neels Hofmeyr Gerrit-Reviewer: Pau Espin Pedrol Gerrit-Reviewer: Stefan Sperling Gerrit-Comment-Date: Fri, 28 Dec 2018 14:52:09 + Gerrit-HasComments: Yes Gerrit-HasLabels: No
Change in osmo-sgsn[master]: Store GSN address in libosmocore struct
Pau Espin Pedrol has posted comments on this change. ( https://gerrit.osmocom.org/12352 ) Change subject: Store GSN address in libosmocore struct .. Patch Set 2: > That's orthogonal to this patch I think: regardless of the use of > extra field in the struct there's no point in keeping effectively a > local duplicate of a struct available in a shared library we're > linked against anyway. It's not a local duplicate struct, it's a struct with an extra field not being used, so I so far no see a good reason to include this patch. Specially because I think that extra field is really not needed since the type can be inferred from length (like we do in most places in struct in46addr in osmo-ggsn and libgtp). Send forthcoming patches using that field and I'll re-check then. -- To view, visit https://gerrit.osmocom.org/12352 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-sgsn Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I6ed32a91483dc608c47df77869033a6e891e9e6a Gerrit-Change-Number: 12352 Gerrit-PatchSet: 2 Gerrit-Owner: Max Gerrit-Reviewer: Jenkins Builder (102) Gerrit-Reviewer: Max Gerrit-Reviewer: Pau Espin Pedrol Gerrit-Comment-Date: Tue, 18 Dec 2018 18:27:13 + Gerrit-HasComments: No Gerrit-HasLabels: No
Change in osmo-sgsn[master]: Store GSN address in libosmocore struct
Max has posted comments on this change. ( https://gerrit.osmocom.org/12352 ) Change subject: Store GSN address in libosmocore struct .. Patch Set 2: > Then please state so in the commit description. Until I see a user for the > "type" field in a forthcoming patch, I'll keep the -1. That's orthogonal to this patch I think: regardless of the use of extra field in the struct there's no point in keeping effectively a local duplicate of a struct available in a shared library we're linked against anyway. -- To view, visit https://gerrit.osmocom.org/12352 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-sgsn Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I6ed32a91483dc608c47df77869033a6e891e9e6a Gerrit-Change-Number: 12352 Gerrit-PatchSet: 2 Gerrit-Owner: Max Gerrit-Reviewer: Jenkins Builder (102) Gerrit-Reviewer: Max Gerrit-Reviewer: Pau Espin Pedrol Gerrit-Comment-Date: Tue, 18 Dec 2018 18:19:45 + Gerrit-HasComments: No Gerrit-HasLabels: No
Change in osmo-sgsn[master]: Store GSN address in libosmocore struct
Max has posted comments on this change. ( https://gerrit.osmocom.org/12352 ) Change subject: Store GSN address in libosmocore struct .. Patch Set 2: > You are breaking the ABI That's not a library - what kind of ABI breakage are you talking about? > basically not using the extra field "type" of the new struct. That's a subject for follow-up patches: I don't want to mix automated code changes with manual ones. -- To view, visit https://gerrit.osmocom.org/12352 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-sgsn Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I6ed32a91483dc608c47df77869033a6e891e9e6a Gerrit-Change-Number: 12352 Gerrit-PatchSet: 2 Gerrit-Owner: Max Gerrit-Reviewer: Jenkins Builder (102) Gerrit-Reviewer: Max Gerrit-Reviewer: Pau Espin Pedrol Gerrit-Comment-Date: Tue, 18 Dec 2018 18:11:11 + Gerrit-HasComments: No Gerrit-HasLabels: No
Change in osmo-sgsn[master]: Store GSN address in libosmocore struct
Pau Espin Pedrol has posted comments on this change. (
https://gerrit.osmocom.org/12352 )
Change subject: Store GSN address in libosmocore struct
..
Patch Set 2: Code-Review-1
(1 comment)
I see no good reason for this change. You are breaking the ABI and basically
not using the extra field "type" of the new struct.
https://gerrit.osmocom.org/#/c/12352/2/include/osmocom/sgsn/gtphub.h
File include/osmocom/sgsn/gtphub.h:
https://gerrit.osmocom.org/#/c/12352/2/include/osmocom/sgsn/gtphub.h@377
PS2, Line 377: struct osmo_gsn_address addr;
You are breaking ABI here, length of struct is different.
include/osmocom/gsm/gsm23003.h
59:struct osmo_gsn_address {
60- enum osmo_gsn_addr_type type;
61- uint8_t length;
62- uint8_t addr[16];
63-};
--
To view, visit https://gerrit.osmocom.org/12352
To unsubscribe, or for help writing mail filters, visit
https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-sgsn
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6ed32a91483dc608c47df77869033a6e891e9e6a
Gerrit-Change-Number: 12352
Gerrit-PatchSet: 2
Gerrit-Owner: Max
Gerrit-Reviewer: Jenkins Builder (102)
Gerrit-Reviewer: Pau Espin Pedrol
Gerrit-Comment-Date: Tue, 18 Dec 2018 17:58:06 +
Gerrit-HasComments: Yes
Gerrit-HasLabels: Yes
Change in osmo-sgsn[master]: Store GSN address in libosmocore struct
Hello Jenkins Builder,
I'd like you to reexamine a change. Please visit
https://gerrit.osmocom.org/12352
to look at the new patch set (#2).
Change subject: Store GSN address in libosmocore struct
..
Store GSN address in libosmocore struct
That's automated code change made using following program:
//spatch --in-place --sp-file dup.spatch -I include --dir ./ --all-includes
@@
@@
struct
- gsn_addr
+ osmo_gsn_address
@@
struct osmo_gsn_address a;
@@
(
a.
- len
+ length
|
a.
- buf
+ addr
)
@@
@@
- /* ... */
- struct gsn_addr {...};
Note that --all-includes is necessary because affected functions are
defined and implemented in files with different subdirectory names under
include and src correspondingly.
Change-Id: I6ed32a91483dc608c47df77869033a6e891e9e6a
---
M include/osmocom/sgsn/gtphub.h
M src/gprs/gtphub.c
M src/gprs/gtphub_ares.c
M tests/gtphub/gtphub_test.c
4 files changed, 61 insertions(+), 61 deletions(-)
git pull ssh://gerrit.osmocom.org:29418/osmo-sgsn refs/changes/52/12352/2
--
To view, visit https://gerrit.osmocom.org/12352
To unsubscribe, or for help writing mail filters, visit
https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-sgsn
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6ed32a91483dc608c47df77869033a6e891e9e6a
Gerrit-Change-Number: 12352
Gerrit-PatchSet: 2
Gerrit-Owner: Max
Gerrit-Reviewer: Jenkins Builder (102)
Change in osmo-sgsn[master]: Store GSN address in libosmocore struct
Max has uploaded this change for review. ( https://gerrit.osmocom.org/12352
Change subject: Store GSN address in libosmocore struct
..
Store GSN address in libosmocore struct
That's automated code change made using following program:
//spatch --in-place --sp-file dup.spatch -I include --dir ./ --all-includes
@@
@@
struct
- gsn_addr
+ osmo_gsn_address
@@
struct osmo_gsn_address a;
@@
(
a.
- len
+ length
|
a.
- buf
+ addr
)
Note that --all-includes is necessary because affected functions are
defined and implemented in files with different subdirectory names under
include and src correspondingly.
Change-Id: I6ed32a91483dc608c47df77869033a6e891e9e6a
---
M include/osmocom/sgsn/gtphub.h
M src/gprs/gtphub.c
M src/gprs/gtphub_ares.c
M tests/gtphub/gtphub_test.c
4 files changed, 61 insertions(+), 53 deletions(-)
git pull ssh://gerrit.osmocom.org:29418/osmo-sgsn refs/changes/52/12352/1
diff --git a/include/osmocom/sgsn/gtphub.h b/include/osmocom/sgsn/gtphub.h
index 8fd9f38..7afe83c 100644
--- a/include/osmocom/sgsn/gtphub.h
+++ b/include/osmocom/sgsn/gtphub.h
@@ -163,22 +163,25 @@
uint8_t buf[16];
};
-void gsn_addr_copy(struct gsn_addr *gsna, const struct gsn_addr *src);
-int gsn_addr_from_str(struct gsn_addr *gsna, const char *numeric_addr_str);
+void gsn_addr_copy(struct osmo_gsn_address *gsna,
+ const struct osmo_gsn_address *src);
+int gsn_addr_from_str(struct osmo_gsn_address *gsna,
+ const char *numeric_addr_str);
/* Return gsna in numeric string form, in a static buffer. */
-const char *gsn_addr_to_str(const struct gsn_addr *gsna);
+const char *gsn_addr_to_str(const struct osmo_gsn_address *gsna);
/* note: strbuf_len doesn't need to be larger than INET6_ADDRSTRLEN + 1. */
-const char *gsn_addr_to_strb(const struct gsn_addr *gsna,
+const char *gsn_addr_to_strb(const struct osmo_gsn_address *gsna,
char *strbuf, int strbuf_len);
/* Return 1 on match, zero otherwise. */
-int gsn_addr_same(const struct gsn_addr *a, const struct gsn_addr *b);
+int gsn_addr_same(const struct osmo_gsn_address *a,
+ const struct osmo_gsn_address *b);
/* Decode sa to gsna. Return 0 on success. If port is non-NULL, the port number
* from sa is also returned. */
-int gsn_addr_from_sockaddr(struct gsn_addr *gsna, uint16_t *port,
+int gsn_addr_from_sockaddr(struct osmo_gsn_address *gsna, uint16_t *port,
const struct osmo_sockaddr *sa);
/* expiry */
@@ -379,7 +382,7 @@
struct llist_head entry;
struct gtphub_peer *peer;
- struct gsn_addr addr;
+ struct osmo_gsn_address addr;
struct llist_head ports;
};
@@ -411,7 +414,7 @@
};
struct gtphub_bind {
- struct gsn_addr local_addr;
+ struct osmo_gsn_address local_addr;
uint16_t local_port;
struct osmo_fd ofd;
@@ -506,14 +509,14 @@
struct gtphub_peer_port *gtphub_port_have(struct gtphub *hub,
struct gtphub_bind *bind,
- const struct gsn_addr *addr,
+ const struct osmo_gsn_address *addr,
uint16_t port);
struct gtphub_peer_port *gtphub_port_find_sa(const struct gtphub_bind *bind,
const struct osmo_sockaddr *addr);
void gtphub_resolved_ggsn(struct gtphub *hub, const char *apn_oi_str,
- struct gsn_addr *resolved_addr,
+ struct osmo_gsn_address *resolved_addr,
time_t now);
const char *gtphub_port_str(struct gtphub_peer_port *port);
diff --git a/src/gprs/gtphub.c b/src/gprs/gtphub.c
index ca5857b..a17306e 100644
--- a/src/gprs/gtphub.c
+++ b/src/gprs/gtphub.c
@@ -161,12 +161,13 @@
}
}
-void gsn_addr_copy(struct gsn_addr *gsna, const struct gsn_addr *src)
+void gsn_addr_copy(struct osmo_gsn_address *gsna,
+ const struct osmo_gsn_address *src)
{
*gsna = *src;
}
-int gsn_addr_from_sockaddr(struct gsn_addr *gsna, uint16_t *port,
+int gsn_addr_from_sockaddr(struct osmo_gsn_address *gsna, uint16_t *port,
const struct osmo_sockaddr *sa)
{
char addr_str[256];
@@ -185,23 +186,24 @@
return gsn_addr_from_str(gsna, addr_str);
}
-int gsn_addr_from_str(struct gsn_addr *gsna, const char *numeric_addr_str)
+int gsn_addr_from_str(struct osmo_gsn_address *gsna,
+ const char *numeric_addr_str)
{
if ((!gsna) || (!numeric_addr_str))
return -1;
int af = AF_INET;
- gsna->len = 4;
+ gsna->length = 4;
const char *pos = numeric_addr_str;
for (; *pos; pos++) {
if (*pos == ':') {
af = AF_INET6;
- gsna->len = 16;
+
