MCCP Fix Revision 3. Removed an un-needed bool, and a good bit of ext… by deathlire · Pull Request #1024 · Mudlet/Mudlet

deathlire · 2017-05-18T01:11:15Z

…ra code that isn't needed for compression which fixes a few other bugs with it. -MH

… code for MCCP, also tested farely well. -MH

…ra code that isn't needed for compression which fixes a few other bugs with it. -MH

vadi2 · 2017-05-18T09:41:55Z

Awesome, thanks so much! Will have a look at testing this. Could you let us know what were the bugs with it, so we can confirm they're gone?

deathlire · 2017-05-19T00:02:29Z

Actually I shouldn't have said bugs, but more like it using the protocol a bit in the wrong way which could lead to bugs happening for certain oddly coded muds with weird compression schemes that try and pass off the wrong version. Mostly got rid of all the bloat and just did it more correctly, after an 18 hour test run of the code it not only seemed stable but also don't actually have to disconnect anymore if you turn force compression off back to on. (If a mud has a compress or +compress command to turn it back on anyways.) Got rid of conflicting code that just wasn't needed and took up cpu time. Another is a bug which may be totally unrelated or just someone else's cache or properties, but when i do a copyover sometimes it would go blank at password and never turn back on, could have also been mud code. But haven't been able to replicate it myself after this fix.

deathlire · 2017-05-19T00:06:04Z

May make a later revision, that also turns compression back on automatically if force off compression is unchecked after being checked and to remove the commented out code. And perhaps fix a few other things I noticed that may behave a little oddly here in a few days or so once this is confirmed to have worked for the masses.

vadi2 · 2017-05-20T10:55:35Z

Sorry you've kinda lost me here. I don't know what exactly changed here in terms of functionality, I see that code is changed - how does it change the behaviour? I'm not familiar with MCCP internals and

using the protocol a bit in the wrong way which could lead to bugs happening for certain oddly coded muds with weird compression schemes that try and pass off the wrong version

Is kind of super vague :(

Basically, what was the driver for this change so I'm on the same page as you?

vadi2 · 2017-05-25T04:03:02Z

Any updates on this? I'd love to have the improvements but I have no idea what's going on here to be able to test the before / after behaviour.

deathlire · 2017-05-25T04:14:35Z

Well to be complete on it.. I've been using the fix for about a week now with no issues at all with popping on or off of compression and copyover's no problem.. have done about 500 copyovers to test and also switching of compression on and off over a few thousand times with timers. had no issues. does this fix need to be redone again? notice conflicting file. (Also if you are really wondering it was mostly an issue of having un-needed code that was messing up the loop a little bit, but this fix gets rid of defunct code and an un-needed boolean that is no longer needed. Nothing to hard core was changed.)

vadi2 · 2017-05-25T04:42:02Z

Yeah but as you'd know, every MUD driver is different and has different bugs - might work for you - might not for someone else. I need to be able to test this myself across a range of MUDs to make sure we don't break it for someone else hey :)

I'll give it a go and report back!

Don't worry about the merge conflict - we broke your PR and we'll fix it. If you're really curious though, read up on git conflict resolution, it's something you'll need to do eventually in a distributed development environment haha.

deathlire · 2017-05-25T04:50:23Z

Yeah i'll read up on those.. i haven't pushed the newer one with the commented code that is cut out, and do appreciate the testing for elsewhere. was just an automatic type of testing i've been doing with it.. my server shuts off compression before a copy over is done, and then resends compress_will after the copyover and mudlet catches right on every time and switches back on. stays off if you force it off, has no problem at all catching on to MCCP v2. So yes testing on a mud that has MCCP v1 would be a good idea as I have dropped complete support for version 1 from my server also on another note the support tag can be added soon as i know people are done with TBuffer.cpp for the other bug. can just enable all support or just what is needed.

deathlire · 2017-05-25T04:52:32Z

seems to need mEncoding in there for build :)

vadi2 · 2017-05-25T04:55:50Z

Oops. If you do a git pull, you'll get the new code, mind adding it in?

deathlire · 2017-05-25T05:31:06Z

Tried, do i need to make a new PR for it?

vadi2 · 2017-05-25T05:44:16Z

Nope, just commit and push, the PR will auto-update.

deathlire · 2017-05-25T06:38:52Z

Finally got it, had some issues with my fork.

deathlire

Don't know that got in there, but don't see anything bad with it 👍

vadi2 · 2017-05-25T06:48:49Z

Didn't you cut mWaitingForCompressedStreamToStart away though?

…ved from ctelnet.h as well -MH

deathlire · 2017-05-25T06:54:44Z

Yeah not sure how it slipped back in, but at least that made me also think to get rid of it in ctelnet.h as well. So it's all good to go now once the check passes.

SlySven

