Fix findbugs violations in samples 83/68383/2
authorTom Pantelis <tompantelis@gmail.com>
Sat, 17 Feb 2018 23:31:07 +0000 (18:31 -0500)
committerTom Pantelis <tompantelis@gmail.com>
Wed, 28 Feb 2018 18:45:18 +0000 (13:45 -0500)
- Inconsistent synchronization
- Using notify() rather than notifyAll()
- Method ignores return value
- Read of unwritten field
- Field not initialized in constructor but dereferenced without null check

Change-Id: I12e8ae3ab25cc41ba05b99e9e4c85f2606a2814d
Signed-off-by: Tom Pantelis <tompantelis@gmail.com>
samples/learning-switch/pom.xml
samples/learning-switch/src/main/java/org/opendaylight/openflowplugin/learningswitch/LearningSwitchHandler.java
samples/learning-switch/src/main/java/org/opendaylight/openflowplugin/learningswitch/LearningSwitchHandlerSimpleImpl.java
samples/learning-switch/src/main/java/org/opendaylight/openflowplugin/learningswitch/LearningSwitchManagerSimpleImpl.java
samples/learning-switch/src/main/java/org/opendaylight/openflowplugin/learningswitch/multi/LearningSwitchManagerMultiImpl.java
samples/learning-switch/src/main/java/org/opendaylight/openflowplugin/learningswitch/multi/MultipleLearningSwitchHandlerFacadeImpl.java
samples/sample-consumer/src/main/java/org/opendaylight/openflowplugin/openflow/samples/consumer/Activator.java
samples/sample-consumer/src/main/java/org/opendaylight/openflowplugin/openflow/samples/consumer/SimpleDropFirewall.java
samples/sample-consumer/src/main/java/org/opendaylight/openflowplugin/openflow/samples/consumer/SimpleDropFirewallCli.java
samples/simple-client/src/main/java/org/opendaylight/openflowjava/protocol/impl/clients/ScenarioHandler.java

index fb18e494e19d0a8ee1daba8a124c838b72223b40..60f33ad0c2a1d50ad06eb4c939c9b47830e9160d 100644 (file)
             <groupId>org.opendaylight.openflowplugin</groupId>
             <artifactId>openflowplugin-api</artifactId>
         </dependency>
+        <dependency>
+            <groupId>org.opendaylight.infrautils</groupId>
+            <artifactId>infrautils-util</artifactId>
+            <version>${infrautils.version}</version>
+        </dependency>
         <dependency>
           <groupId>org.osgi</groupId>
           <artifactId>org.osgi.core</artifactId>
index 48689701d10533e2dc2e6725dbda19b521b2b532..0584e12c16b74acf35d1da54334f61c03d0b13f9 100644 (file)
@@ -8,7 +8,6 @@
 package org.opendaylight.openflowplugin.learningswitch;
 
 import org.opendaylight.yang.gen.v1.urn.opendaylight.flow.inventory.rev130819.tables.Table;
