[PATCH] opensgsn: Replace some TODOs with error paths and fix compiler warning
openggsn: Check return codes and take error paths on failure. Return early when socket() returns -1, and check return codes where indicated by some TODOs. This removes 2 TODOs and fixes a compiler warning about assignment to a variable which then isn't used. Signed-off-by: Michael McTernan <mike.mcternan@wavemobile.com> Index: osmobss/openggsn/lib/tun.c =================================================================== --- osmobss.orig/openggsn/lib/tun.c 2015-04-28 10:51:37.960449539 +0100 +++ osmobss/openggsn/lib/tun.c 2015-04-28 10:54:40.301130739 +0100 @@ -117,6 +117,7 @@ int tun_sifflags(struct tun_t *this, int ifr.ifr_name[IFNAMSIZ - 1] = 0; /* Make sure to terminate */ if ((fd = socket(AF_INET, SOCK_DGRAM, 0)) < 0) { SYS_ERR(DTUN, LOGL_ERROR, errno, "socket() failed"); + return -1; } if (ioctl(fd, SIOCSIFFLAGS, &ifr)) { SYS_ERR(DTUN, LOGL_ERROR, errno, @@ -318,9 +319,19 @@ int tun_addaddr(struct tun_t *this, req.n.nlmsg_seq = 0; req.n.nlmsg_flags |= NLM_F_ACK; - status = sendmsg(fd, &msg, 0); /* TODO Error check */ + status = sendmsg(fd, &msg, 0); + if (status == -1) { + SYS_ERR(DTUN, LOGL_ERROR, errno, "sendmsg() failed"); + close(fd); + return -1; + } + + status = tun_sifflags(this, IFF_UP | IFF_RUNNING); + if (status == -1) { + close(fd); + return -1; + } - tun_sifflags(this, IFF_UP | IFF_RUNNING); /* TODO */ close(fd); this->addrs++; return 0;
On 28 Apr 2015, at 12:06, Mike McTernan (wavemobile) <mike.mcternan@wavemobile.com> wrote:
hi!
+ status = sendmsg(fd, &msg, 0); + if (status == -1) {
this error check is not complete. sendmsg returns the number of bytes sent. This means we should check if everything we wanted to send has been sent (we were bitten by truncated LLE frames in GPRS-NS early on). Do you have time to extend this check? holger
Hi Holger,
this error check is not complete. sendmsg returns the number of bytes sent.
Whoops!
Do you have time to extend this check?
Sure. I've fixed this in the attached v2 of the patch. It was tested by running the following to make sure that code path was reached and passed (which was also verified with a temporary printf): ./sgsnemu --contexts=4 -l 127.0.0.1 -r 127.0.0.2 -d --createif --net 0.0.0.0 Kind Regards, Mike
On 28 Apr 2015, at 13:59, Mike McTernan (wavemobile) <mike.mcternan@wavemobile.com> wrote:
Sure. I've fixed this in the attached v2 of the patch. It was tested by running the following to make sure that code path was reached and passed (which was also verified with a temporary printf):
Which version of OpenGGSN are you using? Where did you check it out? What is the last git commit? I ask because the patch does not apply and the line numbers in tun.c are quite different. I assume you are not getting it from git.osmocom.org? Do you? Where did we point to the SF repo? holger
Hi Holger, Sorry about this - I know you are busy.
Which version of OpenGGSN are you using? Where did you check it out? What is the last git commit? I ask because the patch does not apply and the line numbers in tun.c are quite different.
That's odd. I've cloned from git.osmocom.org. Just taking a fresh tree, it applies cleanly here: $ git clone git://git.osmocom.org/openggsn.git Cloning into 'openggsn'... remote: Counting objects: 1401, done. remote: Compressing objects: 100% (530/530), done. remote: Total 1401 (delta 990), reused 1194 (delta 850) Receiving objects: 100% (1401/1401), 887.13 KiB | 1.58 MiB/s, done. Resolving deltas: 100% (990/990), done. $ cd openggsn $ git apply -p2 ../../osmobss/patches/Check-and-fix-tun-setup-error-paths-v2.patch $ git status # On branch master # Changes not staged for commit: # (use "git add <file>..." to update what will be committed) # (use "git checkout -- <file>..." to discard changes in working directory) # # modified: lib/tun.c # no changes added to commit (use "git add" and/or "git commit -a") One thing I should probably have mentioned is that it needs -p2 since I'm using quilt to manage patch sets across the various libosmo*, openbsc, openggsn, bts etc... so it's one dir up. Did I do something wrong in the above? Kind Regards, Mike
participants (2)
-
Holger Freyther -
Mike McTernan (wavemobile)