I suspect most of this is good, but I am not sure whether we are/were correctly implementing the Telnet sub-option negotiation process here anyhow...

SlySven · 2017-05-25T19:57:48Z

src/ctelnet.cpp

-        // Reset the option state so we can re-enable compression again in the future
-        // such as in the case of a copyover -JM
-        hisOptionState[static_cast<int>(OPT_COMPRESS)] = false;
-        hisOptionState[static_cast<int>(OPT_COMPRESS2)] = false;


Are you sure you should be deleting these? if I understand (and I am not sure that I do 🤕 ) the correct procedure ("Q Method" ?) for negotiating Telnet Sub-Options (i.e. RFC1143) it means we should be tracking the option state for all options and this is part of that...

I think we are also broken on some other options as well but the whole Telnet option negotiating thingy is something I still have to get my head around.

Thing is though when those option states are false they never go back to true, and is unneccesary to keep an eye on them, if hisOptionState[foo] = false .. and then server requests it again it stays false, but you can have those, that is the only reason I just left them commented out and didn't get rid of them. They actually don't fix copyover or anything else, perhaps they did for something a good while ago, not sure. But anyways once it's negotiated it's done for unless you plan to have more buttons for turning off and on compression by sending a WANT for it and such and hope the server listens. Other than that I don't see a reason to keep an eye on something that can be found out by .. if (OPT_COMPRESS || OPT_COMPRESS2) do this or that.. I'm not a C++ expert by any means either, I do mostly just C. But can tinker with c++ and fix things.

Haven't had a reason for adding buttons for this so we won't be cluttering up the UI! I agree it seems that dead code can go.

As I understand it the default state for any Telnet sub-option must be false so your:

Thing is though when those option states are false they never go back to true, and is unneccesary to keep an eye on them...

is puzzling to me - but is that is my problem? 😖 Or should we retain just those two lines (and the comment)?

I think you do need to keep those two hisOptionState up to date because they would be needed for a Telnet Status sub-option report which we DO support (see code for OPT_STATUS i.e. option 5).

Fair point, we should report it back when asked about it - and we need to keep track of it in order to do that.

SlySven · 2017-05-25T19:58:31Z

src/ctelnet.cpp

+        qDebug() << "Listening for new compression sequences";
+        return -1;
    }
+    /* TODO: Remove this commented code once it is deemed safe. -MH


By whom, and how? 😀

We can probably rid of it once PR passes testing!

Only left it there just in case it was something you may need. To work correctly it doesn't need that and knows what's going on. So up to you on that one :)

SlySven · 2017-05-25T20:00:18Z

src/ctelnet.cpp

+
+    // initialize default encoding
+    mEncoding = "UTF-8";
+    encodingChanged(mEncoding);


I think these lines came from (my) #969...

vadi2

Seems to work fine! I've tested on Achaea, DSL MUD, Midnight Sun 2.

Thanks :)

Over to you @SlySven for review.

vadi2 · 2017-06-03T16:06:35Z

@SlySven could you review this please?

SlySven

I want those two hisOptionState being reset so that the MUD server would get our correct understanding of what the remote end is set/reset for if it requested a status report... after that I'd be willing to approve the rest of the code...

vadi2 · 2017-06-04T07:05:33Z

In the interest of time I've applied the change.

deathlire · 2017-06-04T07:06:56Z

This point is moot. No mud server asks our state without sending a request to send one, and I'm not clear on C++ or this function hisOptionState ..but no mud server needs it in the first place, this is all client side.

deathlire · 2017-06-04T07:08:18Z

Where are you going to set them to true? can't see the changes..? a server asks WILL DO or WONT ..they do not care if you have a state of being in that i know of..if not please correct me.

SlySven · 2017-06-04T14:23:07Z

Unless I am reading the code wrong old/new line 688/690 in cTelnet.cpp contains hisOptionState[idxOption] = true; which should set the bit, but only if it has been previously cleared (before a copy-over?):

               if( !hisOptionState[idxOption] )
               {
                    //only if this is not set; if it's set, something's wrong wth the server
                    //(according to telnet specification, option announcement may not be
                    //unless explicitly requested)
 
                    if(
                        ( option == OPT_STATUS ) ||
                        ( option == OPT_TERMINAL_TYPE) ||
                        ( option == OPT_ECHO ) ||
                        ( option == OPT_NAWS ) )
                    {
                        sendTelnetOption( TN_DO, option );
                        hisOptionState[idxOption] = true;
                    }
                    else if( ( option == OPT_COMPRESS ) || ( option == OPT_COMPRESS2 ) )
                    {
                        //these are handled separately, as they're a bit special
                        if( mpHost->mFORCE_NO_COMPRESSION || ( ( option == OPT_COMPRESS ) && ( hisOptionState[static_cast<int>(OPT_COMPRESS2)] ) ) )
                        {
                            //protocol says: reject MCCP v1 if you have previously accepted MCCP v2...
                            sendTelnetOption( TN_DONT, option );
                            hisOptionState[idxOption] = false;
                            qDebug() << "Rejecting MCCP v1, because v2 has already been negotiated or FORCE COMPRESSION OFF is set to ON.";
                        }
                        else
                        {
                            sendTelnetOption( TN_DO, option );
/* HERE ==> */              hisOptionState[idxOption] = true;
                            //inform MCCP object about the change
                            if( option == OPT_COMPRESS )
                            {
                                mMCCP_version_1 = true;
                                qDebug() << "MCCP v1 negotiated.";
                            }
                            else
                            {
                                mMCCP_version_2 = true;
                                qDebug() << "MCCP v2 negotiated!";
                            }
                        }
                    }
                    else if( supportedTelnetOptions.contains( option ) )
                    {
                        sendTelnetOption( TN_DO, option );
                        hisOptionState[idxOption] = true;
                    }
                    else
                    {
                        sendTelnetOption( TN_DONT, option );
                        hisOptionState[idxOption] = false;
                    }
                }
           }