-import org.opendaylight.yang.gen.v1.urn.opendaylight.packet.service.rev130709.PacketProcessingService;
 import org.opendaylight.yangtools.yang.binding.InstanceIdentifier;
 
 public interface LearningSwitchHandler {
@@ -19,25 +18,4 @@ public interface LearningSwitchHandler {
      * @param tablePath the table path
      */
     void onSwitchAppeared(InstanceIdentifier<Table> tablePath);
-
-    /**
-     * Sets the PacketProcessingService.
-     *
-     * @param packetProcessingService the packetProcessingService to set
-     */
-    void setPacketProcessingService(PacketProcessingService packetProcessingService);
-
-   /**
-    * Sets the data store accessor.
-    *
-    * @param dataStoreAccessor the dataStoreAccessor to set
-    */
-    void setDataStoreAccessor(FlowCommitWrapper dataStoreAccessor);
-
-   /**
-    * Sets the DataTreeChangeListener registration publisher.
-    *
-    * @param registrationPublisher the registrationPublisher to set
-    */
-    void setRegistrationPublisher(DataTreeChangeListenerRegistrationHolder registrationPublisher);
 }
index 9c4163676ee07e91d5705f20995126ae4d0b5381..d1a0b9be0ad08c26cca16637906dc30cf75fd407 100644 (file)
@@ -14,8 +14,12 @@ import java.util.Arrays;
 import java.util.HashMap;
 import java.util.HashSet;
 import java.util.Map;
+import java.util.Objects;
 import java.util.Set;
 import java.util.concurrent.atomic.AtomicLong;
+import javax.annotation.Nonnull;
+import javax.annotation.Nullable;
+import org.opendaylight.infrautils.utils.concurrent.JdkFutures;
 import org.opendaylight.yang.gen.v1.urn.ietf.params.xml.ns.yang.ietf.yang.types.rev130715.MacAddress;
 import org.opendaylight.yang.gen.v1.urn.opendaylight.flow.inventory.rev130819.FlowId;
 import org.opendaylight.yang.gen.v1.urn.opendaylight.flow.inventory.rev130819.tables.Table;
@@ -49,21 +53,29 @@ public class LearningSwitchHandlerSimpleImpl implements LearningSwitchHandler, P
     private static final byte[] ETH_TYPE_IPV4 = new byte[] { 0x08, 0x00 };
     private static final int DIRECT_FLOW_PRIORITY = 512;
 
-    private DataTreeChangeListenerRegistrationHolder registrationPublisher;
-    private FlowCommitWrapper dataStoreAccessor;
-    private PacketProcessingService packetProcessingService;
+    private final DataTreeChangeListenerRegistrationHolder registrationPublisher;
+    private final FlowCommitWrapper dataStoreAccessor;
+    private final PacketProcessingService packetProcessingService;
 
-    private boolean isLearning = false;
+    private volatile boolean isLearning = false;
 
     private NodeId nodeId;
     private final AtomicLong flowIdInc = new AtomicLong();
     private final AtomicLong flowCookieInc = new AtomicLong(0x2a00000000000000L);
 
     private InstanceIdentifier<Node> nodePath;
-    private InstanceIdentifier<Table> tablePath;
+    private volatile InstanceIdentifier<Table> tablePath;
 
     private Map<MacAddress, NodeConnectorRef> mac2portMapping;
-    private Set<String> coveredMacPaths;
+    private final Set<String> coveredMacPaths = new HashSet<>();
+
+    public LearningSwitchHandlerSimpleImpl(@Nonnull FlowCommitWrapper dataStoreAccessor,
+            @Nonnull PacketProcessingService packetProcessingService,
+            @Nullable DataTreeChangeListenerRegistrationHolder registrationPublisher) {
+        this.dataStoreAccessor = Objects.requireNonNull(dataStoreAccessor);
+        this.packetProcessingService = Objects.requireNonNull(packetProcessingService);
+        this.registrationPublisher = registrationPublisher;
+    }
 
     @Override
     public synchronized void onSwitchAppeared(InstanceIdentifier<Table> appearedTablePath) {
@@ -86,7 +98,6 @@ public class LearningSwitchHandlerSimpleImpl implements LearningSwitchHandler, P
         nodePath = tablePath.firstIdentifierOf(Node.class);
         nodeId = nodePath.firstKeyOf(Node.class, NodeKey.class).getId();
         mac2portMapping = new HashMap<>();
-        coveredMacPaths = new HashSet<>();
 
         // start forwarding all packages to controller
         FlowId flowId = new FlowId(String.valueOf(flowIdInc.getAndIncrement()));
@@ -103,21 +114,6 @@ public class LearningSwitchHandlerSimpleImpl implements LearningSwitchHandler, P
         dataStoreAccessor.writeFlowToConfig(flowPath, allToCtrlFlow.build());
     }
 
-    @Override
-    public void setRegistrationPublisher(DataTreeChangeListenerRegistrationHolder registrationPublisher) {
-        this.registrationPublisher = registrationPublisher;
-    }
-
-    @Override
-    public void setDataStoreAccessor(FlowCommitWrapper dataStoreAccessor) {
-        this.dataStoreAccessor = dataStoreAccessor;
-    }
-
-    @Override
-    public void setPacketProcessingService(PacketProcessingService packetProcessingService) {
-        this.packetProcessingService = packetProcessingService;
-    }
-
     @Override
     public void onPacketReceived(PacketReceived notification) {
         if (!isLearning) {
@@ -228,6 +224,6 @@ public class LearningSwitchHandlerSimpleImpl implements LearningSwitchHandler, P
                 .setEgress(egress)
                 .setIngress(ingress)
                 .build();
-        packetProcessingService.transmitPacket(input);
+        JdkFutures.addErrorLogging(packetProcessingService.transmitPacket(input), LOG, "transmitPacket");
     }
 }
index 6dc9c9ae17a83120559f650d1880d37f232c4f32..3e59b0646c2fa0546dbb4a97a02fd095891b846b 100644 (file)
@@ -79,10 +79,8 @@ public class LearningSwitchManagerSimpleImpl
         LOG.debug("start() -->");
         FlowCommitWrapper dataStoreAccessor = new FlowCommitWrapperImpl(data);
 
-        LearningSwitchHandlerSimpleImpl learningSwitchHandler = new LearningSwitchHandlerSimpleImpl();
-        learningSwitchHandler.setRegistrationPublisher(this);
-        learningSwitchHandler.setDataStoreAccessor(dataStoreAccessor);
-        learningSwitchHandler.setPacketProcessingService(packetProcessingService);
+        LearningSwitchHandlerSimpleImpl learningSwitchHandler = new LearningSwitchHandlerSimpleImpl(dataStoreAccessor,
+                packetProcessingService, this);
         packetInRegistration = notificationService.registerNotificationListener(learningSwitchHandler);
 
         WakeupOnNode wakeupListener = new WakeupOnNode();
index 68f513d2855d34399fd6519ff1c1ed1abd9e5d97..10808af6714218c16d719a1d0fcf443991632591 100644 (file)
@@ -86,11 +86,8 @@ public class LearningSwitchManagerMultiImpl implements DataTreeChangeListenerReg
         FlowCommitWrapper dataStoreAccessor = new FlowCommitWrapperImpl(data);
 
         PacketInDispatcherImpl packetInDispatcher = new PacketInDispatcherImpl();
-        MultipleLearningSwitchHandlerFacadeImpl learningSwitchHandler = new MultipleLearningSwitchHandlerFacadeImpl();
-        learningSwitchHandler.setRegistrationPublisher(this);
-        learningSwitchHandler.setDataStoreAccessor(dataStoreAccessor);
-        learningSwitchHandler.setPacketProcessingService(packetProcessingService);
-        learningSwitchHandler.setPacketInDispatcher(packetInDispatcher);
+        MultipleLearningSwitchHandlerFacadeImpl learningSwitchHandler = new MultipleLearningSwitchHandlerFacadeImpl(
+                dataStoreAccessor, packetProcessingService, packetInDispatcher);
         packetInRegistration = notificationService.registerNotificationListener(packetInDispatcher);
 
         WakeupOnNode wakeupListener = new WakeupOnNode();
index 88ee493811b5115dc3851d7c3f2a229ecbfa0059..450df3a9ac6f3fca12e0562b5c06e3bb1a358690 100644 (file)
@@ -8,7 +8,8 @@
 
 package org.opendaylight.openflowplugin.learningswitch.multi;
 
-import org.opendaylight.openflowplugin.learningswitch.DataTreeChangeListenerRegistrationHolder;
+import java.util.Objects;
+import javax.annotation.Nonnull;
 import org.opendaylight.openflowplugin.learningswitch.FlowCommitWrapper;
 import org.opendaylight.openflowplugin.learningswitch.InstanceIdentifierUtils;
 import org.opendaylight.openflowplugin.learningswitch.LearningSwitchHandler;
@@ -23,9 +24,18 @@ import org.slf4j.LoggerFactory;
 public class MultipleLearningSwitchHandlerFacadeImpl implements LearningSwitchHandler {
 
     private static final Logger LOG = LoggerFactory.getLogger(MultipleLearningSwitchHandlerFacadeImpl.class);
-    private FlowCommitWrapper dataStoreAccessor;
-    private PacketProcessingService packetProcessingService;
-    private PacketInDispatcherImpl packetInDispatcher;
+    private final FlowCommitWrapper dataStoreAccessor;
+    private final PacketProcessingService packetProcessingService;
+    private final PacketInDispatcherImpl packetInDispatcher;
+
+    public MultipleLearningSwitchHandlerFacadeImpl(@Nonnull FlowCommitWrapper dataStoreAccessor,
+            @Nonnull PacketProcessingService packetProcessingService,
+            @Nonnull PacketInDispatcherImpl packetInDispatcher) {
+        this.dataStoreAccessor = Objects.requireNonNull(dataStoreAccessor);
+        this.packetProcessingService = Objects.requireNonNull(packetProcessingService);
+        this.packetInDispatcher = Objects.requireNonNull(packetInDispatcher);
+    }
+
 
     @Override
     public synchronized void onSwitchAppeared(InstanceIdentifier<Table> appearedTablePath) {
@@ -43,12 +53,8 @@ public class MultipleLearningSwitchHandlerFacadeImpl implements LearningSwitchHa
          */
         if (!packetInDispatcher.getHandlerMapping().containsKey(nodePath)) {
             // delegate this node (owning appearedTable) to SimpleLearningSwitchHandler
-            LearningSwitchHandlerSimpleImpl simpleLearningSwitch = new LearningSwitchHandlerSimpleImpl();
-            /**
-             * We set runtime dependencies
-             */
-            simpleLearningSwitch.setDataStoreAccessor(dataStoreAccessor);
-            simpleLearningSwitch.setPacketProcessingService(packetProcessingService);
+            LearningSwitchHandlerSimpleImpl simpleLearningSwitch = new LearningSwitchHandlerSimpleImpl(
+                    dataStoreAccessor, packetProcessingService, null);
 
             /**
              * We propagate table event to newly instantiated instance of learning switch
@@ -60,25 +66,4 @@ public class MultipleLearningSwitchHandlerFacadeImpl implements LearningSwitchHa
             packetInDispatcher.getHandlerMapping().put(nodePath, simpleLearningSwitch);
         }
     }
-
-    @Override
-    public void setRegistrationPublisher(
-            DataTreeChangeListenerRegistrationHolder registrationPublisher) {
-        //NOOP
-    }
-
-    @Override
-    public void setDataStoreAccessor(FlowCommitWrapper dataStoreAccessor) {
-        this.dataStoreAccessor = dataStoreAccessor;
-    }
-
-    @Override
-    public void setPacketProcessingService(
-            PacketProcessingService packetProcessingService) {
-        this.packetProcessingService = packetProcessingService;
-    }
-
-    public void setPacketInDispatcher(PacketInDispatcherImpl packetInDispatcher) {
-        this.packetInDispatcher = packetInDispatcher;
-    }
 }
index f615e734f0d30ff1347042e65651b1d168fe7477..4ef79f62d1d149267ce4e7ac36c2fd5266617f57 100644 (file)
@@ -10,28 +10,12 @@ package org.opendaylight.openflowplugin.openflow.samples.consumer;
 import org.opendaylight.controller.sal.binding.api.AbstractBindingAwareConsumer;
 import org.opendaylight.controller.sal.binding.api.BindingAwareBroker.ConsumerContext;
 import org.opendaylight.yang.gen.v1.urn.opendaylight.flow.service.rev130819.SalFlowService;
-import org.osgi.framework.BundleContext;
 
 public class Activator extends AbstractBindingAwareConsumer {
 
-    SimpleDropFirewall service;
-    private SalFlowService flowService;
-    private SimpleDropFirewallCli cliAdapter;
-
-    @Override
-    protected void startImpl(BundleContext context) {
-        service = new SimpleDropFirewall();
-
-        cliAdapter.setService(service);
-    }
-
     @Override
     public void onSessionInitialized(ConsumerContext session) {
-        service.setContext(session);
-        flowService = session.getRpcService(SalFlowService.class);
-
-        service.setFlowService(flowService);
-        service.start();
+        SalFlowService flowService = session.getRpcService(SalFlowService.class);
+        new SimpleDropFirewall(flowService).start();
     }
-
 }
index 6c6986aa72e2dcf44e37e82d6005d6f512d82a4c..1ae2478c7a50f2cee581d76996acc1c542dbdb2e 100644 (file)
@@ -7,44 +7,24 @@
  */
 package org.opendaylight.openflowplugin.openflow.samples.consumer;
 
-import java.util.Collection;
 import java.util.concurrent.ExecutionException;
 import java.util.concurrent.Future;
 import java.util.concurrent.TimeUnit;
 import java.util.concurrent.TimeoutException;
-import javax.annotation.Nonnull;
-import org.opendaylight.controller.md.sal.binding.api.DataObjectModification.ModificationType;
-import org.opendaylight.controller.md.sal.binding.api.DataTreeChangeListener;
-import org.opendaylight.controller.md.sal.binding.api.DataTreeModification;
-import org.opendaylight.controller.sal.binding.api.BindingAwareBroker.ConsumerContext;
-import org.opendaylight.controller.sal.binding.api.NotificationService;
 import org.opendaylight.yang.gen.v1.urn.opendaylight.flow.service.rev130819.AddFlowInput;
 import org.opendaylight.yang.gen.v1.urn.opendaylight.flow.service.rev130819.AddFlowOutput;
 import org.opendaylight.yang.gen.v1.urn.opendaylight.flow.service.rev130819.SalFlowService;
-import org.opendaylight.yang.gen.v1.urn.opendaylight.inventory.rev130819.NodeConnectorRemoved;
-import org.opendaylight.yang.gen.v1.urn.opendaylight.inventory.rev130819.NodeConnectorUpdated;
-import org.opendaylight.yang.gen.v1.urn.opendaylight.inventory.rev130819.NodeRemoved;
-import org.opendaylight.yang.gen.v1.urn.opendaylight.inventory.rev130819.NodeUpdated;
-import org.opendaylight.yang.gen.v1.urn.opendaylight.inventory.rev130819.OpendaylightInventoryListener;
-import org.opendaylight.yang.gen.v1.urn.opendaylight.inventory.rev130819.nodes.Node;
 import org.opendaylight.yangtools.yang.common.RpcResult;
 
 public class SimpleDropFirewall {
 
-    private ConsumerContext context;
-    private SalFlowService flowService;
-    private DataTreeChangeListener listener = new NodeListener();
+    private final SalFlowService flowService;
 
-    public void setContext(ConsumerContext session) {
-        this.context = session;
-    }
-
-    public void setFlowService(SalFlowService flowService) {
+    public SimpleDropFirewall(SalFlowService flowService) {
         this.flowService = flowService;
     }
 
     public void start() {
-        NotificationService notificationService = context.getSALService(NotificationService.class);
     }
 
     public boolean addFlow(AddFlowInput flow) throws InterruptedException,
@@ -53,42 +33,4 @@ public class SimpleDropFirewall {
 
         return result.get(5, TimeUnit.SECONDS).isSuccessful();
     }
-
-    private class NodeListener implements DataTreeChangeListener<Node> {
-        @Override
-        public void onDataTreeChanged(@Nonnull Collection<DataTreeModification<Node>> modifications) {
-            for (DataTreeModification modification : modifications) {
-                if (modification.getRootNode().getModificationType() == ModificationType.SUBTREE_MODIFIED) {
-                    // node updated
-                }
-            }
-        }
-    }
-
-    private class InventoryListener implements OpendaylightInventoryListener {
-
-        @Override
-        public void onNodeConnectorRemoved(NodeConnectorRemoved notification) {
-            // TODO Auto-generated method stub
-
-        }
-
-        @Override
-        public void onNodeConnectorUpdated(NodeConnectorUpdated notification) {
-
-        }
-
-        @Override
-        public void onNodeRemoved(NodeRemoved notification) {
-            // TODO Auto-generated method stub
-
-        }
-
-        @Override
-        public void onNodeUpdated(NodeUpdated notification) {
-            // TODO Auto-generated method stub
-
-        }
-
-    }
 }
index d0137d7c965c28b3005307385373adab4d443f87..a9780f4a4bd980c2bc69cdbf733a1a17504fcef9 100644 (file)
@@ -28,12 +28,6 @@ import org.opendaylight.yang.gen.v1.urn.opendaylight.model.match.types.rev131026
 
 public class SimpleDropFirewallCli {
 
-    private SimpleDropFirewall service;
-
-    public void setService(final SimpleDropFirewall service) {
-        this.service = service;
-    }
-
     /**
      * Form of input is: node name node-connector number source ip-address destinatinon ip-address.
      *
index 7787924441035bdfe9421d0c01e57697e606da2d..14d4538aa90a7ba895a50082f95f1dde2b92ebda 100644 (file)
@@ -28,7 +28,7 @@ public class ScenarioHandler extends Thread {
     private final BlockingQueue<byte[]> ofMsg;
     private ChannelHandlerContext ctx;
     private int eventNumber;
-    private boolean scenarioFinished = false;
+    private volatile boolean scenarioFinished = false;
     private int freeze = 2;
     private long sleepBetweenTries = 100L;
     private boolean finishedOK = true;
@@ -94,7 +94,7 @@ public class ScenarioHandler extends Thread {
         LOG.debug("Scenario finished");
         synchronized (this) {
             scenarioFinished = true;
-            this.notify();
+            this.notifyAll();
         }
     }