Fix FindBugs violations and enable enforcement 39/67639/3
authorTom Pantelis <tompantelis@gmail.com>
Sat, 27 Jan 2018 16:30:09 +0000 (11:30 -0500)
committerTom Pantelis <tompantelis@gmail.com>
Fri, 9 Feb 2018 15:17:35 +0000 (10:17 -0500)
- Method call passes null for non-null parameter
- Method ignores exceptional return value
- Boxing/unboxing to parse a primitive
- Redundant nullcheck of value known to be non-null
- Constructor invokes Thread.start()
- Load of known null value

Change-Id: I3cf3c48306c1ade906fe51b5c0492f5a669a2d43
Signed-off-by: Tom Pantelis <tompantelis@gmail.com>
13 files changed:
.gitignore
addresstracker/implementation/src/main/java/org/opendaylight/l2switch/addresstracker/addressobserver/AddressTrackerProvider.java
arphandler/src/main/java/org/opendaylight/l2switch/arphandler/core/PacketDispatcher.java
arphandler/src/main/java/org/opendaylight/l2switch/arphandler/flow/InitialFlowWriter.java
arphandler/src/test/java/org/opendaylight/l2switch/arphandler/core/PacketDispatcherTest.java
hosttracker/implementation/src/main/java/org/opendaylight/l2switch/hosttracker/plugin/internal/HostTrackerImpl.java
hosttracker/implementation/src/main/java/org/opendaylight/l2switch/hosttracker/plugin/internal/OperationProcessor.java
l2switch-main/src/main/java/org/opendaylight/l2switch/flow/InitialFlowWriter.java
loopremover/implementation/src/main/java/org/opendaylight/l2switch/loopremover/flow/InitialFlowWriter.java
packethandler/implementation/src/main/java/org/opendaylight/l2switch/packethandler/decoders/AbstractPacketDecoder.java
packethandler/implementation/src/main/java/org/opendaylight/l2switch/packethandler/decoders/utils/BitBufferHelper.java
packethandler/implementation/src/main/java/org/opendaylight/l2switch/packethandler/decoders/utils/NetUtils.java
parent/pom.xml

index 9d9e33aeef0452a10a23cf21853a722aab78a314..e9e9e8b72db0d109d4782e93ca05e979c98487c6 100644 (file)
@@ -22,3 +22,4 @@ yang-gen-config
 maven-eclipse.xml\r
 .DS_Store\r
 .checkstyle\r
+.fbExcludeFilterFile\r
index 1b083e78e6b2deec7ea00e068db1831c7c7a784b..3b387f3c9b871fd74c906435ffbfd2ad2356a068 100644 (file)
@@ -12,6 +12,7 @@ import java.util.ArrayList;
 import java.util.HashSet;
 import java.util.List;
 import java.util.Set;
+import javax.annotation.Nonnull;
 import org.opendaylight.controller.md.sal.binding.api.DataBroker;
 import org.opendaylight.controller.sal.binding.api.NotificationProviderService;
 import org.opendaylight.yang.gen.v1.urn.opendaylight.packet.address.tracker.config.rev160621.AddressTrackerConfig;
