**** BEGIN LOGGING AT Thu Apr 08 02:59:56 2010 Apr 08 11:10:07 * Djdas is away: Away Apr 09 00:23:20 denkenz: ping Apr 09 00:50:06 holtmann: So it seems the ppp stack is working for me on HSO (though sometimes it takes upwards of 2 minutes to get an IP address), see if it still works on your cards Apr 09 00:50:15 holtmann: I do some testing on MBM tomorrow Apr 09 00:51:38 kristen: pong Apr 09 00:54:03 denkenz: sorry - having connection issues. hopefully I'll stop dropping out now. Apr 09 00:54:32 denkenz: I reviewed your option handling code, and I've got some questions. Apr 09 00:55:02 sure Apr 09 00:55:48 what happens if we need to nak one option, but all other options are acceptable? same with reject - if we need to reject a subset of the received options, but not all of them. Apr 09 00:56:36 we actually can Apr 09 00:57:03 how does that work? Apr 09 00:57:53 So in rcr callback, if you need to reject a subset, you generate a new option array and return it in new_options Apr 09 00:58:24 Same thing for naks, if you need to suggest new set of options, generate a new option array Apr 09 00:58:41 but where do you apply the options you don't want to reject or nak? It looks like you return immediately when you reject something? Apr 09 00:58:42 The LCP code doesn't do this yet Apr 09 00:58:54 ah ok - that is where I'm getting confused. Apr 09 00:59:16 ipcp didn't seem to do this either. Apr 09 00:59:21 options which are not rejected are resubmitted by the peer Apr 09 00:59:32 So you don't apply them when generating nak/rej Apr 09 00:59:59 resubmitted? Apr 09 01:00:23 yes, so lets say I submit IP, DNS, DNS2 and the peer naks IP Apr 09 01:00:31 ok Apr 09 01:00:46 I have to re-submit IP with peer's idea, and then DNS1 and DNS2 again Apr 09 01:00:53 Once the peer acks, then I apply the options Apr 09 01:01:20 so by resubmit you mean include this option in a new Configure-Request? Apr 09 01:01:44 yes Apr 09 01:02:04 hum - that's a little bit different than the way the spec expects you to behave. Apr 09 01:02:48 I don't think so, but quote the region where you think it says that Apr 09 01:02:55 * kristen double checks. Apr 09 01:03:42 For instance, my Option modem conf-naks everything except the IP until it gprs-attaches Apr 09 01:03:55 Then it conf-naks the IP and DNS entries with proper ones Apr 09 01:04:03 And only then conf-acks Apr 09 01:04:27 So I get conf-naks with DNS1 of 11.12.13.14 and DNS2 of 11.12.13.15 Apr 09 01:04:39 Then eventually conf-nak with real numbers + IP Apr 09 01:05:24 ok - I think you are fine. I was confusing myself. Apr 09 01:08:05 ok - I think I understand the code now, thanks for answering my questions. I need to add a Configure-Nak for magic number for loopback detection, so if it's not clear I'll be back. Apr 09 01:09:21 sure Apr 09 01:11:26 I guess the allocation of the new_option is expected to be dynamic (i.e. you take care of freeing them.) Apr 09 01:13:32 yep, I'm not particularly fond of the current solution, but it was getting late in the day and I didn't have anything better Apr 09 01:14:07 denkenz: I think one other potential pitfall I see is that in the switch statement in lcp_rcr the instant you find something to reject (or soon-to-be nak), you return, and don't look for multiple items. Apr 09 01:15:07 I know, that part needs to go option by option and anything it doesn't recognize or is invalid has to be copied to the reject list Apr 09 01:15:11 and it is going to be a little bit of a pain to allocate space for nak'd options because we won't know till we're done scanning them how much space to allocate. So at the moment, it almost seems like you'll have to iterate more than once. Apr 09 01:15:46 yes - I saw that you got rid of the reject and unacceptable option lists, but I'm thinking you might still need them. Apr 09 01:15:47 Nah, easy solution is to just allocate the entire space for the reject equal to the conf-req Apr 09 01:16:11 even if you fill in part of it is fine Apr 09 01:16:21 sure - I guess we don't care about being careful with memory. Apr 09 01:16:30 Doing a double iteration is also fine Apr 09 01:17:02 yes - it is just not as efficient as possible, but no doubt not worth worrying about. Apr 09 01:17:17 Probably ends up more efficient than doing lots of mallocs Apr 09 01:17:49 sounds like a job for the obsessive compulsive optimizer to figure out. Apr 09 01:18:30 In the end, if we find that unacceptable list is required, we can add it back in Apr 09 01:18:56 For now this was the simplest solution and with the amount of options we deal in, the easiest to implement / understand Apr 09 01:22:21 seems entirely feasible to have multiple items to nak. might be worth a comment in the code to indicate that this might be a future issue. Apr 09 01:23:42 actually this was just copy-paste + make compile of the old code Apr 09 01:24:02 We never really nak anything, so I wanted it pushed so everyone could comment Apr 09 01:24:07 It is by no means finished Apr 09 01:24:49 patches are welcome, and I don't guarantee I got everything correct Apr 09 01:27:57 yeah the old lcp code was designed for different logic obviously. i can work with what you have now for sure. Apr 09 02:19:38 denkenz: I will do some testing with my SIM cards and modem here. Lets see how it works out. Apr 09 02:54:42 denkenz: In ipcp_data struct I prefer you use a bitmask for "requested" and not gbooleans. The boolean in GLib are actually a waste of memory. Apr 09 02:55:59 holtmann: That's a good suggestion actually Apr 09 02:56:04 denkenz: Same for lcp_data please. Even if it is not saving us anything right now. Apr 09 02:56:27 holtmann: Remember, I did this in about 5 hours ;) Apr 09 02:57:39 I was just going through the patches and doing a quick review ;) Apr 09 02:57:42 if (timer_data->restart_timer) { Apr 09 02:58:10 And in cases for the timer checks I prefer you do them clearly with "if (timer_id > 0)". Apr 09 02:58:52 Personally I hate the guint of timers etc. and that 0 means invalid. Apr 09 02:59:27 Fix that part, it was actually existing code Apr 09 02:59:52 I will eventually when I go through it again. It is not that urgent. **** ENDING LOGGING AT Fri Apr 09 02:59:56 2010