Issue127 #128

kimmerin · 2022-02-17T18:29:33Z

As "requested" in the issue, these are the changes I've made to be able to set an instance-wide Logger. The static API is kept to avoid breaking issues "out there".

instance-specific logging. The static methods are kept untouched to keep backward compatibility

norrisjeremy

Here are my initial thoughts/comments.

norrisjeremy · 2022-02-17T19:00:18Z

src/main/java/com/jcraft/jsch/Compression.java

@@ -32,7 +32,7 @@
 public interface Compression{
  static public final int INFLATER=0;
  static public final int DEFLATER=1;
-  void init(int type, int level) throws Exception;
+  void init(Session session, int type, int level) throws Exception;


In #126 I've attempted to restore the backwards incompatibilities I introduced in several public interfaces.

Could we change revert this method signature back, and then instead add something like this?

default void init(int type, int level, Session session) throws Exception {init(type, level);} default Logger getLogger() {return JSch.getLogger();}

That way this interface maintains backwards compatibility.
We can thin change the built-in implementations to override these methods as appropriate, but any users that are using custom Compression implementations don't suddenly have their implementations break?

I've reverted the change but solved it a bit differently to keep code duplication in juz.Compression and jzlib.Compression to a minimum. There is now a default setSession-method doing nothing and a static logMessage in Session with a Supplier that is now called in both Compression-implementations.

norrisjeremy · 2022-02-17T19:00:56Z

src/main/java/com/jcraft/jsch/JSch.java

@@ -38,7 +38,7 @@
  /**
   * The version number.
   */
-  public static final String VERSION  = Version.getVersion();
+  public static final String VERSION  = "0.2.0"; //Version.getVersion();


This part needs to be reverted.

That part wasn't intended to be in the commit

norrisjeremy · 2022-02-17T19:04:36Z

src/main/java/com/jcraft/jsch/KeyPair.java

@@ -175,6 +175,15 @@ public void writePrivateKey(OutputStream out, byte[] passphrase){

  abstract byte[] getKeyTypeName();
  public abstract int getKeyType();
+
+  public String getKeyTypeName(String encoding) {


This part seems unrelated to the logging changes?

It is. Shouldn't be part of the commit, either.

norrisjeremy · 2022-02-17T19:05:23Z

src/main/java/com/jcraft/jsch/PortWatcher.java

          channel.init();
+          channel.setSession(session);


Why do we have to call setSession() twice here (both before and after init()?

Shouldn't be. I'll remove the second one (I suppose while going over the changes I've missed the first setSession I've already added and added the second one).

norrisjeremy · 2022-02-17T19:08:28Z

src/main/java/com/jcraft/jsch/Session.java

@@ -73,7 +73,7 @@
  private static final int PACKET_MAX_SIZE = 256 * 1024;

  private byte[] V_S;                                 // server version
-  private byte[] V_C=Util.str2byte("SSH-2.0-JSCH_"+Version.getVersion()); // client version
+  private byte[] V_C=Util.str2byte("SSH-2.0-JSCH_"+JSch.VERSION); // client version


This part needs to be reverted I think.

It's unrelated to the logging issue but worth thinking about the reason why the "logic" to get the implementation's version needs to be done twice.

This probably could remain. Calling Version.getVersion() here originated because earlier I mistakenly removed JSch.VERSION, but then re-added it to restore backwards compatibility in #98.

norrisjeremy · 2022-02-17T19:09:21Z

src/main/java/com/jcraft/jsch/Session.java

+                + " for methods '" + smethods + "'");
+//        if(auth_cancel)
+//          throw new JSchException("Auth cancel");
+//        throw new JSchException("Auth fail");


I think we should just remove these commented out lines.

This one is logging unrelated and is one of the changes I've done to the JSch sources. Seems that I've copied that to this file together with the logging related changes. If it's OK to keep the "for methods"-part in the exception I can remove the commented out code, otherwise I can revert it to the less informative original message

Yes, I think keeping the "for methods" part in the exception is fine. We should just remove the old commented out code here.

norrisjeremy · 2022-02-17T19:11:30Z

src/main/java/com/jcraft/jsch/KeyExchange.java

@@ -69,6 +69,11 @@

  public abstract void init(Session session, 
                            byte[] V_S, byte[] V_C, byte[] I_S, byte[] I_C) throws Exception;
+  public void doInit(Session session, 


I don't think we should make this public. I'd change it to package private.

I've changed it. I've copied init's method signature to make sure that the parameters are identical.

"for methods" information

norrisjeremy · 2022-02-17T20:01:01Z

src/main/java/com/jcraft/jsch/Compression.java

@@ -32,7 +32,10 @@
 public interface Compression{
  static public final int INFLATER=0;
  static public final int DEFLATER=1;
-  void init(Session session, int type, int level) throws Exception;
+  default void setSession(Session session) {


I'd rather go with the approach of adding a default void init(int type, int level, Session session) {init(type, level);} approach, and let actual implementations of Compression handle overriding it.

We would then change the Session class to call the new init(type, level, session) method.
Any existing implementations that users have developed that don't specialize this default implementation continue to work.
And since the implementations that specialize the new void init(int type, int level, Session session) method would then internally gain access to the Session instance and could just call the session.getLogger() method for their internal logging needs.
That would also allow us to avoid having to add the unwieldy static logMessage() method to the Session class.

Is it no valid use case that the Compression classes are used independently, i.e. in an application that need ZLIB compression for something else? In that case your suggestion would break this application because there is no session instance and the logging attempt would lead to a NullPointerException. That's why I've created a logMessage-method that checks for the existence of the session and falls back to the static logger if it's not there.

Users can create their own Compression implementations and then instruct JSch to use utilize them by calling JSch.setConfig("zlib@openssh.com", "my.own.CompressionImpl") or session.setConfig("zlib@openssh.com", "my.own.CompressionImpl").

For the built-in implementations we provide (com.jcraft.jsch.jzlib.Compression & com.jcraft.jsch.juz.Compression), there wouldn't be an NPE thrown because Session would call the new init(type, level, this); (thus never calling init(type, level)).

That's not the use case I've mentioned. If someone is using zlib.Compression "directly" in their application they will use init(type, level) as it's the way it had to be done. With your suggested change that application stops working due to a NullPointerException. I wasn't talking about using own implementations of Compression or the JSch-internal use of it.

The built-in implementations of com.jcraft.jsch.jzlib.Compression & com.jcraft.jsch.juz.Compression aren't intended to be consumed outside of JSch, so I'm not especially worried if somebody got an NPE in that case, because I don't consider them part of the public API.
In fact, in the module-info.java I constructed, these packages (com.jcraft.jsch.jzlib & com.jcraft.jsch.juz) aren't even exported, to further emphasize that point.

I'm far more interested in allow users to continue to instruct JSch to use their own custom Compression implementations without having to adapt them after this change.

We can also introduce null guards in the com.jcraft.jsch.jzlib.Compression & com.jcraft.jsch.juz.Compression for when session is dereferenced to avoid throwing an NPE, for anybody that does happening to utilize them whilst only calling init(type, level).

I've done the changes you've suggested including checking for null and fall back to the jsch logger if no particular session logger is specified.

norrisjeremy · 2022-02-18T05:15:26Z

src/main/java/com/jcraft/jsch/UserInfo.java

@@ -36,4 +36,5 @@
  boolean promptPassphrase(String message);
  boolean promptYesNo(String message);
  void showMessage(String message);
+


I think this stray newline should be removed?

norrisjeremy · 2022-02-18T05:17:42Z

src/main/java/com/jcraft/jsch/jzlib/Compression.java


-  public Compression(){
+  public Compression() {


Can we remove the added space character before the opening brace? It helps to reduce diffs when comparing this implementation with the com.jcraft.jsch.juz version.

norrisjeremy · 2022-02-18T05:22:39Z

src/main/java/com/jcraft/jsch/Channel.java

    if(type.equals("session")){
-      return new ChannelSession();
+      ret = new ChannelSession();


I wonder if we should perhaps we just add a Session session argument to each of the constructors of the ChannelXXX implementations instead of adding an explicit setSession(session) call below?
That would then allow us to remove the addChannel() method in Session & PortWatcher classes I think?
@mwiede any thoughts?

I've solved it this way to keep the number of affected classes as low as possible. As far as I can see, the solution with setChannel() (that is implemented in the abstract class Channel) would allow you to do the same, or do I miss something here?

norrisjeremy · 2022-02-18T05:28:58Z

src/main/java/com/jcraft/jsch/Session.java

@@ -202,8 +204,8 @@ public void connect(int connectTimeout) throws JSchException{
    }
    Packet.setRandom(random);

-    if(JSch.getLogger().isEnabled(Logger.INFO)){
-      JSch.getLogger().log(Logger.INFO,
+    if(jsch.getInstanceLogger().isEnabled(Logger.INFO)){


Would it make more sense to just call the getLogger() method you've added to this class?

I've changed it accordingly. It was required in the light of per-session logging, anyway.

norrisjeremy · 2022-02-18T05:39:32Z

I am wondering: should we structure this such that users have the ability to set a different logger per Session created?
I could imagine a case in which a user opens multiple Session's, all to different hosts, all from the same underlying JSch instance?
I.e.

JSch jsch = new JSch();
Session sess1 = jsch.getSession(username, host1, port);
Session sess2 = jsch.getSession(username, host2, port);
...

Some users may desire the ability to have different loggers for sess1 & sess2 above?

by setLogger

kimmerin · 2022-02-18T07:27:35Z

I am wondering: should we structure this such that users have the ability to set a different logger per Session created?

I've changed the Session to have its own logger instance that get instantiated by JSch's instance logger and allow to change it by providing a corresponding setter method. That means that you can't change the logger on the JSch-level and have that affect existing sessions but I'm not sure if that should be considered an issue.

BTW: Session contains an "active" System.err.println ("fail in inflater"). Should that be commented out or changed to a logging message?

Edit: Changed the way in Session so that the JSch-loggers are used if no particular logger has been set for Session.

specific logger has been set by setLogger.

norrisjeremy · 2022-02-18T17:20:02Z

Hi @kimmerin,

I'll try to review again later this evening or over the weekend.

Thanks!
Jeremy

kimmerin · 2022-02-21T13:46:34Z

I'll try to review again later this evening or over the weekend.

No problem, take your time.

norrisjeremy · 2022-02-22T11:42:05Z

src/main/java/com/jcraft/jsch/juz/Compression.java


  public Compression(){
  }

+  private void logMessage(int level, String message) {
+    Logger logger = session == null ? JSch.getLogger() : session.getLogger();
+    logger.log(level, message);


I think we should add a check to make sure logger.isEnabled(level) here before actually logging the message?

This would also apply to the jzlib implementation of Compression as well.

That wouldn't make much sense, because the common reason to do this is to avoid concatenating a log-message that isn't used later on if level > currentLevel. In logMessage that concatenation has already taken place and logger.logMessage is supposed to do this check itself, so doing this check before doesn't save anything.

If a user has a custom Logger that has a specific logging level disabled, we shouldn't emit a log message at said level.

I've added a check and used a Supplier, so there is no need for all these if(enabled)-logMessage-cascades.

norrisjeremy · 2022-02-22T15:07:33Z

I think this looks good now: can we squash all the commits so that it's one single atomic changeset?

on the instance level. allow the setting of a logger to the JSch instance to allow instance-specific logging. The static methods are kept untouched to keep backward compatibility reenabled message changes according to the feedback in the pull request removed commented out code containing the original exception without "for methods" information removed method unrelated to this issue added missing bracket changed logging in compression classes to use own methods removed whitespaces get logger from jsch on instantiation and allow to set a different one by setLogger changed logger setting to fall back to JSch-logger retrieval if no specific logger has been set by setLogger. added test for logger handling added javadoc check if logging is necessary before calling

…e127

kimmerin · 2022-02-22T18:52:11Z

can we squash all the commits so that it's one single atomic changeset?

I've tried but have to admit that my knowledge of git isn't very much and following a description I've found online ended with my client trying to push to master and not the branch which is not what should happen, I suppose.

Is there a description somewhere how to do that (ideally with Eclipse and not the git command shell)?

kimmerin · 2022-02-22T19:42:52Z

Hm, it looks like that commit 55e3ef3 is the squashed commit that you wanted but then Eclipse did some merge afterwards. Can you use 55e3ef3 for your merge?

kimmerin · 2022-03-03T14:36:08Z

@norrisjeremy Shall I create a new branch, put all the changes in there, commit them and create a new pull request? That would have the same effect.

mwiede · 2022-03-06T13:17:02Z

@kimmerin you do not need to create a new branch, I can select "squash and merge" which aggregates all commits to one.

kimmerin · 2022-03-06T14:55:59Z

@mwiede I shouldn't write tests ;-) Is the Slf4jLogger set by the test framework during build or where is it coming from?

norrisjeremy · 2022-03-06T17:09:35Z

@mwiede I shouldn't write tests ;-) Is the Slf4jLogger set by the test framework during build or where is it coming from?

I wonder if you need to add a @BeforeEach in SessionTest to explicitly reset the JSch static logger instance like in JSchTest?

state

kimmerin · 2022-03-06T18:39:58Z

I wonder if you need to add a @BeforeEach in SessionTest to explicitly reset the JSch static logger instance like in JSchTest?

Except in my test method there is no call of setLogger in this test, so the Slf4JLogger must have been set from the outside. I'm now setting the static logger to null at the beginning of the test.

finally

kimmerin and others added 2 commits February 17, 2022 19:16

allow the setting of a logger to the JSch instance to allow

f7f7be8

instance-specific logging. The static methods are kept untouched to keep backward compatibility

reenabled message

fee4dd7

norrisjeremy suggested changes Feb 17, 2022

View reviewed changes

kimmerin added 4 commits February 17, 2022 20:53

changes according to the feedback in the pull request

f8220b3

removed commented out code containing the original exception without

f71753e

"for methods" information

removed method unrelated to this issue

7c2f3e1

added missing bracket

ffe145d

norrisjeremy suggested changes Feb 17, 2022

View reviewed changes

changed logging in compression classes to use own methods

c156b66

norrisjeremy suggested changes Feb 18, 2022

View reviewed changes

kimmerin added 2 commits February 18, 2022 08:00

removed whitespaces

8de3843

get logger from jsch on instantiation and allow to set a different one

6431bcf

by setLogger

kimmerin added 3 commits February 18, 2022 11:43

changed logger setting to fall back to JSch-logger retrieval if no

48a6ff2

specific logger has been set by setLogger.

added test for logger handling

1cc013c

added javadoc

af03841

norrisjeremy suggested changes Feb 22, 2022

View reviewed changes

check if logging is necessary before calling

b127caf

kimmerin added 2 commits February 22, 2022 16:27

Merge branch 'issue127' of https://github.com/kimmerin/jsch into issu…

636d0d1

…e127

set JSch static logger to null to make sure that we start at a known

77a878e

state

Keep the logger we find at the beginning of the test and reset it in

c7d1466

finally

mwiede merged commit 21156d4 into mwiede:master Mar 8, 2022

Issue127 #128

Issue127 #128

Conversation

kimmerin commented Feb 17, 2022

norrisjeremy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

norrisjeremy Feb 17, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

norrisjeremy Feb 17, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

norrisjeremy Feb 18, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kimmerin Feb 18, 2022 • edited Loading

Choose a reason for hiding this comment

norrisjeremy commented Feb 18, 2022

kimmerin commented Feb 18, 2022 • edited Loading

norrisjeremy commented Feb 18, 2022

kimmerin commented Feb 21, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

norrisjeremy commented Feb 22, 2022

kimmerin commented Feb 22, 2022

kimmerin commented Feb 22, 2022

kimmerin commented Mar 3, 2022

mwiede commented Mar 6, 2022

kimmerin commented Mar 6, 2022

norrisjeremy commented Mar 6, 2022

kimmerin commented Mar 6, 2022

norrisjeremy Feb 17, 2022 •

edited

Loading

norrisjeremy Feb 17, 2022 •

edited

Loading

norrisjeremy Feb 18, 2022 •

edited

Loading

kimmerin Feb 18, 2022 •

edited

Loading

kimmerin commented Feb 18, 2022 •

edited

Loading