Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Issue127 #128

Merged
merged 17 commits into from
Mar 8, 2022
Merged

Issue127 #128

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
f7f7be8
allow the setting of a logger to the JSch instance to allow
kimmerin Feb 17, 2022
fee4dd7
reenabled message
Feb 17, 2022
f8220b3
changes according to the feedback in the pull request
Feb 17, 2022
f71753e
removed commented out code containing the original exception without
Feb 17, 2022
7c2f3e1
removed method unrelated to this issue
Feb 17, 2022
ffe145d
added missing bracket
Feb 17, 2022
c156b66
changed logging in compression classes to use own methods
Feb 17, 2022
8de3843
removed whitespaces
Feb 18, 2022
6431bcf
get logger from jsch on instantiation and allow to set a different one
Feb 18, 2022
48a6ff2
changed logger setting to fall back to JSch-logger retrieval if no
Feb 18, 2022
1cc013c
added test for logger handling
Feb 18, 2022
af03841
added javadoc
Feb 18, 2022
b127caf
check if logging is necessary before calling
Feb 22, 2022
55e3ef3
Implementation of Issue 127: Allow the setting of a logger
kimmerin Feb 17, 2022
636d0d1
Merge branch 'issue127' of https://github.com/kimmerin/jsch into issu…
kimmerin Feb 22, 2022
77a878e
set JSch static logger to null to make sure that we start at a known
kimmerin Mar 6, 2022
c7d1466
Keep the logger we find at the beginning of the test and reset it in
kimmerin Mar 6, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion src/main/java/com/jcraft/jsch/Compression.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,10 @@ LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING
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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@norrisjeremy norrisjeremy Feb 17, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@norrisjeremy norrisjeremy Feb 17, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// nothing
}
void init(int type, int level) throws Exception;
byte[] compress(byte[] buf, int start, int[] len);
byte[] uncompress(byte[] buf, int start, int[] len);
}
2 changes: 1 addition & 1 deletion src/main/java/com/jcraft/jsch/JSch.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ public class JSch{
/**
* The version number.
*/
public static final String VERSION = "0.2.0"; //Version.getVersion();
public static final String VERSION = Version.getVersion();

static Hashtable<String, String> config=new Hashtable<>();
static{
Expand Down
2 changes: 1 addition & 1 deletion src/main/java/com/jcraft/jsch/KeyExchange.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ public abstract class KeyExchange{

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,
void doInit(Session session,
byte[] V_S, byte[] V_C, byte[] I_S, byte[] I_C) throws Exception {
this.session = session;
init(session, V_S, V_C, I_S, I_C);
Expand Down
1 change: 0 additions & 1 deletion src/main/java/com/jcraft/jsch/PortWatcher.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -214,7 +214,6 @@ public void run(){
ChannelDirectTCPIP channel = new ChannelDirectTCPIP();
channel.setSession(session);
channel.init();
channel.setSession(session);
channel.setInputStream(in);
channel.setOutputStream(out);
session.addChannel(channel);
Expand Down
19 changes: 17 additions & 2 deletions src/main/java/com/jcraft/jsch/Session.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,8 @@ LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING
import java.io.OutputStream;
import java.net.Socket;
import java.util.*;
import java.util.function.Supplier;

import javax.crypto.AEADBadTagException;

public class Session implements Runnable{
Expand Down Expand Up @@ -2573,7 +2575,8 @@ private void initDeflater(String method) throws JSchException{
int level=6;
try{ level=Integer.parseInt(getConfig("compression_level"));}
catch(Exception ee){ }
deflater.init(this, Compression.DEFLATER, level);
deflater.setSession(this);
deflater.init(Compression.DEFLATER, level);
}
catch(Exception ee){
throw new JSchException(ee.toString(), ee);
Expand All @@ -2594,7 +2597,8 @@ private void initInflater(String method) throws JSchException{
try{
Class<? extends Compression> c=Class.forName(foo).asSubclass(Compression.class);
inflater=c.getDeclaredConstructor().newInstance();
inflater.init(this, Compression.INFLATER, 0);
inflater.setSession(this);
inflater.init(Compression.INFLATER, 0);
}
catch(Exception ee){
throw new JSchException(ee.toString(), ee);
Expand Down Expand Up @@ -3246,6 +3250,17 @@ private void checkConfig(ConfigRepository.Config config, String key){
this.setConfig(key, value);
}

public static void logMessage(Session session, int level, Supplier<String> message) {
Logger logger = JSch.getLogger();
if (session != null) {
logger = session.getLogger();
}
if (!logger.isEnabled(level)) {
return;
}
logger.log(level, message.get());
}

public Logger getLogger() {
return jsch.getInstanceLogger();
}
Expand Down
13 changes: 3 additions & 10 deletions src/main/java/com/jcraft/jsch/juz/Compression.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,19 +35,15 @@ public Compression(){
}

@Override
public void init(Session session, int type, int level) throws Exception{
this.session = session;
public void init(int type, int level) throws Exception{
if(type==DEFLATER){
deflater=new Deflater(level);
}
else if(type==INFLATER){
inflater=new Inflater();
inflated_buf=new byte[BUF_SIZE];
}
if(session.getLogger().isEnabled(Logger.DEBUG)){
session.getLogger().log(Logger.DEBUG,
"zlib using "+this.getClass().getCanonicalName());
}
Session.logMessage(session, Logger.DEBUG, () -> "zlib using "+this.getClass().getCanonicalName());
}

@Override
Expand Down Expand Up @@ -101,10 +97,7 @@ public byte[] uncompress(byte[] buf, int start, int[] len){
while(inflater.getRemaining()>0);
}
catch(java.util.zip.DataFormatException e){
if(session.getLogger().isEnabled(Logger.WARN)){
session.getLogger().log(Logger.WARN,
"an exception during uncompress\n"+e.toString());
}
Session.logMessage(session, Logger.WARN, () -> "an exception during uncompress\n"+e.toString());
}

if(buf.length<inflated_buf.length+start){
Expand Down
28 changes: 9 additions & 19 deletions src/main/java/com/jcraft/jsch/jzlib/Compression.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@ LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING
*/

package com.jcraft.jsch.jzlib;
import java.util.function.Supplier;

import com.jcraft.jsch.*;

public class Compression implements com.jcraft.jsch.Compression {
Expand All @@ -39,31 +41,26 @@ public class Compression implements com.jcraft.jsch.Compression {
private byte[] inflated_buf;
private Session session;

public Compression(){
public Compression() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

}

@Override
public void init(Session session, int type, int level) throws Exception{
this.session = session;
public void init(int type, int level) throws Exception{
if(type==DEFLATER){
deflater=new Deflater(level);
}
else if(type==INFLATER){
inflater=new Inflater();
inflated_buf=new byte[BUF_SIZE];
}
if(session.getLogger().isEnabled(Logger.DEBUG)){
session.getLogger().log(Logger.DEBUG,
"zlib using "+this.getClass().getCanonicalName());
}
Session.logMessage(session, Logger.DEBUG, () -> "zlib using "+this.getClass().getCanonicalName());
}

@Override
public byte[] compress(byte[] buf, int start, int[] len){
deflater.next_in=buf;
deflater.next_in_index=start;
deflater.avail_in=len[0]-start;
int status;
int outputlen=start;
byte[] outputbuf=buf;
int tmp=0;
Expand All @@ -72,7 +69,7 @@ public byte[] compress(byte[] buf, int start, int[] len){
deflater.next_out=tmpbuf;
deflater.next_out_index=0;
deflater.avail_out=BUF_SIZE;
status=deflater.deflate(JZlib.Z_PARTIAL_FLUSH);
int status=deflater.deflate(JZlib.Z_PARTIAL_FLUSH);
switch(status){
case JZlib.Z_OK:
tmp=BUF_SIZE-deflater.avail_out;
Expand All @@ -85,11 +82,7 @@ public byte[] compress(byte[] buf, int start, int[] len){
outputlen+=tmp;
break;
default:
if(session.getLogger().isEnabled(Logger.WARN)){
session.getLogger().log(Logger.WARN,
"compress: deflate returnd "+status);
}

Session.logMessage(session, Logger.WARN, () -> "compress: deflate returnd "+status);
}
}
while(deflater.avail_out==0);
Expand Down Expand Up @@ -140,11 +133,8 @@ public byte[] uncompress(byte[] buffer, int start, int[] length){
length[0]=inflated_end;
return buffer;
default:
if(session.getLogger().isEnabled(Logger.WARN)){
session.getLogger().log(Logger.WARN,
"uncompress: inflate returnd "+status);
}
return null;
Session.logMessage(session, Logger.WARN, () -> "compress: deflate returnd "+status);
return null;
}
}
}
Expand Down