[I] SessionHelper#signalNegotiationEnd should pass reason through to listener [mina-sshd]

[via GitHub]

duco-lw opened a new issue, #504:
URL: https://github.com/apache/mina-sshd/issues/504

   
https://github.com/apache/mina-sshd/blob/71b842f759f9879d7638bed175e5be006d9c0f46/sshd-core/src/main/java/org/apache/sshd/common/session/helpers/SessionHelper.java#L1149
   
   The `reason` is not passed through to the listener:
   ```
       protected void signalNegotiationEnd(
               SessionListener listener,
               Map<KexProposalOption, String> c2sOptions, 
Map<KexProposalOption, String> s2cOptions,
               Map<KexProposalOption, String> negotiatedGuess, Throwable 
reason) {
           if (listener == null) {
               return;
           }
   
           listener.sessionNegotiationEnd(this, c2sOptions, s2cOptions, 
negotiatedGuess, null);
       }
   ```
   The doc for this says the negotiation is successful if this is null:
   ```
       /**
        * Signals the end of the negotiation options handling
        *
        * @param session           The referenced {@link Session}
        * @param clientProposal    The client proposal options (un-modifiable)
        * @param serverProposal    The server proposal options (un-modifiable)
        * @param negotiatedOptions The successfully negotiated options so far - 
even if exception occurred (un-modifiable)
        * @param reason            Negotiation end reason - {@code null} if 
successful
        */
       default void sessionNegotiationEnd(
               Session session,
               Map<KexProposalOption, String> clientProposal,
               Map<KexProposalOption, String> serverProposal,
               Map<KexProposalOption, String> negotiatedOptions,
               Throwable reason) {
           // ignored
       }
   ```
   
https://github.com/apache/mina-sshd/blob/71b842f759f9879d7638bed175e5be006d9c0f46/sshd-core/src/main/java/org/apache/sshd/common/session/SessionListener.java#L151
   
   This looks like this will always be "successful", even in the error case?
   ```
           } catch (IOException | RuntimeException | Error e) {
               signalNegotiationEnd(c2sOptions, s2cOptions, negotiatedGuess, e);
               throw e;
           }
   ```
   
https://github.com/apache/mina-sshd/blob/71b842f759f9879d7638bed175e5be006d9c0f46/sshd-core/src/main/java/org/apache/sshd/common/session/helpers/AbstractSession.java#L2259
   
   Would changing the line from
   ```
           listener.sessionNegotiationEnd(this, c2sOptions, s2cOptions, 
negotiatedGuess, null);
   ```
   to
   ```
           listener.sessionNegotiationEnd(this, c2sOptions, s2cOptions, 
negotiatedGuess, reason);
   ```
   break anything?


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscr...@mina.apache.org.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@mina.apache.org
For additional commands, e-mail: dev-h...@mina.apache.org