From 1ae23b4dccb37e3cd335e0371855c5ca8be63891 Mon Sep 17 00:00:00 2001 From: John Hood Date: Mon, 7 Nov 2016 00:52:19 -0500 Subject: [PATCH] Prefer early exit/break to large conditional blocks. Some of these are large and contain smaller unrelated refactors. --- src/frontend/stmclient.cc | 126 ++++++++++++++++---------------- src/frontend/terminaloverlay.cc | 46 ++++++------ src/network/network.cc | 107 ++++++++++++++------------- src/terminal/terminaldisplay.cc | 75 +++++++++---------- 4 files changed, 176 insertions(+), 178 deletions(-) diff --git a/src/frontend/stmclient.cc b/src/frontend/stmclient.cc index 0720b24..22f4d75 100644 --- a/src/frontend/stmclient.cc +++ b/src/frontend/stmclient.cc @@ -314,82 +314,82 @@ bool STMClient::process_user_input( int fd ) NetworkType &net = *network; - if ( !net.shutdown_in_progress() ) { - overlays.get_prediction_engine().set_local_frame_sent( net.get_sent_state_last() ); + if ( net.shutdown_in_progress() ) { + return true; + } + overlays.get_prediction_engine().set_local_frame_sent( net.get_sent_state_last() ); - /* Don't predict for bulk data. */ - bool paste = bytes_read > 100; - if ( paste ) { - overlays.get_prediction_engine().reset(); + /* Don't predict for bulk data. */ + bool paste = bytes_read > 100; + if ( paste ) { + overlays.get_prediction_engine().reset(); + } + + for ( int i = 0; i < bytes_read; i++ ) { + char the_byte = buf[ i ]; + + if ( !paste ) { + overlays.get_prediction_engine().new_user_byte( the_byte, local_framebuffer ); } - for ( int i = 0; i < bytes_read; i++ ) { - char the_byte = buf[ i ]; + if ( quit_sequence_started ) { + if ( the_byte == '.' ) { /* Quit sequence is Ctrl-^ . */ + if ( net.has_remote_addr() && (!net.shutdown_in_progress()) ) { + overlays.get_notification_engine().set_notification_string( wstring( L"Exiting on user request..." ), true ); + net.start_shutdown(); + return true; + } + return false; + } else if ( the_byte == 0x1a ) { /* Suspend sequence is escape_key Ctrl-Z */ + /* Restore terminal and terminal-driver state */ + swrite( STDOUT_FILENO, display.close().c_str() ); - if ( !paste ) { - overlays.get_prediction_engine().new_user_byte( the_byte, local_framebuffer ); - } - - if ( quit_sequence_started ) { - if ( the_byte == '.' ) { /* Quit sequence is Ctrl-^ . */ - if ( net.has_remote_addr() && (!net.shutdown_in_progress()) ) { - overlays.get_notification_engine().set_notification_string( wstring( L"Exiting on user request..." ), true ); - net.start_shutdown(); - return true; - } else { - return false; - } - } else if ( the_byte == 0x1a ) { /* Suspend sequence is escape_key Ctrl-Z */ - /* Restore terminal and terminal-driver state */ - swrite( STDOUT_FILENO, display.close().c_str() ); - - if ( tcsetattr( STDIN_FILENO, TCSANOW, &saved_termios ) < 0 ) { - perror( "tcsetattr" ); - exit( 1 ); - } - - fputs( "\n\033[37;44m[mosh is suspended.]\033[m\n", stdout ); - - fflush( NULL ); - - /* actually suspend */ - kill( 0, SIGSTOP ); - - resume(); - } else if ( (the_byte == escape_pass_key) || (the_byte == escape_pass_key2) ) { - /* Emulation sequence to type escape_key is escape_key + - escape_pass_key (that is escape key without Ctrl) */ - net.get_current_state().push_back( Parser::UserByte( escape_key ) ); - } else { - /* Escape key followed by anything other than . and ^ gets sent literally */ - net.get_current_state().push_back( Parser::UserByte( escape_key ) ); - net.get_current_state().push_back( Parser::UserByte( the_byte ) ); + if ( tcsetattr( STDIN_FILENO, TCSANOW, &saved_termios ) < 0 ) { + perror( "tcsetattr" ); + exit( 1 ); } - quit_sequence_started = false; + fputs( "\n\033[37;44m[mosh is suspended.]\033[m\n", stdout ); - if ( overlays.get_notification_engine().get_notification_string() == escape_key_help ) { - overlays.get_notification_engine().set_notification_string( L"" ); - } + fflush( NULL ); - continue; + /* actually suspend */ + kill( 0, SIGSTOP ); + + resume(); + } else if ( (the_byte == escape_pass_key) || (the_byte == escape_pass_key2) ) { + /* Emulation sequence to type escape_key is escape_key + + escape_pass_key (that is escape key without Ctrl) */ + net.get_current_state().push_back( Parser::UserByte( escape_key ) ); + } else { + /* Escape key followed by anything other than . and ^ gets sent literally */ + net.get_current_state().push_back( Parser::UserByte( escape_key ) ); + net.get_current_state().push_back( Parser::UserByte( the_byte ) ); } - quit_sequence_started = (escape_key > 0) && (the_byte == escape_key) && (lf_entered || (! escape_requires_lf)); - if ( quit_sequence_started ) { - lf_entered = false; - overlays.get_notification_engine().set_notification_string( escape_key_help, true, false ); - continue; + quit_sequence_started = false; + + if ( overlays.get_notification_engine().get_notification_string() == escape_key_help ) { + overlays.get_notification_engine().set_notification_string( L"" ); } - lf_entered = ( (the_byte == 0x0A) || (the_byte == 0x0D) ); /* LineFeed, Ctrl-J, '\n' or CarriageReturn, Ctrl-M, '\r' */ - - if ( the_byte == 0x0C ) { /* Ctrl-L */ - repaint_requested = true; - } - - net.get_current_state().push_back( Parser::UserByte( the_byte ) ); + continue; } + + quit_sequence_started = (escape_key > 0) && (the_byte == escape_key) && (lf_entered || (! escape_requires_lf)); + if ( quit_sequence_started ) { + lf_entered = false; + overlays.get_notification_engine().set_notification_string( escape_key_help, true, false ); + continue; + } + + lf_entered = ( (the_byte == 0x0A) || (the_byte == 0x0D) ); /* LineFeed, Ctrl-J, '\n' or CarriageReturn, Ctrl-M, '\r' */ + + if ( the_byte == 0x0C ) { /* Ctrl-L */ + repaint_requested = true; + } + + net.get_current_state().push_back( Parser::UserByte( the_byte ) ); } return true; diff --git a/src/frontend/terminaloverlay.cc b/src/frontend/terminaloverlay.cc index f864aeb..d47f936 100644 --- a/src/frontend/terminaloverlay.cc +++ b/src/frontend/terminaloverlay.cc @@ -90,32 +90,30 @@ Validity ConditionalOverlayCell::get_validity( const Framebuffer &fb, int row, const Cell ¤t = *( fb.get_cell( row, col ) ); /* see if it hasn't been updated yet */ - if ( late_ack >= expiration_frame ) { - if ( unknown ) { - return CorrectNoCredit; - } - - if ( replacement.is_blank() ) { /* too easy for this to trigger falsely */ - return CorrectNoCredit; - } - - if ( current.contents_match( replacement ) ) { - vector::const_iterator it = original_contents.begin(); - for ( ; it != original_contents.end(); it++ ) { - if ( it->contents_match( replacement ) ) - break; - } - if ( it == original_contents.end() ) { - return Correct; - } else { - return CorrectNoCredit; - } - } else { - return IncorrectOrExpired; - } + if ( late_ack < expiration_frame ) { + return Pending; } - return Pending; + if ( unknown ) { + return CorrectNoCredit; + } + + if ( replacement.is_blank() ) { /* too easy for this to trigger falsely */ + return CorrectNoCredit; + } + + if ( current.contents_match( replacement ) ) { + vector::const_iterator it = original_contents.begin(); + for ( ; it != original_contents.end(); it++ ) { + if ( it->contents_match( replacement ) ) + break; + } + if ( it == original_contents.end() ) { + return Correct; + } + return CorrectNoCredit; + } + return IncorrectOrExpired; } Validity ConditionalCursorMove::get_validity( const Framebuffer &fb, diff --git a/src/network/network.cc b/src/network/network.cc index b7cc79d..9dfcfdf 100644 --- a/src/network/network.cc +++ b/src/network/network.cc @@ -523,66 +523,65 @@ string Connection::recv_one( int sock_to_recv, bool nonblocking ) dos_assert( p.direction == (server ? TO_SERVER : TO_CLIENT) ); /* prevent malicious playback to sender */ - if ( p.seq >= expected_receiver_seq ) { /* don't use out-of-order packets for timestamp or targeting */ - expected_receiver_seq = p.seq + 1; /* this is security-sensitive because a replay attack could otherwise - screw up the timestamp and targeting */ + if ( p.seq < expected_receiver_seq ) { /* don't use (but do return) out-of-order packets for timestamp or targeting */ + return p.payload; + } + expected_receiver_seq = p.seq + 1; /* this is security-sensitive because a replay attack could otherwise + screw up the timestamp and targeting */ - if ( p.timestamp != uint16_t(-1) ) { - saved_timestamp = p.timestamp; - saved_timestamp_received_at = timestamp(); + if ( p.timestamp != uint16_t(-1) ) { + saved_timestamp = p.timestamp; + saved_timestamp_received_at = timestamp(); - if ( congestion_experienced ) { - /* signal counterparty to slow down */ - /* this will gradually slow the counterparty down to the minimum frame rate */ - saved_timestamp -= CONGESTION_TIMESTAMP_PENALTY; - if ( server ) { - fprintf( stderr, "Received explicit congestion notification.\n" ); - } - } - } - - if ( p.timestamp_reply != uint16_t(-1) ) { - uint16_t now = timestamp16(); - double R = timestamp_diff( now, p.timestamp_reply ); - - if ( R < 5000 ) { /* ignore large values, e.g. server was Ctrl-Zed */ - if ( !RTT_hit ) { /* first measurement */ - SRTT = R; - RTTVAR = R / 2; - RTT_hit = true; - } else { - const double alpha = 1.0 / 8.0; - const double beta = 1.0 / 4.0; - - RTTVAR = (1 - beta) * RTTVAR + ( beta * fabs( SRTT - R ) ); - SRTT = (1 - alpha) * SRTT + ( alpha * R ); - } - } - } - - /* auto-adjust to remote host */ - has_remote_addr = true; - last_heard = timestamp(); - - if ( server ) { /* only client can roam */ - if ( remote_addr_len != header.msg_namelen || - memcmp( &remote_addr, &packet_remote_addr, remote_addr_len ) != 0 ) { - remote_addr = packet_remote_addr; - remote_addr_len = header.msg_namelen; - char host[ NI_MAXHOST ], serv[ NI_MAXSERV ]; - int errcode = getnameinfo( &remote_addr.sa, remote_addr_len, - host, sizeof( host ), serv, sizeof( serv ), - NI_DGRAM | NI_NUMERICHOST | NI_NUMERICSERV ); - if ( errcode != 0 ) { - throw NetworkException( std::string( "recv_one: getnameinfo: " ) + gai_strerror( errcode ), 0 ); - } - fprintf( stderr, "Server now attached to client at %s:%s\n", - host, serv ); + if ( congestion_experienced ) { + /* signal counterparty to slow down */ + /* this will gradually slow the counterparty down to the minimum frame rate */ + saved_timestamp -= CONGESTION_TIMESTAMP_PENALTY; + if ( server ) { + fprintf( stderr, "Received explicit congestion notification.\n" ); } } } - return p.payload; /* we do return out-of-order or duplicated packets to caller */ + if ( p.timestamp_reply != uint16_t(-1) ) { + uint16_t now = timestamp16(); + double R = timestamp_diff( now, p.timestamp_reply ); + + if ( R < 5000 ) { /* ignore large values, e.g. server was Ctrl-Zed */ + if ( !RTT_hit ) { /* first measurement */ + SRTT = R; + RTTVAR = R / 2; + RTT_hit = true; + } else { + const double alpha = 1.0 / 8.0; + const double beta = 1.0 / 4.0; + + RTTVAR = (1 - beta) * RTTVAR + ( beta * fabs( SRTT - R ) ); + SRTT = (1 - alpha) * SRTT + ( alpha * R ); + } + } + } + + /* auto-adjust to remote host */ + has_remote_addr = true; + last_heard = timestamp(); + + if ( server && /* only client can roam */ + ( remote_addr_len != header.msg_namelen || + memcmp( &remote_addr, &packet_remote_addr, remote_addr_len ) != 0 ) ) { + remote_addr = packet_remote_addr; + remote_addr_len = header.msg_namelen; + char host[ NI_MAXHOST ], serv[ NI_MAXSERV ]; + int errcode = getnameinfo( &remote_addr.sa, remote_addr_len, + host, sizeof( host ), serv, sizeof( serv ), + NI_DGRAM | NI_NUMERICHOST | NI_NUMERICSERV ); + if ( errcode != 0 ) { + throw NetworkException( std::string( "recv_one: getnameinfo: " ) + gai_strerror( errcode ), 0 ); + } + fprintf( stderr, "Server now attached to client at %s:%s\n", + host, serv ); + } + return p.payload; } std::string Connection::port( void ) const diff --git a/src/terminal/terminaldisplay.cc b/src/terminal/terminaldisplay.cc index d7081b6..02b1ce9 100644 --- a/src/terminal/terminaldisplay.cc +++ b/src/terminal/terminaldisplay.cc @@ -181,29 +181,30 @@ std::string Display::new_frame( bool initialized, const Framebuffer &last, const for ( int row = 0; row < f.ds.get_height(); row++ ) { const Row *new_row = f.get_row( 0 ); const Row *old_row = &*rows.at( row ); - if ( new_row == old_row || *new_row == *old_row ) { - /* if row 0, we're looking at ourselves and probably didn't scroll */ - if ( row == 0 ) { - break; - } - /* found a scroll */ - lines_scrolled = row; - scroll_height = 1; - - /* how big is the region that was scrolled? */ - for ( int region_height = 1; - lines_scrolled + region_height < f.ds.get_height(); - region_height++ ) { - if ( *f.get_row( region_height ) - == *rows.at( lines_scrolled + region_height ) ) { - scroll_height = region_height + 1; - } else { - break; - } - } - + if ( ! ( new_row == old_row || *new_row == *old_row ) ) { + continue; + } + /* if row 0, we're looking at ourselves and probably didn't scroll */ + if ( row == 0 ) { break; } + /* found a scroll */ + lines_scrolled = row; + scroll_height = 1; + + /* how big is the region that was scrolled? */ + for ( int region_height = 1; + lines_scrolled + region_height < f.ds.get_height(); + region_height++ ) { + if ( *f.get_row( region_height ) + == *rows.at( lines_scrolled + region_height ) ) { + scroll_height = region_height + 1; + } else { + break; + } + } + + break; } if ( scroll_height ) { @@ -458,23 +459,23 @@ bool Display::put_row( bool initialized, FrameState &frame, const Framebuffer &f } } - if ( wrote_last_cell - && (frame_y < f.ds.get_height() - 1) ) { - /* To hint that a word-select should group the end of one line - with the beginning of the next, we let the real cursor - actually wrap around in cases where it wrapped around for us. */ - if ( wrap_this ) { - /* Update our cursor, and ask for wrap on the next row. */ - frame.cursor_x = 0; - frame.cursor_y++; - return true; - } else { - /* Resort to CR/LF and update our cursor. */ - frame.append( "\r\n" ); - frame.cursor_x = 0; - frame.cursor_y++; - } + if ( ! ( wrote_last_cell + && (frame_y < f.ds.get_height() - 1) ) ) { + return false; } + /* To hint that a word-select should group the end of one line + with the beginning of the next, we let the real cursor + actually wrap around in cases where it wrapped around for us. */ + if ( wrap_this ) { + /* Update our cursor, and ask for wrap on the next row. */ + frame.cursor_x = 0; + frame.cursor_y++; + return true; + } + /* Resort to CR/LF and update our cursor. */ + frame.append( "\r\n" ); + frame.cursor_x = 0; + frame.cursor_y++; return false; }