[NET-727] Enable accessing options map for TFTP request packet and allow using `blksize` option by vimukthiD · Pull Request #331 · apache/commons-net

vimukthiD · 2025-02-26T08:16:31Z

relates to existing https://issues.apache.org/jira/browse/NET-727

This changes includes the mapping of options map in TFTPRequestPacket (according to RFC-2347) and allowing to use blksize (according to the RFC-2348) when doing a TFTP buffered send/receive.

garydgregory

Hello @vimukthiD
Thank you for your PR!
Please rebase on git master to pick up the latest and address the comments I've scattered throughout.
TY!

garydgregory · 2025-02-26T12:35:03Z

src/main/java/org/apache/commons/net/tftp/TFTP.java

+    private int packetSize = TFTPPacket.SEGMENT_SIZE + 4;
+
+    /** Internal state to track if the send/receive buffers are initialized */
+    private boolean buffersInityialized = false;


Fix spelling of buffersInityialized.

src/main/java/org/apache/commons/net/tftp/TFTP.java

garydgregory · 2025-02-26T12:36:33Z

src/main/java/org/apache/commons/net/tftp/TFTP.java

+    public final void resetBuffersToSize(int packetSize) {
+        // the packet size should be between 8 - 65464 (inclusively) then we add 4 for the header
+        this.packetSize = (Math.min(Math.max(packetSize, 8), 65464) + 4);
+


Remove extra vertical whitespace.

garydgregory · 2025-02-26T12:36:50Z

src/main/java/org/apache/commons/net/tftp/TFTP.java

+    }
+
+    /**
+     * Returns the buffer size of the buffered used by {@link #bufferedSend bufferedSend() } and {@link #bufferedReceive bufferedReceive() }.


Javadoc: A getter "Gets..."

garydgregory · 2025-02-26T12:40:23Z

src/main/java/org/apache/commons/net/tftp/TFTPErrorPacket.java

    /** The no such user error code according to RFC 783, value 7. */
    public static final int NO_SUCH_USER = 7;

+    /** The invalid options error code according to RFC 2347, value 8. */


New public and protected elements should have a Javadoc @SInCE tag. In this case, for 3.12.0.

src/main/java/org/apache/commons/net/tftp/TFTPOptionsAckPacket.java

garydgregory · 2025-02-26T13:55:49Z

src/main/java/org/apache/commons/net/tftp/TFTPOptionsAckPacket.java

+            byte[] key = entry.getKey().getBytes(StandardCharsets.US_ASCII);
+            System.arraycopy(key, 0, data, offset, key.length);
+            offset += key.length;
+


No need for blank lines, if there is something to call out, then write an inline // comment.

garydgregory · 2025-02-26T13:56:56Z

src/main/java/org/apache/commons/net/tftp/TFTPPacket.java

    public static final int ERROR = 5;

+    /**
+     * This is the actual TFTP spec identifier and is equal to 6.


No need to "This is", just say what it is 'TFTP spec identifier {@value}.'

garydgregory · 2025-02-26T14:09:03Z

src/main/java/org/apache/commons/net/tftp/TFTPRequestPacket.java

    }

+    /**
+     * Returns the options extensions of the request as a map.


A getter "Gets...".

vimukthiD · 2025-02-27T13:42:03Z

Hello @garydgregory ,
thank you very much for checking the changes and sorry about the silly mistakes.
I've done the changes and updated the pull request.
Please let me know if I've missed anything.
Thank you !

garydgregory

@vimukthiD

Thank you for updating your pull-request. You'll more comments to address in this round.

To verify your code coverage, run:

mvn clean site -Dcommons.jacoco.haltOnFailure=false -Pjacoco

and inspect the page:

target/site/jacoco/org.apache.commons.net.tftp/index.html#dn-b

Try to get the new code the highest possible coverage if you can.

TY!

garydgregory · 2025-02-27T15:29:59Z

src/main/java/org/apache/commons/net/tftp/TFTP.java

    private DatagramPacket sendDatagram;

+    /** Internal packet size, which is the data octet size plus 4 (for TFTP header octets) */
+    private int packetSize = TFTPPacket.SEGMENT_SIZE + 4;


Don't duplicate the expression that defines PACKET_SIZE, reuse that constant.

garydgregory · 2025-02-27T15:31:18Z

src/main/java/org/apache/commons/net/tftp/TFTPErrorPacket.java

+    /**
+     * The invalid options error code according to RFC 2347, value 8.
+     * @since 3.12.0
+     * */


Fix formatting.

garydgregory · 2025-02-27T15:31:59Z

src/main/java/org/apache/commons/net/tftp/TFTPOptionsAckPacket.java

+        for (Map.Entry<String, String> entry : options.entrySet()) {
+            optionsLength += entry.getKey().length() + 1 + entry.getValue().length() + 1;
+        }
+


Remove extra vertical whitespace.

garydgregory · 2025-02-27T15:32:03Z

src/main/java/org/apache/commons/net/tftp/TFTPOptionsAckPacket.java

+        int offset = 0;
+        data[offset++] = 0;
+        data[offset++] = (byte) type;
+


Remove extra vertical whitespace.

garydgregory · 2025-02-27T15:32:27Z

src/main/java/org/apache/commons/net/tftp/TFTPOptionsAckPacket.java