@@ -46,8 +47,7 @@ public class AddressTrackerProvider {
         addressObservationWriter.setTimestampUpdateInterval(timestampUpdateInterval);
         Set<String> packetTypes = processObserveAddressesFrom(observerAddressesFrom);
 
-        if (packetTypes == null || packetTypes.isEmpty()) { // set default to
-                                                            // arp
+        if (packetTypes.isEmpty()) { // set default to arp
             packetTypes = new HashSet<>();
             packetTypes.add(ARP_PACKET_TYPE);
         }
@@ -77,6 +77,7 @@ public class AddressTrackerProvider {
         LOG.info("AddressTracker torn down.", this);
     }
 
+    @Nonnull
     private Set<String> processObserveAddressesFrom(String observeAddressesFrom) {
         Set<String> packetTypes = new HashSet<>();
         if (observeAddressesFrom == null || observeAddressesFrom.isEmpty()) {
@@ -84,7 +85,7 @@ public class AddressTrackerProvider {
             return packetTypes;
         }
         String[] observeAddressFromSplit = observeAddressesFrom.split(",");
-        if (observeAddressFromSplit == null || observeAddressFromSplit.length == 0) {
+        if (observeAddressFromSplit.length == 0) {
             packetTypes.add(ARP_PACKET_TYPE);
             return packetTypes;
         }
index de0a7583f4fba0b880223e53e75a20d489d1ec62..014a9276d79ddcb4ba47c3e4120a0684a362bd45 100644 (file)
@@ -8,6 +8,10 @@
 
 package org.opendaylight.l2switch.arphandler.core;
 
+import com.google.common.util.concurrent.FutureCallback;
+import com.google.common.util.concurrent.Futures;
+import com.google.common.util.concurrent.JdkFutureAdapters;
+import com.google.common.util.concurrent.MoreExecutors;
 import java.util.List;
 import org.opendaylight.l2switch.arphandler.inventory.InventoryReader;
 import org.opendaylight.yang.gen.v1.urn.ietf.params.xml.ns.yang.ietf.yang.types.rev130715.MacAddress;
@@ -21,6 +25,7 @@ import org.opendaylight.yang.gen.v1.urn.opendaylight.packet.service.rev130709.Pa
 import org.opendaylight.yang.gen.v1.urn.opendaylight.packet.service.rev130709.TransmitPacketInput;
 import org.opendaylight.yang.gen.v1.urn.opendaylight.packet.service.rev130709.TransmitPacketInputBuilder;
 import org.opendaylight.yangtools.yang.binding.InstanceIdentifier;
+import org.opendaylight.yangtools.yang.common.RpcResult;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
@@ -133,7 +138,19 @@ public class PacketDispatcher {
                 .setEgress(egress) //
                 .setIngress(ingress) //
                 .build();
-        packetProcessingService.transmitPacket(input);
+
+        Futures.addCallback(JdkFutureAdapters.listenInPoolThread(packetProcessingService.transmitPacket(input)),
+            new FutureCallback<RpcResult<Void>>() {
+                @Override
+                public void onSuccess(RpcResult<Void> result) {
+                    LOG.debug("transmitPacket was successful");
+                }
+
+                @Override
+                public void onFailure(Throwable failure) {
+                    LOG.debug("transmitPacket for {} failed", input, failure);
+                }
+            }, MoreExecutors.directExecutor());
     }
 
     private void refreshInventoryReader() {
index d320bcc0221d27192ca822274f753c3cf12dd295..396f79391de7a359b7f6d03d5730b360dbd9ff78 100644 (file)
@@ -139,7 +139,7 @@ public class InitialFlowWriter implements DataTreeChangeListener<Node> {
         }
 
         if (!nodeIds.isEmpty()) {
-            initialFlowExecutor.submit(new InitialFlowWriterProcessor(nodeIds));
+            initialFlowExecutor.execute(new InitialFlowWriterProcessor(nodeIds));
         }
     }
 
index 1af47a62291d77e36b4b9c72393ea8d3d3660fbb..6692e09f19c0244614ae5c9155c3f6f6c47c5849 100644 (file)
@@ -7,7 +7,8 @@
  */
 package org.opendaylight.l2switch.arphandler.core;
 
-import static org.mockito.Mockito.any;
+import static org.mockito.Matchers.any;
+import static org.mockito.Mockito.doReturn;
 import static org.mockito.Mockito.times;
 import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.when;
@@ -17,6 +18,7 @@ import java.util.HashMap;
 import java.util.List;
 import org.junit.Before;
 import org.junit.Test;
+import org.mockito.Mock;
 import org.mockito.Mockito;
 import org.mockito.MockitoAnnotations;
 import org.opendaylight.l2switch.arphandler.inventory.InventoryReader;
@@ -32,11 +34,12 @@ import org.opendaylight.yang.gen.v1.urn.opendaylight.inventory.rev130819.nodes.N
 import org.opendaylight.yang.gen.v1.urn.opendaylight.packet.service.rev130709.PacketProcessingService;
 import org.opendaylight.yang.gen.v1.urn.opendaylight.packet.service.rev130709.TransmitPacketInput;
 import org.opendaylight.yangtools.yang.binding.InstanceIdentifier;
+import org.opendaylight.yangtools.yang.common.RpcResultBuilder;
 
 public class PacketDispatcherTest {
-    @MockitoAnnotations.Mock
+    @Mock
     private PacketProcessingService packetProcessingService;
-    @MockitoAnnotations.Mock
+    @Mock
     private InventoryReader inventoryReader;
     private PacketDispatcher packetDispatcher;
 
@@ -46,6 +49,8 @@ public class PacketDispatcherTest {
         packetDispatcher = new PacketDispatcher();
         packetDispatcher.setPacketProcessingService(packetProcessingService);
         packetDispatcher.setInventoryReader(inventoryReader);
+
+        doReturn(RpcResultBuilder.success().buildFuture()).when(packetProcessingService).transmitPacket(any());
     }
 
     @Test
@@ -72,7 +77,7 @@ public class PacketDispatcherTest {
 
     @Test
     public void testFloodPacket() throws Exception {
-        List<NodeConnectorRef> nodeConnectors = new ArrayList<NodeConnectorRef>();
+        List<NodeConnectorRef> nodeConnectors = new ArrayList<>();
         InstanceIdentifier<NodeConnector> ncInsId1 = InstanceIdentifier.builder(Nodes.class).child(Node.class)
                 .child(NodeConnector.class, new NodeConnectorKey(new NodeConnectorId("1"))).build();
         InstanceIdentifier<NodeConnector> ncInsId2 = InstanceIdentifier.builder(Nodes.class).child(Node.class)
@@ -147,7 +152,7 @@ public class PacketDispatcherTest {
         when(inventoryReader.getControllerSwitchConnectors()).thenReturn(controllerSwitchConnectors);
         when(inventoryReader.getNodeConnector(any(InstanceIdentifier.class), any(MacAddress.class))).thenReturn(null);
 
-        List<NodeConnectorRef> nodeConnectors = new ArrayList<NodeConnectorRef>();
+        List<NodeConnectorRef> nodeConnectors = new ArrayList<>();
         InstanceIdentifier<NodeConnector> ncInsId2 = InstanceIdentifier.builder(Nodes.class)
                 .child(Node.class, new NodeKey(new NodeId("2")))
                 .child(NodeConnector.class, new NodeConnectorKey(new NodeConnectorId("2"))).build();
index 50c8efc65866471dc884e36b7c9daa9c04c6221e..2fd37c3367e26c2e1e442cef19589018c8e44ee7 100644 (file)
@@ -17,6 +17,7 @@ import java.util.concurrent.ExecutionException;
 import java.util.concurrent.Executors;
 import java.util.concurrent.ScheduledExecutorService;
 import java.util.concurrent.TimeUnit;
+import javax.annotation.Nonnull;
 import org.opendaylight.controller.md.sal.binding.api.DataBroker;
 import org.opendaylight.controller.md.sal.binding.api.DataObjectModification;
 import org.opendaylight.controller.md.sal.binding.api.DataTreeChangeListener;
@@ -66,6 +67,7 @@ public class HostTrackerImpl implements DataTreeChangeListener<DataObject> {
     private final ConcurrentClusterAwareHostHashMap hosts;
     private final ConcurrentClusterAwareLinkHashMap links;
     private final OperationProcessor opProcessor;
+    private final Thread processorThread;
     private ListenerRegistration<DataTreeChangeListener> addrsNodeListenerRegistration;
     private ListenerRegistration<DataTreeChangeListener> hostNodeListenerRegistration;
     private ListenerRegistration<DataTreeChangeListener> linkNodeListenerRegistration;
@@ -85,8 +87,7 @@ public class HostTrackerImpl implements DataTreeChangeListener<DataObject> {
         this.hostPurgeAge = config.getHostPurgeAge();
         this.hostPurgeInterval = config.getHostPurgeInterval();
         this.opProcessor = new OperationProcessor(dataService);
-        Thread processorThread = new Thread(opProcessor);
-        processorThread.start();
+        processorThread = new Thread(opProcessor);
         final String maybeTopologyId = config.getTopologyId();
         if (maybeTopologyId == null || maybeTopologyId.isEmpty()) {
             this.topologyId = TOPOLOGY_NAME;
@@ -104,6 +105,8 @@ public class HostTrackerImpl implements DataTreeChangeListener<DataObject> {
 
     @SuppressWarnings("unchecked")
     public void init() {
+        processorThread.start();
+
         InstanceIdentifier<Addresses> addrCapableNodeConnectors = //
                 InstanceIdentifier.builder(Nodes.class) //
                         .child(org.opendaylight.yang.gen.v1.urn.opendaylight.inventory.rev130819.nodes.Node.class)
@@ -379,7 +382,7 @@ public class HostTrackerImpl implements DataTreeChangeListener<DataObject> {
         for (Host h : hosts.values()) {
             final HostNode hn = h.getHostNode().getAugmentation(HostNode.class);
             if (hn == null) {
-                LOG.warn("Encountered non-host node {} in hosts during purge", hn);
+                LOG.warn("Encountered non-host node {} in hosts during purge", h);
             } else if (hn.getAddresses() != null) {
                 boolean purgeHosts = false;
                 // if the node is a host and has addresses, check to see if it's been seen recently
@@ -421,7 +424,7 @@ public class HostTrackerImpl implements DataTreeChangeListener<DataObject> {
      *
      * @param host  reference to Host node
      */
-    private int removeHosts(final Host host, int numHostsPurged) {
+    private int removeHosts(@Nonnull final Host host, int numHostsPurged) {
         // remove associated links with the host before removing hosts
         removeAssociatedLinksFromHosts(host);
         // purge hosts from local & MD-SAL database
@@ -441,25 +444,22 @@ public class HostTrackerImpl implements DataTreeChangeListener<DataObject> {
      *
      * @param host  reference to Host node
      */
-    private void removeAssociatedLinksFromHosts(final Host host) {
-        if (host != null) {
-            if (host.getId() != null) {
-                List<Link> linksToRemove = new ArrayList<>();
-                for (Link link: links.values()) {
-                    if (link.toString().contains(host.getId().getValue())) {
-                        linksToRemove.add(link);
-                    }
+    private void removeAssociatedLinksFromHosts(@Nonnull final Host host) {
+        if (host.getId() != null) {
+            List<Link> linksToRemove = new ArrayList<>();
+            for (Link link: links.values()) {
+                if (link.toString().contains(host.getId().getValue())) {
+                    linksToRemove.add(link);
                 }
-                links.removeAll(linksToRemove);
-            } else {
-                LOG.warn("Encountered host with no id , Unexpected host id {}. ", host);
             }
+            links.removeAll(linksToRemove);
         } else {
-            LOG.warn("Encountered Host with no value, Unexpected host {}. ", host);
+            LOG.warn("Encountered host with no id , Unexpected host id {}. ", host);
         }
     }
 
     public void close() {
+        processorThread.interrupt();
         this.addrsNodeListenerRegistration.close();
         this.hostNodeListenerRegistration.close();
         this.linkNodeListenerRegistration.close();
index 37fe218e1e69978e33f66e5a9b798073b41534bc..69746d8c4812859a9f1dae934ca373e7c1bb67cf 100644 (file)
@@ -13,6 +13,7 @@ import com.google.common.util.concurrent.Futures;
 import com.google.common.util.concurrent.MoreExecutors;
 import java.util.concurrent.BlockingQueue;
 import java.util.concurrent.LinkedBlockingQueue;
+import java.util.concurrent.atomic.AtomicReference;
 import org.opendaylight.controller.md.sal.binding.api.BindingTransactionChain;
 import org.opendaylight.controller.md.sal.binding.api.DataBroker;
 import org.opendaylight.controller.md.sal.binding.api.ReadWriteTransaction;
@@ -31,12 +32,12 @@ public class OperationProcessor implements AutoCloseable, Runnable, TransactionC
     private static final Logger LOG = LoggerFactory.getLogger(OperationProcessor.class);
     private final DataBroker dataBroker;
     private final BlockingQueue<HostTrackerOperation> queue;
-    private BindingTransactionChain transactionChain;
+    private final AtomicReference<BindingTransactionChain> transactionChain = new AtomicReference<>();
 
     OperationProcessor(final DataBroker dataBroker) {
         this.dataBroker = Preconditions.checkNotNull(dataBroker);
         this.queue = new LinkedBlockingQueue<>(QUEUE_DEPTH);
-        this.transactionChain = dataBroker.createTransactionChain(this);
+        this.transactionChain.set(dataBroker.createTransactionChain(this));
     }
 
     @Override
@@ -55,7 +56,12 @@ public class OperationProcessor implements AutoCloseable, Runnable, TransactionC
         while (!done) {
             try {
                 HostTrackerOperation op = queue.take();
-                ReadWriteTransaction tx = transactionChain.newReadWriteTransaction();
+                final BindingTransactionChain txChain = transactionChain.get();
+                if (txChain == null) {
+                    break;
+                }
+
+                ReadWriteTransaction tx = txChain.newReadWriteTransaction();
 
                 int ops = 0;
                 while (op != null && ops < OPS_PER_CHAIN) {
@@ -73,16 +79,20 @@ public class OperationProcessor implements AutoCloseable, Runnable, TransactionC
     }
 
     @Override
-    public void close() throws Exception {
-        if (transactionChain != null) {
-            transactionChain.close();
+    public void close() {
+        final BindingTransactionChain txChain = transactionChain.getAndSet(null);
+        if (txChain != null) {
+            txChain.close();
         }
     }
 
     private void chainFailure() {
         try {
-            transactionChain.close();
-            transactionChain = dataBroker.createTransactionChain(this);
+            final BindingTransactionChain prevChain = transactionChain.getAndSet(
+                    dataBroker.createTransactionChain(this));
+            if (prevChain != null) {
+                prevChain.close();
+            }
             clearQueue();
         } catch (IllegalStateException e) {
             LOG.warn(e.getLocalizedMessage());
index 53e0373bdcd60e81a14a644b6d44a7c730b06d76..3784d2a6054be315009db0aa6896dfe62631aafb 100644 (file)
@@ -123,7 +123,7 @@ public class InitialFlowWriter implements DataTreeChangeListener<Node> {
         }
 
         if (!nodeIds.isEmpty()) {
-            initialFlowExecutor.submit(new InitialFlowWriterProcessor(nodeIds));
+            initialFlowExecutor.execute(new InitialFlowWriterProcessor(nodeIds));
         }
     }
 
index 26ea89d07c12faf244d72e08b4e472c1a93f619a..d4859a19f7dc151195f20457cd18d1a570886d2c 100644 (file)
@@ -132,7 +132,7 @@ public class InitialFlowWriter implements DataTreeChangeListener<Node> {
         }
 
         if (!nodeIds.isEmpty()) {
-            initialFlowExecutor.submit(new InitialFlowWriterProcessor(nodeIds));
+            initialFlowExecutor.execute(new InitialFlowWriterProcessor(nodeIds));
         }
     }
 
index d3c305aa3183ec2b1856c93676d87fafd7b6cac5..f006788439d42c862b0bb8150947dc1785205b70 100644 (file)
@@ -59,7 +59,7 @@ public abstract class AbstractPacketDecoder<C, P extends Notification>
      * necessary and publishes corresponding event on successful decoding.
      */
     public void decodeAndPublish(final C consumedPacketNotification) {
-        decodeAndPublishExecutor.submit(() -> {
+        decodeAndPublishExecutor.execute(() -> {
             P packetNotification = null;
             if (consumedPacketNotification != null && canDecode(consumedPacketNotification)) {
                 packetNotification = decode(consumedPacketNotification);
@@ -73,8 +73,6 @@ public abstract class AbstractPacketDecoder<C, P extends Notification>
     /**
      * Decodes the payload in given Packet further and returns a extension of
      * Packet. e.g. ARP, IPV4, LLDP etc.
-     *
-     * @return
      */
     public abstract P decode(C consumedPacketNotification);
 
index c5cdbd2a2b195b79adaba9d4fe8901707598f9a4..f0d6699b11374310100700970b89b31499df5fc5 100644 (file)
@@ -9,6 +9,7 @@
 package org.opendaylight.l2switch.packethandler.decoders.utils;
 
 import java.util.Arrays;
+import javax.annotation.Nonnull;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
@@ -60,9 +61,10 @@ public final class BitBufferHelper {
         int startOffset = data.length * NetUtils.NUM_BITS_IN_A_BYTE - numBits;
         byte[] bits = null;
         try {
-            bits = BitBufferHelper.getBits(data, startOffset, numBits);
+            bits = getBits(data, startOffset, numBits);
         } catch (BufferException e) {
-            LOG.error("", e);
+            LOG.error("getBits failed", e);
+            return 0;
         }
         return (short) toNumber(bits, numBits);
     }
@@ -98,7 +100,8 @@ public final class BitBufferHelper {
         try {
             bits = BitBufferHelper.getBits(data, startOffset, numBits);
         } catch (BufferException e) {
-            LOG.error("", e);
+            LOG.error("getBits failed", e);
+            return 0;
         }
         return (int) toNumber(bits, numBits);
     }
@@ -137,7 +140,8 @@ public final class BitBufferHelper {
         try {
             bits = BitBufferHelper.getBits(data, startOffset, numBits);
         } catch (BufferException e) {
-            LOG.error("", e);
+            LOG.error("getBits failed", e);
+            return 0;
         }
         return toNumber(bits, numBits);
     }
@@ -329,7 +333,7 @@ public final class BitBufferHelper {
      * @param numBits the number of bits
      * @return numerical value of byte array passed
      */
-    public static long toNumber(byte[] array, int numBits) {
+    public static long toNumber(@Nonnull byte[] array, int numBits) {
         int length = numBits / NetUtils.NUM_BITS_IN_A_BYTE;
         int bitsRest = numBits % NetUtils.NUM_BITS_IN_A_BYTE;
         int startOffset = array.length - length;
index 250d3b0499fa7dc89d151d57427178a722e5db46..f70a33ed4b17be96d9eaa828229ce7fc81b74d4a 100644 (file)
@@ -141,7 +141,6 @@ public final class NetUtils {
      *            integer representing the length of the prefix network mask
      * @param isV6
      *            boolean representing the IP version of the returned address
-     * @return
      */
     public static InetAddress getInetNetworkMask(int prefixMaskLength, boolean isV6) {
         if (prefixMaskLength < 0 || !isV6 && prefixMaskLength > 32 || isV6 && prefixMaskLength > 128) {
@@ -406,7 +405,7 @@ public final class NetUtils {
             return false;
         }
         if (values.length >= 2) {
-            int prefix = Integer.valueOf(values[1]);
+            int prefix = Integer.parseInt(values[1]);
             if (prefix < 0 || prefix > 32) {
                 return false;
             }
@@ -439,7 +438,7 @@ public final class NetUtils {
         }
 
         if (values.length >= 2) {
-            int prefix = Integer.valueOf(values[1]);
+            int prefix = Integer.parseInt(values[1]);
             if (prefix < 0 || prefix > 128) {
                 return false;
             }
index cad70b9f09384fbf1c1e56d4975d0d8e43b59771..e378f7b37e3a61759f2571cbe43d565d800a520a 100644 (file)
               <configuration>
                   <propertyExpansion>checkstyle.violationSeverity=error</propertyExpansion>
               </configuration>
-
+          </plugin>
+          <plugin>
+              <groupId>org.codehaus.mojo</groupId>
+              <artifactId>findbugs-maven-plugin</artifactId>
+              <configuration>
+                  <failOnError>true</failOnError>
+              </configuration>
           </plugin>
       </plugins>
   </build>