vadi2 · 2017-06-04T14:25:31Z

Yep, the option is back in c3a6185, and I think we've exhausted the limits of useful conversation of this - let's move on. Is this good to go?

SlySven

OK, glad we got that option tracking stuff sorted (I just hope all the other sub-options are also done correctly - but that will take more study and is not the target of this PR 😈) so

vadi2 · 2017-06-04T16:12:27Z

I think we reached a good middle ground - got rid of uneeded code without sactificing functionality. Thanks @deathlire for the improvement to Mudlet, hope to see more 😄

deathlire added 2 commits May 17, 2017 19:33

Removed a bool for waiting on compression, removed a lot of un-needed…

f964837

… code for MCCP, also tested farely well. -MH

MCCP Fix Revision 3. Removed an un-needed bool, and a good bit of ext…

9bc4d42

…ra code that isn't needed for compression which fixes a few other bugs with it. -MH

vadi2 requested review from SlySven and vadi2 May 18, 2017 09:41

vadi2 added the 1 - Ready label May 18, 2017

vadi2 assigned deathlire May 25, 2017

Merge branch 'development' into development

c3886fb

deathlire added 2 commits May 25, 2017 01:55

Fix for mEncoding from mistyped 'encoding'

c768a9e

Fix for mEncoding from mistyped 'encoding'

4317578

deathlire commented May 25, 2017

View reviewed changes

Somehow mWaitingForCompressedStreamToStart got put back in, also remo…

0765529

…ved from ctelnet.h as well -MH

SlySven reviewed May 25, 2017

View reviewed changes

vadi2 approved these changes May 31, 2017

View reviewed changes

vadi2 assigned SlySven and unassigned deathlire May 31, 2017

vadi2 added this to the 3.2 milestone Jun 1, 2017

SlySven reviewed Jun 3, 2017

View reviewed changes

Re-added state tracking for mccp, needed for reporting back to server

c3a6185

SlySven approved these changes Jun 4, 2017

View reviewed changes

vadi2 merged commit 09b785e into Mudlet:development Jun 4, 2017

vadi2 mentioned this pull request Sep 9, 2019

Mudlet fails to negotiate MCCPv1 #3056

Closed

Uh oh!

Conversation

deathlire commented May 18, 2017

Uh oh!

vadi2 commented May 18, 2017

Uh oh!

deathlire commented May 19, 2017

Uh oh!

deathlire commented May 19, 2017

Uh oh!

vadi2 commented May 20, 2017

Uh oh!

vadi2 commented May 25, 2017

Uh oh!

deathlire commented May 25, 2017

Uh oh!

vadi2 commented May 25, 2017

Uh oh!

deathlire commented May 25, 2017

Uh oh!

deathlire commented May 25, 2017

Uh oh!

vadi2 commented May 25, 2017

Uh oh!

deathlire commented May 25, 2017

Uh oh!

vadi2 commented May 25, 2017

Uh oh!

deathlire commented May 25, 2017

Uh oh!

deathlire left a comment

Choose a reason for hiding this comment

Uh oh!

vadi2 commented May 25, 2017

Uh oh!

deathlire commented May 25, 2017

Uh oh!

SlySven left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SlySven May 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vadi2 left a comment

Choose a reason for hiding this comment

Uh oh!

vadi2 commented Jun 3, 2017

Uh oh!

SlySven left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vadi2 commented Jun 4, 2017

Uh oh!

deathlire commented Jun 4, 2017

Uh oh!

deathlire commented Jun 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SlySven commented Jun 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

SlySven May 25, 2017 •

edited

Loading

SlySven left a comment •

edited

Loading

deathlire commented Jun 4, 2017 •

edited

Loading

SlySven commented Jun 4, 2017 •

edited

Loading