From 4892efadc4ef9a61831828bddfe0e939b4334779 Mon Sep 17 00:00:00 2001 From: Michal Polkorab Date: Thu, 17 Jul 2014 12:21:38 +0200 Subject: [PATCH 1/1] Bug 1373 - Fixed NullPointerException on incorrect input - now throws IllegalArgumentException. This is just logged and pushed into Future, the Future delegates the information further to upper ODL layers Change-Id: I64ac1e4904e8837ab5b26572235bdaf5fac984da Signed-off-by: Michal Polkorab --- .../impl/connection/ChannelOutboundQueue.java | 2 +- .../connection/MessageListenerWrapper.java | 50 +++++++++++++++++++ .../protocol/impl/core/OFEncoder.java | 14 +++--- .../DeserializerRegistryImpl.java | 6 +-- .../serialization/SerializerRegistryImpl.java | 9 ++-- .../protocol/impl/core/OFEncoderTest.java | 20 ++++++-- 6 files changed, 84 insertions(+), 17 deletions(-) create mode 100644 openflow-protocol-impl/src/main/java/org/opendaylight/openflowjava/protocol/impl/connection/MessageListenerWrapper.java diff --git a/openflow-protocol-impl/src/main/java/org/opendaylight/openflowjava/protocol/impl/connection/ChannelOutboundQueue.java b/openflow-protocol-impl/src/main/java/org/opendaylight/openflowjava/protocol/impl/connection/ChannelOutboundQueue.java index 765a9794..40af2791 100644 --- a/openflow-protocol-impl/src/main/java/org/opendaylight/openflowjava/protocol/impl/connection/ChannelOutboundQueue.java +++ b/openflow-protocol-impl/src/main/java/org/opendaylight/openflowjava/protocol/impl/connection/ChannelOutboundQueue.java @@ -180,8 +180,8 @@ final class ChannelOutboundQueue extends ChannelInboundHandlerAdapter { break; } - final ChannelFuture p = channel.write(h.takeMessage()); final GenericFutureListener> l = h.takeListener(); + final ChannelFuture p = channel.write(new MessageListenerWrapper(h.takeMessage(), l)); if (l != null) { p.addListener(l); } diff --git a/openflow-protocol-impl/src/main/java/org/opendaylight/openflowjava/protocol/impl/connection/MessageListenerWrapper.java b/openflow-protocol-impl/src/main/java/org/opendaylight/openflowjava/protocol/impl/connection/MessageListenerWrapper.java new file mode 100644 index 00000000..68311d23 --- /dev/null +++ b/openflow-protocol-impl/src/main/java/org/opendaylight/openflowjava/protocol/impl/connection/MessageListenerWrapper.java @@ -0,0 +1,50 @@ +/* + * Copyright (c) 2014 Pantheon Technologies s.r.o. and others. All rights reserved. + * + * This program and the accompanying materials are made available under the + * terms of the Eclipse Public License v1.0 which accompanies this distribution, + * and is available at http://www.eclipse.org/legal/epl-v10.html + */ + +package org.opendaylight.openflowjava.protocol.impl.connection; + +import io.netty.util.concurrent.Future; +import io.netty.util.concurrent.GenericFutureListener; + +import org.opendaylight.yang.gen.v1.urn.opendaylight.openflow.protocol.rev130731.OfHeader; + +/** + * Wraps outgoing message and includes listener attached to this message. This object + * is sent to OFEncoder. When OFEncoder fails to serialize the message, + * listener is filled with exception. The exception is then delegated to upper ODL layers. + * @author michal.polkorab + */ +public class MessageListenerWrapper { + + private OfHeader msg; + private GenericFutureListener> listener; + + /** + * @param msg outgoing message + * @param listener listener attached to channel.write(msg) Future + */ + public MessageListenerWrapper(Object msg, GenericFutureListener> listener) { + this.msg = (OfHeader) msg; + this.listener = listener; + } + + /** + * @return outgoing message (downstream) + */ + public OfHeader getMsg() { + return msg; + } + + + /** + * @return listener listening on message sending success / failure + */ + public GenericFutureListener> getListener() { + return listener; + } +} \ No newline at end of file diff --git a/openflow-protocol-impl/src/main/java/org/opendaylight/openflowjava/protocol/impl/core/OFEncoder.java b/openflow-protocol-impl/src/main/java/org/opendaylight/openflowjava/protocol/impl/core/OFEncoder.java index c93f1fcf..6080003f 100644 --- a/openflow-protocol-impl/src/main/java/org/opendaylight/openflowjava/protocol/impl/core/OFEncoder.java +++ b/openflow-protocol-impl/src/main/java/org/opendaylight/openflowjava/protocol/impl/core/OFEncoder.java @@ -11,9 +11,10 @@ package org.opendaylight.openflowjava.protocol.impl.core; import io.netty.buffer.ByteBuf; import io.netty.channel.ChannelHandlerContext; import io.netty.handler.codec.MessageToByteEncoder; +import io.netty.util.concurrent.Future; +import org.opendaylight.openflowjava.protocol.impl.connection.MessageListenerWrapper; import org.opendaylight.openflowjava.protocol.impl.serialization.SerializationFactory; -import org.opendaylight.yang.gen.v1.urn.opendaylight.openflow.protocol.rev130731.OfHeader; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -22,7 +23,7 @@ import org.slf4j.LoggerFactory; * @author michal.polkorab * @author timotej.kubas */ -public class OFEncoder extends MessageToByteEncoder { +public class OFEncoder extends MessageToByteEncoder { private static final Logger LOGGER = LoggerFactory.getLogger(OFEncoder.class); private SerializationFactory serializationFactory; @@ -33,14 +34,15 @@ public class OFEncoder extends MessageToByteEncoder { } @Override - protected void encode(ChannelHandlerContext ctx, OfHeader msg, ByteBuf out) + protected void encode(ChannelHandlerContext ctx, MessageListenerWrapper wrapper, ByteBuf out) throws Exception { LOGGER.trace("Encoding"); try { - serializationFactory.messageToBuffer(msg.getVersion(), out, msg); + serializationFactory.messageToBuffer(wrapper.getMsg().getVersion(), out, wrapper.getMsg()); } catch(Exception e) { - LOGGER.error("Message serialization failed"); - LOGGER.error(e.getMessage(), e); + LOGGER.warn("Message serialization failed: {}", e.getMessage()); + Future newFailedFuture = ctx.newFailedFuture(e); + wrapper.getListener().operationComplete(newFailedFuture); out.clear(); return; } diff --git a/openflow-protocol-impl/src/main/java/org/opendaylight/openflowjava/protocol/impl/deserialization/DeserializerRegistryImpl.java b/openflow-protocol-impl/src/main/java/org/opendaylight/openflowjava/protocol/impl/deserialization/DeserializerRegistryImpl.java index f78dbb85..a1efd1dc 100644 --- a/openflow-protocol-impl/src/main/java/org/opendaylight/openflowjava/protocol/impl/deserialization/DeserializerRegistryImpl.java +++ b/openflow-protocol-impl/src/main/java/org/opendaylight/openflowjava/protocol/impl/deserialization/DeserializerRegistryImpl.java @@ -59,7 +59,7 @@ public class DeserializerRegistryImpl implements DeserializerRegistry { OFGeneralDeserializer deserializer = registry.get(key); if (deserializer == null) { throw new NullPointerException("Deserializer for key: " + key.toString() - + " was not found"); + + " was not found - please verify that all needed deserializers ale loaded correctly"); } return (DESERIALIZER_TYPE) deserializer; } @@ -68,7 +68,7 @@ public class DeserializerRegistryImpl implements DeserializerRegistry { public void registerDeserializer(MessageCodeKey key, OFGeneralDeserializer deserializer) { if ((key == null) || (deserializer == null)) { - throw new NullPointerException("MessageCodeKey or Deserializer is null"); + throw new IllegalArgumentException("MessageCodeKey or Deserializer is null"); } if (deserializer instanceof DeserializerRegistryInjector) { ((DeserializerRegistryInjector) deserializer).injectDeserializerRegistry(this); @@ -79,7 +79,7 @@ public class DeserializerRegistryImpl implements DeserializerRegistry { @Override public boolean unregisterDeserializer(MessageCodeKey key) { if (key == null) { - throw new NullPointerException("MessageCodeKey is null"); + throw new IllegalArgumentException("MessageCodeKey is null"); } OFGeneralDeserializer deserializer = registry.remove(key); if (deserializer == null) { diff --git a/openflow-protocol-impl/src/main/java/org/opendaylight/openflowjava/protocol/impl/serialization/SerializerRegistryImpl.java b/openflow-protocol-impl/src/main/java/org/opendaylight/openflowjava/protocol/impl/serialization/SerializerRegistryImpl.java index 47b2d4a5..e8a02bd7 100644 --- a/openflow-protocol-impl/src/main/java/org/opendaylight/openflowjava/protocol/impl/serialization/SerializerRegistryImpl.java +++ b/openflow-protocol-impl/src/main/java/org/opendaylight/openflowjava/protocol/impl/serialization/SerializerRegistryImpl.java @@ -61,8 +61,9 @@ public class SerializerRegistryImpl implements SerializerRegistry { MessageTypeKey msgTypeKey) { OFGeneralSerializer serializer = registry.get(msgTypeKey); if (serializer == null) { - throw new NullPointerException("Serializer for key: " + msgTypeKey.toString() - + " was not found"); + throw new IllegalArgumentException("Serializer for key: {}" + msgTypeKey.toString() + + " was not found - please verify that you are using correct message" + + " combination (e.g. OF v1.0 message to OF v1.0 device)"); } return (SERIALIZER_TYPE) serializer; } @@ -71,7 +72,7 @@ public class SerializerRegistryImpl implements SerializerRegistry { public void registerSerializer( MessageTypeKey msgTypeKey, OFGeneralSerializer serializer) { if ((msgTypeKey == null) || (serializer == null)) { - throw new NullPointerException("MessageTypeKey or Serializer is null"); + throw new IllegalArgumentException("MessageTypeKey or Serializer is null"); } if (serializer instanceof SerializerRegistryInjector) { ((SerializerRegistryInjector) serializer).injectSerializerRegistry(this); @@ -82,7 +83,7 @@ public class SerializerRegistryImpl implements SerializerRegistry { @Override public boolean unregisterSerializer(MessageTypeKey msgTypeKey) { if (msgTypeKey == null) { - throw new NullPointerException("MessageTypeKey is null"); + throw new IllegalArgumentException("MessageTypeKey is null"); } OFGeneralSerializer serializer = registry.remove(msgTypeKey); if (serializer == null) { diff --git a/openflow-protocol-impl/src/test/java/org/opendaylight/openflowjava/protocol/impl/core/OFEncoderTest.java b/openflow-protocol-impl/src/test/java/org/opendaylight/openflowjava/protocol/impl/core/OFEncoderTest.java index 11d2098a..9cb7dca0 100644 --- a/openflow-protocol-impl/src/test/java/org/opendaylight/openflowjava/protocol/impl/core/OFEncoderTest.java +++ b/openflow-protocol-impl/src/test/java/org/opendaylight/openflowjava/protocol/impl/core/OFEncoderTest.java @@ -16,12 +16,16 @@ import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; import io.netty.buffer.ByteBuf; import io.netty.channel.ChannelHandlerContext; +import io.netty.util.concurrent.Future; +import io.netty.util.concurrent.GenericFutureListener; import org.junit.Assert; import org.junit.Before; import org.junit.Test; import org.mockito.Mock; import org.mockito.MockitoAnnotations; +import org.opendaylight.openflowjava.protocol.api.util.EncodeConstants; +import org.opendaylight.openflowjava.protocol.impl.connection.MessageListenerWrapper; import org.opendaylight.openflowjava.protocol.impl.serialization.SerializationFactory; import org.opendaylight.yang.gen.v1.urn.opendaylight.openflow.protocol.rev130731.OfHeader; import org.opendaylight.yangtools.yang.binding.DataObject; @@ -34,8 +38,11 @@ public class OFEncoderTest { @Mock ChannelHandlerContext mockChHndlrCtx ; @Mock SerializationFactory mockSerializationFactory ; + @Mock MessageListenerWrapper wrapper; @Mock OfHeader mockMsg ; @Mock ByteBuf mockOut ; + @Mock Future future; + @Mock GenericFutureListener> listener; OFEncoder ofEncoder = new OFEncoder() ; @@ -55,8 +62,10 @@ public class OFEncoderTest { @Test public void testEncodeSuccess() { when(mockOut.readableBytes()).thenReturn(1); + when(wrapper.getMsg()).thenReturn(mockMsg); + when(wrapper.getMsg().getVersion()).thenReturn((short) EncodeConstants.OF13_VERSION_ID); try { - ofEncoder.encode(mockChHndlrCtx, mockMsg, mockOut); + ofEncoder.encode(mockChHndlrCtx, wrapper, mockOut); } catch (Exception e) { Assert.fail(); } @@ -70,9 +79,12 @@ public class OFEncoderTest { */ @Test public void testEncodeSerializationException() { + when(wrapper.getMsg()).thenReturn(mockMsg); + when(wrapper.getListener()).thenReturn(listener); + when(wrapper.getMsg().getVersion()).thenReturn((short) EncodeConstants.OF13_VERSION_ID); doThrow(new IllegalArgumentException()).when(mockSerializationFactory).messageToBuffer(anyShort(),any(ByteBuf.class), any(DataObject.class)); try { - ofEncoder.encode(mockChHndlrCtx, mockMsg, mockOut); + ofEncoder.encode(mockChHndlrCtx, wrapper, mockOut); } catch (Exception e) { Assert.fail(); } @@ -87,8 +99,10 @@ public class OFEncoderTest { @Test public void testEncodeSerializesNoBytes() { when(mockOut.readableBytes()).thenReturn(0); + when(wrapper.getMsg()).thenReturn(mockMsg); + when(wrapper.getMsg().getVersion()).thenReturn((short) EncodeConstants.OF13_VERSION_ID); try { - ofEncoder.encode(mockChHndlrCtx, mockMsg, mockOut); + ofEncoder.encode(mockChHndlrCtx, wrapper, mockOut); } catch (Exception e) { Assert.fail(); } -- 2.36.6