tomaswolf commented on code in PR #314:
URL: https://github.com/apache/mina-sshd/pull/314#discussion_r1085815331
##########
sshd-sftp/src/main/java/org/apache/sshd/sftp/server/AbstractSftpSubsystemHelper.java:
##########
@@ -1250,6 +1250,9 @@ protected void doRename(int id, String oldPath, String
newPath, Collection<CopyO
SftpFileSystemAccessor accessor = getFileSystemAccessor();
accessor.renameFile(this, o, n, opts);
} catch (IOException | RuntimeException | Error e) {
+ if (log.isDebugEnabled()) {
+ log.debug("doRename({})[id={}] Exception", getServerSession(),
id, e);
+ }
Review Comment:
Hmmm. Log-then-throw is in most cases an anti-pattern. It seems that it
would not be necessary to log here: just throw and let it be handled in the
sendStatus() call in doRename(Buffer, int) or doPosixRename(Buffer, int).
Except that here Error is caught, too (another anti-pattern), and then
re-thrown. I would expect there to be some exception quite high up in the call
hierarchy logging an Error, though. So in summary I don't think this extra
logging here is necessary.
##########
sshd-sftp/src/main/java/org/apache/sshd/sftp/server/SftpSubsystem.java:
##########
@@ -893,6 +893,9 @@ protected void doClose(int id, String handle) throws
IOException {
nodeHandle.close();
listener.closed(session, handle, nodeHandle, null);
} catch (IOException | RuntimeException | Error e) {
+ if (log.isTraceEnabled()) {
+ log.debug("doClose({})[id={}] Exception", session, id, e);
+ }
Review Comment:
This is a similar case as the doRename() over in
AbstractSftpSubsystemHelper. Log-then-throw should not be necessary;
IOException and RuntimeException will be logged in the sendStatus() call in
doClose(Buffer, int), and for Error there should be last-resort handler
somewhere higher up that logs it.
Besides, it should be log.isDebugEnabled(), if at all.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
