Here is another set of cosmetic improvements for openggsn (branch: neels/refactor).
This prints the host address in human readable form and covers two more similar occurences.
A new error log fix has been added (see 4/4).
A nonstandard curly brace change from the previous patch has been dropped, and replaced by a blank line to increase readability.
~Neels
It would print the memory location of the address buffer. Instead, print the human readable host address and port.
The current code base supports only IPv4, and thread safety is apparently not required, hence just use inet_ntoa(). (The IPv6 and thread capable version is 4 times longer and harder to read.) --- gtp/gtp.c | 26 ++++++++++++++------------ 1 file changed, 14 insertions(+), 12 deletions(-)
diff --git a/gtp/gtp.c b/gtp/gtp.c index cfce244..08a9d61 100644 --- a/gtp/gtp.c +++ b/gtp/gtp.c @@ -71,6 +71,11 @@ len, osmo_hexdump((const uint8_t *) pack, len), \ ##args);
+#define LOGP_WITH_ADDR(ss, level, addr, fmt, args...) \ + LOGP(ss, level, "addr(%s:%d) " fmt, \ + inet_ntoa((addr).sin_addr), htons((addr).sin_port), \ + ##args); + /* API Functions */
const char *gtp_version() @@ -739,10 +744,9 @@ int gtp_new(struct gsn_t **gsn, char *statedir, struct in_addr *listen,
if (bind((*gsn)->fd0, (struct sockaddr *)&addr, sizeof(addr)) < 0) { (*gsn)->err_socket++; - LOGP(DLGTP, LOGL_ERROR, - "bind(fd0=%d, addr=%lx, len=%d) failed: Error = %s\n", - (*gsn)->fd0, (unsigned long)&addr, sizeof(addr), - strerror(errno)); + LOGP_WITH_ADDR(DLGTP, LOGL_ERROR, addr, + "bind(fd0=%d) failed: Error = %s\n", + (*gsn)->fd0, strerror(errno)); return -1; }
@@ -765,10 +769,9 @@ int gtp_new(struct gsn_t **gsn, char *statedir, struct in_addr *listen,
if (bind((*gsn)->fd1c, (struct sockaddr *)&addr, sizeof(addr)) < 0) { (*gsn)->err_socket++; - LOGP(DLGTP, LOGL_ERROR, - "bind(fd1c=%d, addr=%lx, len=%d) failed: Error = %s\n", - (*gsn)->fd1c, (unsigned long)&addr, sizeof(addr), - strerror(errno)); + LOGP_WITH_ADDR(DLGTP, LOGL_ERROR, addr, + "bind(fd1c=%d) failed: Error = %s\n", + (*gsn)->fd1c, strerror(errno)); return -1; }
@@ -791,10 +794,9 @@ int gtp_new(struct gsn_t **gsn, char *statedir, struct in_addr *listen,
if (bind((*gsn)->fd1u, (struct sockaddr *)&addr, sizeof(addr)) < 0) { (*gsn)->err_socket++; - LOGP(DLGTP, LOGL_ERROR, - "bind(fd1c=%d, addr=%lx, len=%d) failed: Error = %s\n", - (*gsn)->fd1c, (unsigned long)&addr, sizeof(addr), - strerror(errno)); + LOGP_WITH_ADDR(DLGTP, LOGL_ERROR, addr, + "bind(fd1c=%d) failed: Error = %s\n", + (*gsn)->fd1c, strerror(errno)); return -1; }
Fix spelling dublicate -> duplicate in comments and in (apparently only statically used) gtp_dublicate().
Add two TODO comments.
Fix other spelling/punctuation and one numbering in comments.
Remove an opening brace from a comment to not mix up cindent in vim.
Break a long line. --- gtp/gtp.c | 38 ++++++++++++++++++++------------------ gtp/gtp.h | 4 ++-- sgsnemu/sgsnemu.c | 20 +++++++++++++------- 3 files changed, 35 insertions(+), 27 deletions(-)
diff --git a/gtp/gtp.c b/gtp/gtp.c index 08a9d61..2a39934 100644 --- a/gtp/gtp.c +++ b/gtp/gtp.c @@ -305,19 +305,19 @@ static uint32_t get_tei(void *pack) * requires the receiving GSN to send a response, with the same information * as in the original response. For most messages this happens automatically: * - * Echo: Automatically dublicates the original response + * Echo: Automatically duplicates the original response * Create pdp context: The SGSN may send create context request even if - * a context allready exist (imsi+nsapi?). This means that the reply will - automatically dublicate the original response. It might however have + * a context already exist (imsi+nsapi?). This means that the reply will + * automatically duplicate the original response. It might however have * side effects in the application which is asked twice to validate * the login. - * Update pdp context: Automatically dublicates the original response??? + * Update pdp context: Automatically duplicates the original response??? * Delete pdp context. Automatically in gtp0, but in gtp1 will generate * a nonexist reply message. * * The correct solution will be to make a queue containing response messages. * This queue should be checked whenever a request is received. If the - * response is allready in the queue that response should be transmitted. + * response is already in the queue that response should be transmitted. * It should be possible to find messages in this queue on the basis of * the sequence number and peer GSN IP address (The sequense number is unique * within each path). This need to be implemented by a hash table. Furthermore @@ -614,7 +614,7 @@ int gtp_notification(struct gsn_t *gsn, int version, return 0; }
-int gtp_dublicate(struct gsn_t *gsn, int version, +int gtp_duplicate(struct gsn_t *gsn, int version, struct sockaddr_in *peer, uint16_t seq) { struct qmsg_t *qmsg; @@ -873,8 +873,8 @@ int gtp_echo_ind(struct gsn_t *gsn, int version, struct sockaddr_in *peer, int fd, void *pack, unsigned len) {
- /* Check if it was a dublicate request */ - if (!gtp_dublicate(gsn, 0, peer, get_seq(pack))) + /* Check if it was a duplicate request */ + if (!gtp_duplicate(gsn, 0, peer, get_seq(pack))) return 0;
/* Send off reply to request */ @@ -1258,13 +1258,15 @@ int gtp_create_pdp_ind(struct gsn_t *gsn, int version, uint8_t linked_nsapi = 0; struct pdp_t *linked_pdp = NULL;
- if (!gtp_dublicate(gsn, version, peer, seq)) + /* TODO describe what this is all about: */ + if (!gtp_duplicate(gsn, version, peer, seq)) return 0;
pdp = &pdp_buf; memset(pdp, 0, sizeof(struct pdp_t));
if (version == 0) { + /* TODO code dup: get_tid() */ uint64_t tid = be64toh(((union gtp_packet *)pack)->gtp0.h.tid);
pdp_set_imsi_nsapi(pdp, tid); @@ -1511,7 +1513,7 @@ int gtp_create_pdp_ind(struct gsn_t *gsn, int version, if (!pdp_getimsi(&pdp_old, pdp->imsi, pdp->nsapi)) { /* Found old pdp with same tid. Now the voodoo begins! */ /* 09.60 / 29.060 allows create on existing context to "steal" */ - /* the context which was allready established */ + /* the context which was already established */ /* We check that the APN, selection mode and MSISDN is the same */ DEBUGP(DLGTP, "gtp_create_pdp_ind: Old context found\n"); if ((pdp->apn_req.l == pdp_old->apn_req.l) @@ -1953,9 +1955,9 @@ int gtp_update_pdp_ind(struct gsn_t *gsn, int version, uint64_t imsi; uint8_t nsapi;
- /* Is this a dublicate ? */ - if (!gtp_dublicate(gsn, version, peer, seq)) { - return 0; /* We allready send of response once */ + /* Is this a duplicate ? */ + if (!gtp_duplicate(gsn, version, peer, seq)) { + return 0; /* We already sent of response once */ }
/* Decode information elements */ @@ -2085,7 +2087,7 @@ int gtp_update_pdp_ind(struct gsn_t *gsn, int version, }
/* TEIC (conditional) */ - /* If TEIC is not included it means that we have allready received it */ + /* If TEIC is not included it means that we have already received it */ /* TODO: From 29.060 it is not clear if TEI_C MUST be included for */ /* all updated contexts, or only for one of the linked contexts */ gtpie_gettv4(ie, GTPIE_TEI_C, 0, &pdp->teic_gn); @@ -2424,9 +2426,9 @@ int gtp_delete_pdp_ind(struct gsn_t *gsn, int version, int n; int count = 0;
- /* Is this a dublicate ? */ - if (!gtp_dublicate(gsn, version, peer, seq)) { - return 0; /* We allready send off response once */ + /* Is this a duplicate ? */ + if (!gtp_duplicate(gsn, version, peer, seq)) { + return 0; /* We already sent off response once */ }
/* Find the linked context in question */ @@ -2638,7 +2640,7 @@ int gtp_gpdu_ind(struct gsn_t *gsn, int version, return 0; }
-/* Receives GTP packet and sends off for further processing +/* Receives GTP packet and sends off for further processing. * Function will check the validity of the header. If the header * is not valid the packet is either dropped or a version not * supported is returned to the peer. diff --git a/gtp/gtp.h b/gtp/gtp.h index 76c967b..7f8ec91 100644 --- a/gtp/gtp.h +++ b/gtp/gtp.h @@ -147,7 +147,7 @@ struct ul16_t;
struct gtp0_header { /* Descriptions from 3GPP 09.60 */ uint8_t flags; /* 01 bitfield, with typical values */ - /* 000..... Version: 1 (0) */ + /* 000..... Version: 0 */ /* ...1111. Spare (7) */ /* .......0 SNDCP N-PDU Number flag (0) */ uint8_t type; /* 02 Message type. T-PDU = 0xff */ @@ -223,7 +223,7 @@ union gtp_packet { * application this struct is provided in order to store all * relevant information related to the gsn. * - * Note that this does not include information storage for ' + * Note that this does not include information storage for * each pdp context. This is stored in another struct. *************************************************************/
diff --git a/sgsnemu/sgsnemu.c b/sgsnemu/sgsnemu.c index 5b56751..13692e0 100644 --- a/sgsnemu/sgsnemu.c +++ b/sgsnemu/sgsnemu.c @@ -1569,7 +1569,7 @@ int main(int argc, char **argv) 512 = Flat rate, 256 = Hot billing */
/* Create context */ - /* We send this of once. Retransmissions are handled by gtplib */ + /* We send this off once. Retransmissions are handled by gtplib */ gtp_create_context_req(gsn, pdp, &iparr[n]); }
@@ -1577,9 +1577,9 @@ int main(int argc, char **argv)
printf("Waiting for response from ggsn........\n\n");
- /******************************************************************/ + /******************************************************************/ /* Main select loop */ - /******************************************************************/ + /******************************************************************/
while ((0 != state) && (5 != state)) {
@@ -1613,7 +1613,7 @@ int main(int argc, char **argv) state = 3; }
- /* Send off disconnect */ + /* Send disconnect */ if (3 == state) { state = 4; stoptime = time(NULL) + 5; /* Extra seconds to allow disconnect */ @@ -1628,17 +1628,23 @@ int main(int argc, char **argv) } }
- /* Send of ping packets */ + /* Send ping packets */ diff = 0; while ((diff <= 0) && /* Send off an ICMP ping packet */ - /*if ( */ (options.pinghost.s_addr) && (2 == state) && + /*if */ (options.pinghost.s_addr) && (2 == state) && ((pingseq < options.pingcount) || (options.pingcount == 0))) { + if (!pingseq) gettimeofday(&firstping, &tz); /* Set time of first ping */ gettimeofday(&tv, &tz); - diff = 1000000 / options.pingrate * pingseq - 1000000 * (tv.tv_sec - firstping.tv_sec) - (tv.tv_usec - firstping.tv_usec); /* Microseconds safe up to 500 sec */ + + /* Microseconds safe up to 500 sec */ + diff = 1e6 / options.pingrate * pingseq + - 1e6 * (tv.tv_sec - firstping.tv_sec) + - (tv.tv_usec - firstping.tv_usec); + if (diff <= 0) { if (options.debug) printf("Create_ping %d\n", diff);
On 12 Oct 2015, at 14:00, Neels Hofmeyr nhofmeyr@sysmocom.de wrote:
Hi!
Fix spelling dublicate -> duplicate in comments and in (apparently only statically used) gtp_dublicate().
Add two TODO comments.
Fix other spelling/punctuation and one numbering in comments.
Remove an opening brace from a comment to not mix up cindent in vim.
Break a long line.
I am sorry to be picky in this regard. If the subject of the change is to fix the typo in comments, code and function names, then please limit yourself to such a fix.
Is sed -i s,dublicate,duplicate,g enough for the first round?
gtp/gtp.c | 38 ++++++++++++++++++++------------------ gtp/gtp.h | 4 ++--
-int gtp_dublicate(struct gsn_t *gsn, int version, +int gtp_duplicate(struct gsn_t *gsn, int version, struct sockaddr_in *peer, uint16_t seq) {
- if (!gtp_dublicate(gsn, version, peer, seq))
/* TODO describe what this is all about: */
if (!gtp_duplicate(gsn, version, peer, seq)) return 0;
pdp = &pdp_buf; memset(pdp, 0, sizeof(struct pdp_t));
if (version == 0) {
/* TODO code dup: get_tid() */uint64_t tid = be64toh(((union gtp_packet *)pack)->gtp0.h.tid);
diff --git a/gtp/gtp.h b/gtp/gtp.h index 76c967b..7f8ec91 100644 --- a/gtp/gtp.h +++ b/gtp/gtp.h @@ -147,7 +147,7 @@ struct ul16_t;
struct gtp0_header { /* Descriptions from 3GPP 09.60 */ uint8_t flags; /* 01 bitfield, with typical values */
- /* 000..... Version: 1 (0) */
- /* 000..... Version: 0 */
diff --git a/sgsnemu/sgsnemu.c b/sgsnemu/sgsnemu.c index 5b56751..13692e0 100644 --- a/sgsnemu/sgsnemu.c +++ b/sgsnemu/sgsnemu.c @@ -1569,7 +1569,7 @@ int main(int argc, char **argv) 512 = Flat rate, 256 = Hot billing */
/* Create context */
/* We send this of once. Retransmissions are handled by gtplib */
/* We send this off once. Retransmissions are handled by gtplib */
- /******************************************************************/
- /******************************************************************/ /* Main select loop */
- /******************************************************************/
/******************************************************************/
while ((0 != state) && (5 != state)) {
@@ -1613,7 +1613,7 @@ int main(int argc, char **argv) state = 3; }
/* Send off disconnect */
if (3 == state) { state = 4; stoptime = time(NULL) + 5; /* Extra seconds to allow disconnect *//* Send disconnect */@@ -1628,17 +1628,23 @@ int main(int argc, char **argv) } }
/* Send of ping packets */
diff = 0; while ((diff <= 0) && /* Send off an ICMP ping packet *//* Send ping packets */
/*if ( */ (options.pinghost.s_addr) && (2 == state) &&
/*if */ (options.pinghost.s_addr) && (2 == state) && ((pingseq < options.pingcount) || (options.pingcount == 0))) {if (!pingseq) gettimeofday(&firstping, &tz); /* Set time of first ping */ gettimeofday(&tv, &tz);
diff = 1000000 / options.pingrate * pingseq - 1000000 * (tv.tv_sec - firstping.tv_sec) - (tv.tv_usec - firstping.tv_usec); /* Microseconds safe up to 500 sec */
/* Microseconds safe up to 500 sec */diff = 1e6 / options.pingrate * pingseq- 1e6 * (tv.tv_sec - firstping.tv_sec)- (tv.tv_usec - firstping.tv_usec);if (diff <= 0) { if (options.debug) printf("Create_ping %d\n", diff);-- 2.1.4
Fix spelling dublicate -> duplicate.
This is potentially breaking API compat, but currently, no users of gsn_t.dublicate are known. --- gtp/gtp.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/gtp/gtp.h b/gtp/gtp.h index 7f8ec91..4f96015 100644 --- a/gtp/gtp.h +++ b/gtp/gtp.h @@ -276,7 +276,7 @@ struct gsn_t { uint64_t tooshort; /* Number of too short headers 29.60 11.1.2 */ uint64_t unknown; /* Number of unknown messages 29.60 11.1.3 */ uint64_t unexpect; /* Number of unexpected messages 29.60 11.1.4 */ - uint64_t dublicate; /* Number of dublicate or unsolicited replies */ + uint64_t duplicate; /* Number of duplicate or unsolicited replies */ uint64_t missing; /* Number of missing information field messages */ uint64_t incorrect; /* Number of incorrect information field messages */ uint64_t invalid; /* Number of invalid message format messages */
Fix: the code handles fd1u but prints fd1c. --- gtp/gtp.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/gtp/gtp.c b/gtp/gtp.c index 2a39934..73ed76a 100644 --- a/gtp/gtp.c +++ b/gtp/gtp.c @@ -795,8 +795,8 @@ int gtp_new(struct gsn_t **gsn, char *statedir, struct in_addr *listen, if (bind((*gsn)->fd1u, (struct sockaddr *)&addr, sizeof(addr)) < 0) { (*gsn)->err_socket++; LOGP_WITH_ADDR(DLGTP, LOGL_ERROR, addr, - "bind(fd1c=%d) failed: Error = %s\n", - (*gsn)->fd1c, strerror(errno)); + "bind(fd1u=%d) failed: Error = %s\n", + (*gsn)->fd1u, strerror(errno)); return -1; }