| 00:25.36 | *** join/#asterisk-dev infobot (~infobot@208.53.50.136) |
| 00:25.36 | *** topic/#asterisk-dev is Asterisk Development Discussion -=- http://www.asterisk.org/developers -=- Tier 2 and 3.14159265 support is in #asterisk -=- Check out our blog! blogs.asterisk.org -=- Follow on Twitter at @AsteriskDev |
| 02:19.44 | *** join/#asterisk-dev infobot (ibot@208.53.50.136) |
| 02:19.44 | *** topic/#asterisk-dev is Asterisk Development Discussion -=- http://www.asterisk.org/developers -=- Tier 2 and 3.14159265 support is in #asterisk -=- Check out our blog! blogs.asterisk.org -=- Follow on Twitter at @AsteriskDev |
| 06:00.47 | *** join/#asterisk-dev tsearle (~tsearle@37.19.10.33) |
| 06:00.47 | *** mode/#asterisk-dev [+o tsearle] by ChanServ |
| 07:09.53 | *** join/#asterisk-dev jkroon (~jkroon@165.16.203.126) |
| 07:29.59 | *** join/#asterisk-dev hehol (~hehol@gatekeeper.loca.net) |
| 08:10.17 | jkroon | file, cresl1n, would either of you happen to be around? |
| 08:21.48 | jkroon | i would like to discuss the main sticky point from richard with respect to https://gerrit.asterisk.org/#/c/asterisk/+/9858/ (the chan_sip change that none of us really want to touch on). |
| 08:26.16 | jkroon | the last sticky point is how to handle tcp not matching the source port, my (and Alexander's) opinion is that it should only match against non-dynamic peers, whereas Richard is argueing against that towards original behaviour, which will again prevent Alexander's use-case. |
| 11:27.03 | *** join/#asterisk-dev hehol (~hehol@gatekeeper.loca.net) |
| 11:36.00 | *** join/#asterisk-dev jsmith (~jsmith@fedora/jsmith) |
| 11:36.00 | *** mode/#asterisk-dev [+o jsmith] by ChanServ |
| 12:50.58 | *** join/#asterisk-dev libardi (~libardi@177.194.124.58) |
| 14:03.28 | *** join/#asterisk-dev kharwell (kharwell@nat/digium/x-ylpvjywuxrphezhy) |
| 14:03.28 | *** mode/#asterisk-dev [+o kharwell] by ChanServ |
| 14:20.03 | jkroon | seanbright, would you prefer I rewrite that loop to utilize ast_asprintf? That would make sense. |
| 14:23.44 | seanbright | jkroon: it was just an observation to make that code a bit less verbose. doesn't require any action on your part unless it's an itch you would like to scratch. |
| 14:39.03 | *** join/#asterisk-dev hehol (~hehol@gatekeeper.loca.net) |
| 14:39.37 | jkroon | already scratched. |
| 14:39.46 | *** join/#asterisk-dev dakudos (~dakudos@c-73-243-106-97.hsd1.co.comcast.net) |
| 14:40.15 | seanbright | ast_asprintf doesn't return char * |
| 14:40.37 | jkroon | hangs head in shame. |
| 14:40.48 | seanbright | include/asterisk/astmm.h |
| 14:40.50 | seanbright | docs are in there |
| 14:41.01 | seanbright | you gotta at least test compile these things :-) |
| 14:41.13 | jkroon | i did. it compiled. |
| 14:41.16 | *** join/#asterisk-dev rmudgett (rmudgett@nat/digium/x-wqnvjnwqdrkfhgmm) |
| 14:41.16 | *** mode/#asterisk-dev [+o rmudgett] by ChanServ |
| 14:41.17 | seanbright | really? |
| 14:41.18 | seanbright | weird |
| 14:41.44 | jkroon | i know i've read some things in the past about implicit int to pointer casts ... still, should at least warn. |
| 14:41.57 | seanbright | when you run configure, are you passing --enable-dev-mode |
| 14:41.57 | seanbright | ? |
| 14:42.08 | jkroon | no i'm not. |
| 14:42.12 | seanbright | give that a whirl |
| 14:42.16 | jkroon | didn't even know such an option existed ... |
| 14:42.29 | seanbright | its a secret amongst the inner circle |
| 14:42.34 | *** join/#asterisk-dev bford (uid283514@gateway/web/irccloud.com/x-ceoqlxhmrgutrpel) |
| 14:42.34 | *** mode/#asterisk-dev [+o bford] by ChanServ |
| 14:42.35 | seanbright | not sure how i found out about it. |
| 14:42.55 | jkroon | it returns the length of the string excluding the \0, or -1 on failure? |
| 14:43.16 | jkroon | as per gnu asprintf |
| 14:43.22 | seanbright | if (ast_asprintf(...) < 0) { /* it failed */ } |
| 14:43.28 | jkroon | done. |
| 14:43.31 | jkroon | just recompiling again. |
| 14:52.09 | jkroon | and now it fails ... |
| 14:52.46 | stefan27 | it failed? |
| 14:53.53 | jkroon | yea, when I compile tested earlier it compiled, now I "fixed" it (plainly not), and now it works. must've not saved earlier. |
| 14:57.01 | jkroon | seanbright, there is a conflicting define for ast_asprintf in include/asterisk/utils.h |
| 14:57.38 | seanbright | jkroon: did you change the #includes in manager.c? |
| 14:57.47 | jkroon | no i did not touch them. |
| 14:58.03 | seanbright | but you're geting a compile failure about conflicting definitions? |
| 14:58.34 | jkroon | no, but i'm getting the version in utils.h, not not astmm.h. |
| 14:58.56 | jkroon | and for some reason it's complaining about my pointer to char* being const ... which I don't see how that's possible |
| 14:58.57 | seanbright | k |
| 14:59.43 | seanbright | because headers is a const char * array |
| 15:00.18 | seanbright | const char *headers[AST_MAX_MANHEADERS] |
| 15:00.18 | jkroon | just leave it? |
| 15:00.26 | seanbright | cast it |
| 15:00.37 | seanbright | ast_asprintf((char **) ... |
| 15:01.16 | jkroon | that works but generally i don't like that as a solution. OR I can use a temp ... |
| 15:01.27 | seanbright | that is what the previous code did |
| 15:01.32 | seanbright | snprintf((char *) m.headers[m.hdrcount], hdrlen, "%s: %s", v->name, v->value); |
| 15:02.09 | jkroon | indeed, you're right. ok, so use a temp char* which will gladly assign to the const, or use the cast? |
| 15:02.34 | file | I wish astmm gave me a memory address |
| 15:02.34 | seanbright | personally, i would cast it. but that's just me. |
| 15:02.52 | seanbright | if you're going to introduce a temp, you might as well just not touch that code at all and keep it how it is now |
| 15:02.56 | jkroon | i'm 50/50 so going with the cast. |
| 15:04.16 | seanbright | all if statements need braces |
| 15:14.41 | rmudgett | jkroon: hi |
| 15:17.39 | file | rmudgett: the COLP change is a bit in limbo... isn't it? |
| 15:17.58 | rmudgett | yes, yes it is |
| 15:18.19 | rmudgett | The problem really is his dialplan not being able to handle colp updates |
| 15:18.37 | file | hum |
| 15:19.01 | stefan27 | colp is file's last name |
| 15:19.05 | rmudgett | The only thing a patch can do is suppress the initial colp update |
| 15:22.31 | jkroon | hi rmudgett |
| 15:23.53 | jkroon | rmudgett, can we possibly discuss the sip/tcp situation please? would like to better understand why you disagree with the change? |
| 15:25.08 | jkroon | specifically, the part about port matching for host=dynamic connections (tcp|tls|ws|wss) |
| 15:26.42 | rmudgett | I'm leaning to allow what you are doing with the dynamic connections. It just will prevent matching if we do believe the contact header because NAT is not configured or there is no rport in the message. |
| 15:27.26 | rmudgett | The main problem is chan_sip's poor matching. The type=user peer is severely crippled because it doesn't allow outgoing calls. |
| 15:28.00 | rmudgett | Hence friend is the only thing you can use and that is giving the current grief because it matches by username and ip address |
| 15:28.19 | rmudgett | make that or ip address |
| 15:28.57 | seanbright | so i'm looking more at this manager patch, and this is kinda funny. http header parsing splits the headers into name value pairs, the manager glue code pastes them back together, and then the manager header functions split them up again |
| 15:29.02 | seanbright | just funny |
| 15:29.09 | rmudgett | A lot of people have wound up relying on matching by ip address for their endpoints leading to this quandry. |
| 15:30.07 | jkroon | rmudgett, yea, the chan_sip matching has always been confusing to me to begin with, but after having read the code i realized there was a bunch of undeterministic choices being made, which definitely did not help. |
| 15:30.27 | jkroon | indeterministic. |
| 15:30.32 | file | organic free range |
| 15:30.45 | rmudgett | random chickens! |
| 15:31.08 | jkroon | seanbright, i didn't even dig this far. my goal was to eliminate those verbose messages ... |
| 15:31.45 | jkroon | rmudgett, i've been wondering quite a lot about that point. |
| 15:33.19 | jkroon | i get where alexander is coming from and the old code definitely won't deal with what he needs. |
| 15:34.02 | rmudgett | I was looking at why a type=user could not dial as a result of this. I think it might be trivial to allow users to dial. However, that would be a real behavior change this late in chan_sip's life. |
| 15:34.18 | file | I would -2 that so fast |
| 15:34.39 | file | at this point it seems every change to chan_sip does SOMETHING unexpected |
| 15:34.57 | jkroon | rmudgett, because it's matching on some IP first? |
| 15:35.53 | rmudgett | I think the original reporter on the mailing list was relying on the friend matching by IP and not username. |
| 15:35.56 | jkroon | https://community.asterisk.org/t/peer-matching-in-asterisk-using-chan-sip/74281 <-- sample of more confusion that this change might actually fix. |
| 15:36.30 | jkroon | although, the config he provides there is insane to begin with. |
| 15:39.42 | rmudgett | The order of peer definition is still critical to matching. As a huristic type=peer should be listed first followed by type=firend That way the peers would be found first by IP address and the friends would be found first by username before being found by IP |
| 15:40.24 | seanbright | jkroon: yeah, it's not relevant to your patch and changing it would be pretty invasive. just something i noticed. |
| 15:40.39 | jkroon | rmudgett, if you've got an example or test case that I've not thought of that currently works that might break, please do send that to me. I'll happily take a look, I know the patch started merely to get TCP|TLS for host!=dynamic working again, but I think it's become a bit more than that now. |
| 15:41.35 | jkroon | imho (and unfortunately I'd shoot myself in the foot at this stage) chan_sip needs to be retired soon. |
| 15:41.37 | rmudgett | Its nearly there now though. |
| 15:41.52 | jkroon | have you scoped out patch set 7? |
| 15:41.55 | seanbright | chan_sip is going nowhere i'm afraid |
| 15:42.30 | jkroon | nowhere could be a void. anyway, /dev/fiance is waiting, i better head off as in right now. |
| 15:42.43 | jkroon | adious folks, will follow up again a bit later. |
| 15:42.47 | seanbright | your priorities are all mixed up |
| 15:43.47 | rmudgett | On to the other item. The dead routine that adds confusion to analyzing the matching routine cannot be called. That routine could only be called if the container were searched using the ao2_find() function. Nothing uses ao2_find() with that container so the function cannot be called. |
| 15:44.31 | rmudgett | Heh. Missed him. |
| 17:07.53 | *** join/#asterisk-dev gtjoseph (~gtjoseph@unaffiliated/gtj) |
| 17:07.53 | *** mode/#asterisk-dev [+o gtjoseph] by ChanServ |
| 17:46.35 | cresl1n | Is anybody else planning on +1'ing or +2'ing the DUNDi reviews? |
| 17:46.53 | file | I had it on my list to look over, but haven't been in the mood for it |
| 17:48.17 | cresl1n | "You fight when the necessity arisesâno matter the mood! Mood's a thing for cattle or making love or playing the baliset. It's not for fighting.â âBehold, as a wild ass in the desert, go I forth to my work.â" - Gurney Halleck |
| 17:48.21 | cresl1n | :-P |
| 17:48.27 | cresl1n | I'm just kidding of course |
| 17:48.42 | seanbright | i haven't been called a wild ass in the desert since high school |
| 17:48.56 | file | Evil Meeseeks could have said that.... |
| 17:48.57 | cresl1n | Darn, I think that was a mix of two quotes |
| 17:49.09 | cresl1n | Failed google search |
| 18:32.24 | *** join/#asterisk-dev jkroon (~jkroon@165.16.204.169) |
| 19:09.36 | jkroon | rmudgett, ok, so to get rid of peer_ipcmp_cb(), is it safe to pass NULL for that function pointer? |
| 19:10.17 | rmudgett | Yes. If NULL is passed it uses the default match function which matches anything. |
| 19:10.59 | jkroon | would you prefer I nuke that before merging then? |
| 19:11.47 | rmudgett | yes. It is "related" in that it reduces confusion analyzing the code. |
| 19:11.55 | jkroon | regarding the other suggestions for TCP, your comments in the sample code snippet does make sense to me. |
| 19:13.31 | jkroon | don't know WS and WSS semantics that much, so I'll have to take your word on that, the rest actually makes sense too, assuming I understand it correctly. There is rport auto settings too IIRC, will that end up setting SIP_NAT_FORCE_RPORT for us to use here? |
| 19:13.43 | rmudgett | Yeah. In the original reporter's case the roaming users were always behind a NAT and that is the most likely scenario when there would be multiple peers coming from the same address. |
| 19:15.01 | jkroon | fully agree with that, just want to make sure we don't break the nat=auto ... looks like set_peer_nat does what I was hoping for there. In which case I'll incorporate that suggestion too. |
| 19:15.22 | rmudgett | It is parse_register_contact() that determines if we believe the contact header. |
| 19:16.26 | rmudgett | The only flag we don't have access to is SIP_NAT_RPORT_PRESENT which I think we can ignore |
| 19:17.27 | jkroon | and set_peer_nat is called from various places, amongst others (mostly) functions that seems to at a glance relate to registration. So I think we're good on that one. |
| 19:17.59 | jkroon | and that would also cover the case where we have a non-NAT registration (more) correct. adjustments to changelog? |
| 19:18.58 | rmudgett | Yes. set_peer_nat() is called before parse_register_contact(). |
| 19:20.22 | rmudgett | You could update the commit message a bit but it is getting tedious. You have some tabs in there which should be fixed. |
| 19:22.46 | rmudgett | Other than what I've already mentioned in previous comments and the tabs in the commit message I don't have anything else. |
| 19:23.43 | jkroon | ok, i'll just make mention of it where I mention WS and WSS right at the end. I do agree that the changelog is more troublesome than the code itself. even wondered if some of that shouldn't really be comments in the code, but git blame will find the information for someone that's looking. |
| 19:26.56 | jkroon | i note you don't return CMP_STOP in your sample code - implications? |
| 19:27.27 | rmudgett | jkroon: seanbright made a change to your manager header patch which your cherry-picks didn't pick up. It didn't look like you used the gerrit cherry-pick button or there were conflicts and you couldn't. |
| 19:27.57 | rmudgett | Most uses of CMP_STOP are unnecessary. There are very few cases where it makes a difference. |
| 19:28.45 | jkroon | there was a conflict due to an added variable in the auth callback. |
| 19:28.59 | rmudgett | k |
| 19:29.12 | jkroon | will look at that again as soon as I'm done with the SIP one now :) |
| 19:29.20 | seanbright | (sorry) |
| 19:30.15 | jkroon | no stress - we all want the best possible code, and that means that one has to look and learn from those that's more familiar with the code. I would still like to understand the difference between CMD_STOP or not :) |
| 19:33.17 | jkroon | then again: serious? Guess it's like one jmp instruction less in the binary ... but the compiler really should optimize that anyway. I'll amend. |
| 19:35.14 | jkroon | if you want to be *that* pedantic, comparison against zero implies the CPU don't have to perform a subtraction, so >= 0 ... |
| 19:36.41 | seanbright | you're talking about my change? it's just fewer lines of code. it wasn't meant as a performance improvement. |
| 19:36.51 | seanbright | it's your patch, feel free to roll that bit back |
| 19:40.10 | jkroon | i just felt my version was a tad more clear (something I tend to prefer over brevity) |
| 19:40.51 | seanbright | by all means. just remember that the if statement needs the braces. |
| 19:40.53 | jkroon | really boils down to the same thing ... i'm happy with either option ... ? |
| 19:41.16 | jkroon | got it. always brace :). |
| 19:41.51 | rmudgett | yep |
| 19:42.03 | jkroon | ok, so how do i "roll back" or state that we rather merge 7 than 8? |
| 19:42.56 | jkroon | nm, need to add braces ... |
| 19:43.29 | rmudgett | The CMD_STOP stops container traversal immediately. If you are looking for only one item then stop does nothing when you find something. If you are looking for OBJ_MULTIPLE items then using it prevents you from finding more than one anyway. |
| 19:44.08 | jkroon | rmudgett, thanks for that explanation. |
| 19:44.30 | jkroon | seanbright, going with your version of comparing against -1. |
| 20:01.14 | jkroon | how difficult would it be to allow documentation reload? i had a case a few days back where i had to restart asterisk to get the opus documentation to load so that the module would load? (not a major concern, just wondering if there's another itch I can scratch) |
| 20:07.44 | file | I don't think anyone here has the experience required in that area to really be able to answer that query |
| 20:09.16 | jkroon | shot. will scratch at a later stage. |
| 20:11.57 | file | there is a comment saying that ref counting would need to done; |
| 20:12.02 | file | https://www.irccloud.com/pastebin/HkZhf1GB/ |
| 20:14.34 | jkroon | that would make sense. it doesn't bug me enough to make that effort. |
| 20:21.52 | jkroon | file, rmudgett, seanbright - thanks a lot for all the assistance and advise, much appreciated. good night. |
| 20:22.07 | rmudgett | bye |
| 21:03.14 | *** join/#asterisk-dev jameswf (uid27319@gateway/web/irccloud.com/x-fzuqsdlskfaqglqq) |
| 22:57.09 | *** part/#asterisk-dev kharwell (kharwell@nat/digium/x-ylpvjywuxrphezhy) |