garydgregory commented on code in PR #331:
URL: https://github.com/apache/commons-net/pull/331#discussion_r1971509426
##########
src/main/java/org/apache/commons/net/tftp/TFTP.java:
##########
@@ -112,6 +112,12 @@ public static final String getModeName(final int mode) {
/** A datagram used to minimize memory allocation in bufferedSend() */
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;
+
+ /** Internal state to track if the send/receive buffers are initialized */
+ private boolean buffersInityialized = false;
Review Comment:
Fix spelling of `buffersInityialized`.
##########
src/main/java/org/apache/commons/net/tftp/TFTP.java:
##########
@@ -258,4 +266,31 @@ public final void send(final TFTPPacket packet) throws
IOException {
protected void trace(final String direction, final TFTPPacket packet) {
}
+ /**
+ * Sets the size of the buffers used to receive and send packets.
+ * This method can be used to increase the size of the buffer to support
`blksize`.
+ * For which valid values range between "8" and "65464" octets (RFC-2348).
+ * This only refers to the number of data octets, it does not include the
four octets of TFTP header
+ *
+ * @param packetSize The size of the data octets not including 4 octets
for the header.
+ */
+ 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);
+
+ // if the buffers are already initialized reinitialize
+ if (buffersInityialized) {
+ endBufferedOps();
+ beginBufferedOps();
+ }
+ }
+
+ /**
+ * Returns the buffer size of the buffered used by {@link #bufferedSend
bufferedSend() } and {@link #bufferedReceive bufferedReceive() }.
Review Comment:
Javadoc: A getter "Gets..."
##########
src/main/java/org/apache/commons/net/tftp/TFTPErrorPacket.java:
##########
@@ -62,6 +62,9 @@ public final class TFTPErrorPacket extends TFTPPacket {
/** 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. */
Review Comment:
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:
##########
@@ -0,0 +1,116 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.commons.net.tftp;
+
+import java.io.ByteArrayOutputStream;
+import java.io.IOException;
+import java.net.DatagramPacket;
+import java.net.InetAddress;
+import java.nio.charset.StandardCharsets;
+import java.util.HashMap;
+import java.util.Map;
+
+/**
+ * A final class derived from TFTPPacket defining the TFTP OACK (Option
Acknowledgment) packet type.
+ * <p>
+ * Details regarding this packet type can be found in RFC 2347.
+ */
+public final class TFTPOptionsAckPacket extends TFTPPacket {
+
+ private final Map<String, String> options;
+
+ /**
+ * Creates an OACK packet informing which options are accepted.
Review Comment:
Javadoc: A constructor "Constructs..."
##########
src/main/java/org/apache/commons/net/tftp/TFTPOptionsAckPacket.java:
##########
@@ -0,0 +1,116 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.commons.net.tftp;
+
+import java.io.ByteArrayOutputStream;
+import java.io.IOException;
+import java.net.DatagramPacket;
+import java.net.InetAddress;
+import java.nio.charset.StandardCharsets;
+import java.util.HashMap;
+import java.util.Map;
+
+/**
+ * A final class derived from TFTPPacket defining the TFTP OACK (Option
Acknowledgment) packet type.
+ * <p>
+ * Details regarding this packet type can be found in RFC 2347.
+ */
+public final class TFTPOptionsAckPacket extends TFTPPacket {
+
+ private final Map<String, String> options;
+
+ /**
+ * Creates an OACK packet informing which options are accepted.
+ *
+ * @param address The host to which the packet is going to be sent.
+ * @param port The port to which the packet is going to be sent.
+ * @param options The options accepted.
+ */
+ public TFTPOptionsAckPacket(InetAddress address, int port, Map<String,
String> options) {
+ super(OACK, address, port);
+ this.options = new HashMap<>(options);
+ }
+
+ public Map<String, String> getOptions() {
+ return options;
+ }
+
+ /**
+ * Creates a UDP datagram containing all the accepted options data in the
proper format. This is a method exposed to the programmer in case he
+ * wants to implement his own TFTP client instead of using the {@link
org.apache.commons.net.tftp.TFTPClient} class. Under normal circumstances, you
should
+ * not have a need to call this method.
+ *
+ * @return A UDP datagram containing the TFTP OACK packet.
+ */
+ @Override
+ public DatagramPacket newDatagram() {
+ ByteArrayOutputStream byteStream = new ByteArrayOutputStream();
+ byteStream.write(0);
+ byteStream.write(OACK);
+
+ try {
+ for (Map.Entry<String, String> entry : options.entrySet()) {
+
byteStream.write(entry.getKey().getBytes(StandardCharsets.US_ASCII));
+ byteStream.write(0);
+
byteStream.write(entry.getValue().getBytes(StandardCharsets.US_ASCII));
+ byteStream.write(0);
+ }
+ } catch (IOException e) {
+ throw new RuntimeException("Error creating OACK packet", e);
+ }
+
+ byte[] data = byteStream.toByteArray();
+ return new DatagramPacket(data, data.length, getAddress(), getPort());
+ }
+
+ /**
+ * This is a method only available within the package for implementing
efficient datagram transport by eliminating buffering. It takes a datagram as an
Review Comment:
Say what the method does 1st: "Creates a new ..." and then a paragraph for
this detail.
##########
src/main/java/org/apache/commons/net/tftp/TFTPPacket.java:
##########
@@ -66,6 +66,12 @@ public abstract class TFTPPacket {
*/
public static final int ERROR = 5;
+ /**
+ * This is the actual TFTP spec identifier and is equal to 6.
Review Comment:
No need to "This is", just say what it is 'TFTP spec identifier {@value}.'
##########
src/main/java/org/apache/commons/net/tftp/TFTPOptionsAckPacket.java:
##########
@@ -0,0 +1,116 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.commons.net.tftp;
+
+import java.io.ByteArrayOutputStream;
+import java.io.IOException;
+import java.net.DatagramPacket;
+import java.net.InetAddress;
+import java.nio.charset.StandardCharsets;
+import java.util.HashMap;
+import java.util.Map;
+
+/**
+ * A final class derived from TFTPPacket defining the TFTP OACK (Option
Acknowledgment) packet type.
+ * <p>
+ * Details regarding this packet type can be found in RFC 2347.
+ */
Review Comment:
New public and protected elements should have a Javadoc @since tag. In this
case, for 3.12.0.
Close HTML tags.
##########
src/main/java/org/apache/commons/net/tftp/TFTP.java:
##########
@@ -258,4 +266,31 @@ public final void send(final TFTPPacket packet) throws
IOException {
protected void trace(final String direction, final TFTPPacket packet) {
}
+ /**
+ * Sets the size of the buffers used to receive and send packets.
+ * This method can be used to increase the size of the buffer to support
`blksize`.
+ * For which valid values range between "8" and "65464" octets (RFC-2348).
+ * This only refers to the number of data octets, it does not include the
four octets of TFTP header
+ *
+ * @param packetSize The size of the data octets not including 4 octets
for the header.
+ */
+ 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);
+
Review Comment:
Remove extra vertical whitespace.
##########
src/main/java/org/apache/commons/net/tftp/TFTP.java:
##########
@@ -258,4 +266,31 @@ public final void send(final TFTPPacket packet) throws
IOException {
protected void trace(final String direction, final TFTPPacket packet) {
}
+ /**
+ * Sets the size of the buffers used to receive and send packets.
+ * This method can be used to increase the size of the buffer to support
`blksize`.
+ * For which valid values range between "8" and "65464" octets (RFC-2348).
+ * This only refers to the number of data octets, it does not include the
four octets of TFTP header
+ *
+ * @param packetSize The size of the data octets not including 4 octets
for the header.
+ */
Review Comment:
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:
##########
@@ -0,0 +1,116 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.commons.net.tftp;
+
+import java.io.ByteArrayOutputStream;
+import java.io.IOException;
+import java.net.DatagramPacket;
+import java.net.InetAddress;
+import java.nio.charset.StandardCharsets;
+import java.util.HashMap;
+import java.util.Map;
+
+/**
+ * A final class derived from TFTPPacket defining the TFTP OACK (Option
Acknowledgment) packet type.
+ * <p>
+ * Details regarding this packet type can be found in RFC 2347.
+ */
+public final class TFTPOptionsAckPacket extends TFTPPacket {
+
+ private final Map<String, String> options;
+
+ /**
+ * Creates an OACK packet informing which options are accepted.
+ *
+ * @param address The host to which the packet is going to be sent.
+ * @param port The port to which the packet is going to be sent.
+ * @param options The options accepted.
+ */
+ public TFTPOptionsAckPacket(InetAddress address, int port, Map<String,
String> options) {
+ super(OACK, address, port);
+ this.options = new HashMap<>(options);
+ }
+
+ public Map<String, String> getOptions() {
+ return options;
+ }
+
+ /**
+ * Creates a UDP datagram containing all the accepted options data in the
proper format. This is a method exposed to the programmer in case he
+ * wants to implement his own TFTP client instead of using the {@link
org.apache.commons.net.tftp.TFTPClient} class. Under normal circumstances, you
should
+ * not have a need to call this method.
+ *
+ * @return A UDP datagram containing the TFTP OACK packet.
+ */
+ @Override
+ public DatagramPacket newDatagram() {
+ ByteArrayOutputStream byteStream = new ByteArrayOutputStream();
+ byteStream.write(0);
+ byteStream.write(OACK);
+
+ try {
+ for (Map.Entry<String, String> entry : options.entrySet()) {
+
byteStream.write(entry.getKey().getBytes(StandardCharsets.US_ASCII));
+ byteStream.write(0);
+
byteStream.write(entry.getValue().getBytes(StandardCharsets.US_ASCII));
+ byteStream.write(0);
+ }
+ } catch (IOException e) {
+ throw new RuntimeException("Error creating OACK packet", e);
Review Comment:
Throwing a `RuntimeException` is an anti-pattern. If you must convert, then
use an `UncheckedIOException` or Commons IO's `Uncheck` class.
##########
src/main/java/org/apache/commons/net/tftp/TFTPOptionsAckPacket.java:
##########
@@ -0,0 +1,116 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.commons.net.tftp;
+
+import java.io.ByteArrayOutputStream;
+import java.io.IOException;
+import java.net.DatagramPacket;
+import java.net.InetAddress;
+import java.nio.charset.StandardCharsets;
+import java.util.HashMap;
+import java.util.Map;
+
+/**
+ * A final class derived from TFTPPacket defining the TFTP OACK (Option
Acknowledgment) packet type.
+ * <p>
+ * Details regarding this packet type can be found in RFC 2347.
+ */
+public final class TFTPOptionsAckPacket extends TFTPPacket {
+
+ private final Map<String, String> options;
+
+ /**
+ * Creates an OACK packet informing which options are accepted.
+ *
+ * @param address The host to which the packet is going to be sent.
+ * @param port The port to which the packet is going to be sent.
+ * @param options The options accepted.
+ */
+ public TFTPOptionsAckPacket(InetAddress address, int port, Map<String,
String> options) {
+ super(OACK, address, port);
+ this.options = new HashMap<>(options);
+ }
+
+ public Map<String, String> getOptions() {
Review Comment:
All new public and protected elements should have Javadoc comments.
##########
src/main/java/org/apache/commons/net/tftp/TFTPOptionsAckPacket.java:
##########
@@ -0,0 +1,116 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.commons.net.tftp;
+
+import java.io.ByteArrayOutputStream;
+import java.io.IOException;
+import java.net.DatagramPacket;
+import java.net.InetAddress;
+import java.nio.charset.StandardCharsets;
+import java.util.HashMap;
+import java.util.Map;
+
+/**
+ * A final class derived from TFTPPacket defining the TFTP OACK (Option
Acknowledgment) packet type.
+ * <p>
+ * Details regarding this packet type can be found in RFC 2347.
+ */
+public final class TFTPOptionsAckPacket extends TFTPPacket {
+
+ private final Map<String, String> options;
+
+ /**
+ * Creates an OACK packet informing which options are accepted.
+ *
+ * @param address The host to which the packet is going to be sent.
+ * @param port The port to which the packet is going to be sent.
+ * @param options The options accepted.
+ */
+ public TFTPOptionsAckPacket(InetAddress address, int port, Map<String,
String> options) {
+ super(OACK, address, port);
+ this.options = new HashMap<>(options);
+ }
+
+ public Map<String, String> getOptions() {
+ return options;
+ }
+
+ /**
+ * Creates a UDP datagram containing all the accepted options data in the
proper format. This is a method exposed to the programmer in case he
+ * wants to implement his own TFTP client instead of using the {@link
org.apache.commons.net.tftp.TFTPClient} class. Under normal circumstances, you
should
+ * not have a need to call this method.
+ *
+ * @return A UDP datagram containing the TFTP OACK packet.
+ */
+ @Override
+ public DatagramPacket newDatagram() {
+ ByteArrayOutputStream byteStream = new ByteArrayOutputStream();
+ byteStream.write(0);
+ byteStream.write(OACK);
+
+ try {
+ for (Map.Entry<String, String> entry : options.entrySet()) {
+
byteStream.write(entry.getKey().getBytes(StandardCharsets.US_ASCII));
+ byteStream.write(0);
+
byteStream.write(entry.getValue().getBytes(StandardCharsets.US_ASCII));
+ byteStream.write(0);
+ }
+ } catch (IOException e) {
+ throw new RuntimeException("Error creating OACK packet", e);
+ }
+
+ byte[] data = byteStream.toByteArray();
+ return new DatagramPacket(data, data.length, getAddress(), getPort());
+ }
+
+ /**
+ * This is a method only available within the package for implementing
efficient datagram transport by eliminating buffering. It takes a datagram as an
+ * argument, and a byte buffer in which to store the raw datagram data.
Inside the method, the data is set as the datagram's data and the datagram
returned.
+ *
+ * @param datagram The datagram to create.
+ * @param data The buffer to store the packet and to use in the
datagram.
+ * @return The datagram argument.
+ */
+ @Override
+ DatagramPacket newDatagram(DatagramPacket datagram, byte[] data) {
+ int offset = 0;
+ data[offset++] = 0;
+ data[offset++] = (byte) type;
+
+ for (Map.Entry<String, String> entry : options.entrySet()) {
+ byte[] key = entry.getKey().getBytes(StandardCharsets.US_ASCII);
+ System.arraycopy(key, 0, data, offset, key.length);
+ offset += key.length;
+
Review Comment:
No need for blank lines, if there is something to call out, then write an
inline // comment.
##########
src/main/java/org/apache/commons/net/tftp/TFTPRequestPacket.java:
##########
@@ -151,6 +183,16 @@ public final int getMode() {
return mode;
}
+ /**
+ * Returns the options extensions of the request as a map.
Review Comment:
A getter "Gets...".
--
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]