+    }
+
+    private int writeOptionsData(byte[] data, int offset) {
+        // all the options would get written to the data byte array in following format


Put this inline comment in a Javadoc comment using <pre> tags, keep it simple, you don't need "~~"s...

garydgregory · 2025-02-27T15:51:23Z

src/main/java/org/apache/commons/net/tftp/TFTP.java

+    }
+
+    /**
+     * Gets the buffer size of the buffered used by {@link #bufferedSend bufferedSend() } and {@link #bufferedReceive bufferedReceive() }.


Simplify clutter: {@link #bufferedSend bufferedSend() } -> {@link #bufferedSend(TFTPPacket)} and so on.

garydgregory · 2025-02-27T15:54:17Z

src/main/java/org/apache/commons/net/tftp/TFTP.java

+     * @return current buffer size
+     * @since 3.12.0
+     */
+    public int getBufferSize() {


This method is not tested or called, remove it or add a unit test for it.

Rename to match the instance variable.

garydgregory · 2025-02-27T15:58:19Z

src/main/java/org/apache/commons/net/tftp/TFTPOptionsAckPacket.java

+import java.util.HashMap;
+import java.util.Map;
+
+/**


This class is not tested. Remove it or create a unit test.

garydgregory · 2025-02-27T16:00:54Z

src/main/java/org/apache/commons/net/tftp/TFTPRequestPacket.java

+        for (Map.Entry<String, String> entry : options.entrySet()) {
+            optionsLength += entry.getKey().length() + 1 + entry.getValue().length() + 1;
+        }
+


Remove extra vertical whitespace.

garydgregory · 2025-02-27T16:01:06Z

src/main/java/org/apache/commons/net/tftp/TFTPRequestPacket.java

+                    throw new TFTPPacketException("Invalid option format");
+                }
+            }
+


Remove extra vertical whitespace.

garydgregory · 2025-03-01T00:04:28Z

Hello @vimukthiD
Make sure to rebase on git master which will pick up the latest Checlstyle rules that automatically tell you some of the comments I made above when you run the default Maven build (mvn by itself) or explicitly run Checkstyle (mvn checkstyle:check, which is faster than a whole build).

…w using `blksize` option (RFC-2348) to increase data size.

vimukthiD · 2025-03-04T13:25:19Z

Hello @garydgregory
thank you very much for checking the changes again and sorry about the delay with the changes.
I've added the fixes you've mentioned and some minor tests.
Please let me know if I've missed anything.
Thank you !

garydgregory

Hello @vimukthiD

Thank you for your updates.

The class TFTPOptionsAckPacket is not used in main. Why is it needed?

TY!

vimukthiD · 2025-03-05T11:48:37Z

Hello @garydgregory,

Oh yeah, sorry about that. At the moment it's not used, but was planning on implementing RFC-2347 which would require the TFTPOptionsAckPacket. Maybe it would be better to remove it from this changes and add it along its usages.
Let me know what you think.

Thank you!

garydgregory · 2025-03-05T11:51:42Z

Hello @vimukthiD
If it's not used, then remove it and save that work on your end for another PR in the future.

vimukthiD · 2025-03-05T12:15:28Z

Hello @garydgregory ,

Sorry for the trouble. I've removed the TFTPOptionsAckPacket

Thank you !

'blksize' option #331

garydgregory requested changes Feb 26, 2025

View reviewed changes

vimukthiD force-pushed the RFC-2348-blksize branch from 6a8940a to 7022564 Compare February 27, 2025 13:16

vimukthiD requested a review from garydgregory February 27, 2025 13:37

garydgregory requested changes Feb 27, 2025

View reviewed changes

vimukthiD added 3 commits March 4, 2025 17:33

NET-727 enable accessing options map for TFTP request packet and allo…

df92584

…w using `blksize` option (RFC-2348) to increase data size.

add review changes

7159eb8

add review changes (fix style issues and add minor tests)

f45b494

vimukthiD force-pushed the RFC-2348-blksize branch from fb3b0f9 to f45b494 Compare March 4, 2025 13:21

vimukthiD requested a review from garydgregory March 4, 2025 13:25

garydgregory reviewed Mar 4, 2025

View reviewed changes

remove unused class and tests

6a844c1

garydgregory added 2 commits March 5, 2025 08:14

Fix Javadoc formatting

4dbeb92

Javadoc formatting

e674126

garydgregory merged commit 0a85ec8 into apache:master Mar 5, 2025
12 checks passed

garydgregory changed the title ~~NET-727 enable accessing options map for TFTP request packet and allow using blksize option~~ [NET-727] Enable accessing options map for TFTP request packet and allow using blksize option Mar 5, 2025

asfgit pushed a commit that referenced this pull request Mar 5, 2025

Add accessing options map for TFTP request packet and allow using

d024e94

'blksize' option #331

Conversation

vimukthiD commented Feb 26, 2025

Uh oh!

garydgregory left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vimukthiD commented Feb 27, 2025

Uh oh!

garydgregory left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

garydgregory commented Mar 1, 2025

Uh oh!

vimukthiD commented Mar 4, 2025

Uh oh!

garydgregory left a comment

Choose a reason for hiding this comment

Uh oh!

vimukthiD commented Mar 5, 2025

Uh oh!

garydgregory commented Mar 5, 2025

Uh oh!

vimukthiD commented Mar 5, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants