Refactor DHCP server with StateMachine. am: 1f20cb6617 am: db59c2f0be (b963096d) · Commits · e / os / android_packages_modules_NetworkStack

src/android/net/dhcp/DhcpServer.java

+183 −104

Original line number	Diff line number	Diff line
		@@ -20,6 +20,9 @@ import static android.net.dhcp.DhcpPacket.DHCP_CLIENT;
		import static android.net.dhcp.DhcpPacket.DHCP_HOST_NAME;
		import static android.net.dhcp.DhcpPacket.DHCP_SERVER;
		import static android.net.dhcp.DhcpPacket.ENCAP_BOOTP;
		import static android.net.dhcp.IDhcpServer.STATUS_INVALID_ARGUMENT;
		import static android.net.dhcp.IDhcpServer.STATUS_SUCCESS;
		import static android.net.dhcp.IDhcpServer.STATUS_UNKNOWN_ERROR;
		import static android.net.shared.Inet4AddressUtils.getBroadcastAddress;
		import static android.net.shared.Inet4AddressUtils.getPrefixMaskAsInet4Address;
		import static android.net.util.NetworkStackUtils.DHCP_RAPID_COMMIT_VERSION;
		@@ -48,8 +51,6 @@ import android.net.util.NetworkStackUtils;
		import android.net.util.SharedLog;
		import android.net.util.SocketUtils;
		import android.os.Handler;
		import android.os.HandlerThread;
		import android.os.Looper;
		import android.os.Message;
		import android.os.RemoteException;
		import android.os.SystemClock;
		@@ -63,6 +64,8 @@ import androidx.annotation.Nullable;
		import androidx.annotation.VisibleForTesting;

		import com.android.internal.util.HexDump;
		import com.android.internal.util.State;
		import com.android.internal.util.StateMachine;

		import java.io.FileDescriptor;
		import java.io.IOException;
		@@ -78,12 +81,12 @@ import java.util.ArrayList;
		* authoritative for all leases on the subnet, which means that DHCP requests for unknown leases of
		* unknown hosts receive a reply instead of being ignored.
		*
		* <p>The server is single-threaded (including send/receive operations): all internal operations are
		* done on the provided {@link Looper}. Public methods are thread-safe and will schedule operations
		* on the looper asynchronously.
		* <p>The server relies on StateMachine's handler (including send/receive operations): all internal
		* operations are done in StateMachine's looper. Public methods are thread-safe and will schedule
		* operations on that looper asynchronously.
		* @hide
		*/
		public class DhcpServer extends IDhcpServer.Stub {
		public class DhcpServer extends StateMachine {
		private static final String REPO_TAG = "Repository";

		// Lease time to transmit to client instead of a negative time in case a lease expired before
		@@ -93,12 +96,11 @@ public class DhcpServer extends IDhcpServer.Stub {
		private static final int CMD_START_DHCP_SERVER = 1;
		private static final int CMD_STOP_DHCP_SERVER = 2;
		private static final int CMD_UPDATE_PARAMS = 3;
		private static final int CMD_SUSPEND_DHCP_SERVER = 4;

		@NonNull
		private final Context mContext;
		@NonNull
		private final HandlerThread mHandlerThread;
		@NonNull
		private final String mIfName;
		@NonNull
		private final DhcpLeaseRepository mLeaseRepo;
		@@ -108,19 +110,22 @@ public class DhcpServer extends IDhcpServer.Stub {
		private final Dependencies mDeps;
		@NonNull
		private final Clock mClock;
		@NonNull
		private DhcpServingParams mServingParams;

		@Nullable
		private volatile ServerHandler mHandler;

		private final boolean mDhcpRapidCommitEnabled;

		// Accessed only on the handler thread
		@Nullable
		private DhcpPacketListener mPacketListener;
		@Nullable
		private FileDescriptor mSocket;
		@NonNull
		private DhcpServingParams mServingParams;
		@Nullable
		private IDhcpEventCallbacks mEventCallbacks;

		private final boolean mDhcpRapidCommitEnabled;

		// States.
		private final StoppedState mStoppedState = new StoppedState();
		private final StartedState mStartedState = new StartedState();
		private final RunningState mRunningState = new RunningState();

		/**
		* Clock to be used by DhcpServer to track time for lease expiration.
		@@ -164,7 +169,7 @@ public class DhcpServer extends IDhcpServer.Stub {
		/**
		* Create a packet listener that will send packets to be processed.
		*/
		DhcpPacketListener makePacketListener();
		DhcpPacketListener makePacketListener(@NonNull Handler handler);

		/**
		* Create a clock that the server will use to track time.
		@@ -178,12 +183,6 @@ public class DhcpServer extends IDhcpServer.Stub {
		void addArpEntry(@NonNull Inet4Address ipv4Addr, @NonNull MacAddress ethAddr,
		@NonNull String ifname, @NonNull FileDescriptor fd) throws IOException;

		/**
		* Verify that the caller is allowed to call public methods on DhcpServer.
		* @throws SecurityException The caller is not allowed to call public methods on DhcpServer.
		*/
		void checkCaller() throws SecurityException;

		/**
		* Check whether or not one specific experimental feature for connectivity namespace is
		* enabled.
		@@ -210,8 +209,8 @@ public class DhcpServer extends IDhcpServer.Stub {
		}

		@Override
		public DhcpPacketListener makePacketListener() {
		return new PacketListener();
		public DhcpPacketListener makePacketListener(@NonNull Handler handler) {
		return new PacketListener(handler);
		}

		@Override
		@@ -225,11 +224,6 @@ public class DhcpServer extends IDhcpServer.Stub {
		NetworkStackUtils.addArpEntry(ipv4Addr, ethAddr, ifname, fd);
		}

		@Override
		public void checkCaller() {
		enforceNetworkStackCallingPermission();
		}

		@Override
		public boolean isFeatureEnabled(@NonNull Context context, @NonNull String name) {
		return NetworkStackUtils.isFeatureEnabled(context, NAMESPACE_CONNECTIVITY, name);
		@@ -244,19 +238,18 @@ public class DhcpServer extends IDhcpServer.Stub {

		public DhcpServer(@NonNull Context context, @NonNull String ifName,
		@NonNull DhcpServingParams params, @NonNull SharedLog log) {
		this(context, new HandlerThread(DhcpServer.class.getSimpleName() + "." + ifName),
		ifName, params, log, null);
		this(context, ifName, params, log, null);
		}

		@VisibleForTesting
		DhcpServer(@NonNull Context context, @NonNull HandlerThread handlerThread,
		@NonNull String ifName, @NonNull DhcpServingParams params, @NonNull SharedLog log,
		@Nullable Dependencies deps) {
		DhcpServer(@NonNull Context context, @NonNull String ifName, @NonNull DhcpServingParams params,
		@NonNull SharedLog log, @Nullable Dependencies deps) {
		super(DhcpServer.class.getSimpleName() + "." + ifName);

		if (deps == null) {
		deps = new DependenciesImpl();
		}
		mContext = context;
		mHandlerThread = handlerThread;
		mIfName = ifName;
		mServingParams = params;
		mLog = log;
		@@ -264,6 +257,61 @@ public class DhcpServer extends IDhcpServer.Stub {
		mClock = deps.makeClock();
		mLeaseRepo = deps.makeLeaseRepository(mServingParams, mLog, mClock);
		mDhcpRapidCommitEnabled = deps.isFeatureEnabled(context, DHCP_RAPID_COMMIT_VERSION);

		// CHECKSTYLE:OFF IndentationCheck
		addState(mStoppedState);
		addState(mStartedState);
		addState(mRunningState, mStartedState);
		// CHECKSTYLE:ON IndentationCheck

		setInitialState(mStoppedState);

		super.start();
		}

		/**
		* Make a IDhcpServer connector to communicate with this DhcpServer.
		*/
		public IDhcpServer makeConnector() {
		return new DhcpServerConnector();
		}

		private class DhcpServerConnector extends IDhcpServer.Stub {
		@Override
		public void start(@Nullable INetworkStackStatusCallback cb) {
		enforceNetworkStackCallingPermission();
		DhcpServer.this.start(cb);
		}

		@Override
		public void startWithCallbacks(@Nullable INetworkStackStatusCallback statusCb,
		@Nullable IDhcpEventCallbacks eventCb) {
		enforceNetworkStackCallingPermission();
		DhcpServer.this.start(statusCb, eventCb);
		}

		@Override
		public void updateParams(@Nullable DhcpServingParamsParcel params,
		@Nullable INetworkStackStatusCallback cb) {
		enforceNetworkStackCallingPermission();
		DhcpServer.this.updateParams(params, cb);
		}

		@Override
		public void stop(@Nullable INetworkStackStatusCallback cb) {
		enforceNetworkStackCallingPermission();
		DhcpServer.this.stop(cb);
		}

		@Override
		public int getInterfaceVersion() {
		return this.VERSION;
		}

		@Override
		public String getInterfaceHash() {
		return this.HASH;
		}
		}

		/**
		@@ -272,9 +320,8 @@ public class DhcpServer extends IDhcpServer.Stub {
		* <p>It is not legal to call this method more than once; in particular the server cannot be
		* restarted after being stopped.
		*/
		@Override
		public void start(@Nullable INetworkStackStatusCallback cb) {
		startWithCallbacks(cb, null);
		void start(@Nullable INetworkStackStatusCallback cb) {
		start(cb, null);
		}

		/**
		@@ -283,12 +330,8 @@ public class DhcpServer extends IDhcpServer.Stub {
		* <p>It is not legal to call this method more than once; in particular the server cannot be
		* restarted after being stopped.
		*/
		@Override
		public void startWithCallbacks(@Nullable INetworkStackStatusCallback statusCb,
		void start(@Nullable INetworkStackStatusCallback statusCb,
		@Nullable IDhcpEventCallbacks eventCb) {
		mDeps.checkCaller();
		mHandlerThread.start();
		mHandler = new ServerHandler(mHandlerThread.getLooper());
		sendMessage(CMD_START_DHCP_SERVER, new Pair<>(statusCb, eventCb));
		}

		@@ -296,19 +339,15 @@ public class DhcpServer extends IDhcpServer.Stub {
		* Update serving parameters. All subsequently received requests will be handled with the new
		* parameters, and current leases that are incompatible with the new parameters are dropped.
		*/
		@Override
		public void updateParams(@Nullable DhcpServingParamsParcel params,
		@Nullable INetworkStackStatusCallback cb) throws RemoteException {
		mDeps.checkCaller();
		void updateParams(@Nullable DhcpServingParamsParcel params,
		@Nullable INetworkStackStatusCallback cb) {
		final DhcpServingParams parsedParams;
		try {
		// throws InvalidParameterException with null params
		parsedParams = DhcpServingParams.fromParcelableObject(params);
		} catch (DhcpServingParams.InvalidParameterException e) {
		mLog.e("Invalid parameters sent to DhcpServer", e);
		if (cb != null) {
		cb.onStatusAvailable(STATUS_INVALID_ARGUMENT);
		}
		maybeNotifyStatus(cb, STATUS_INVALID_ARGUMENT);
		return;
		}
		sendMessage(CMD_UPDATE_PARAMS, new Pair<>(parsedParams, cb));
		@@ -320,28 +359,74 @@ public class DhcpServer extends IDhcpServer.Stub {
		* <p>As the server is stopped asynchronously, some packets may still be processed shortly after
		* calling this method.
		*/
		@Override
		public void stop(@Nullable INetworkStackStatusCallback cb) {
		mDeps.checkCaller();
		void stop(@Nullable INetworkStackStatusCallback cb) {
		sendMessage(CMD_STOP_DHCP_SERVER, cb);
		}

		private void sendMessage(int what, @Nullable Object obj) {
		if (mHandler == null) {
		mLog.e("Attempting to send a command to stopped DhcpServer: " + what);
		private void maybeNotifyStatus(@Nullable INetworkStackStatusCallback cb, int statusCode) {
		if (cb == null) return;
		try {
		cb.onStatusAvailable(statusCode);
		} catch (RemoteException e) {
		mLog.e("Could not send status back to caller", e);
		}
		}

		class StoppedState extends State {
		private INetworkStackStatusCallback mOnStopCallback;

		@Override
		public void enter() {
		maybeNotifyStatus(mOnStopCallback, STATUS_SUCCESS);
		mOnStopCallback = null;
		}

		@Override
		public boolean processMessage(Message msg) {
		switch (msg.what) {
		case CMD_START_DHCP_SERVER:
		final Pair<INetworkStackStatusCallback, IDhcpEventCallbacks> obj =
		(Pair<INetworkStackStatusCallback, IDhcpEventCallbacks>) msg.obj;
		mStartedState.mOnStartCallback = obj.first;
		mEventCallbacks = obj.second;
		transitionTo(mRunningState);
		return HANDLED;
		default:
		return NOT_HANDLED;
		}
		}
		}

		class StartedState extends State {
		private INetworkStackStatusCallback mOnStartCallback;

		@Override
		public void enter() {
		if (mPacketListener != null) {
		mLog.e("Starting DHCP server more than once is not supported.");
		maybeNotifyStatus(mOnStartCallback, STATUS_UNKNOWN_ERROR);
		mOnStartCallback = null;
		return;
		}
		mHandler.sendMessage(mHandler.obtainMessage(what, obj));
		mPacketListener = mDeps.makePacketListener(getHandler());

		if (!mPacketListener.start()) {
		mLog.e("Fail to start DHCP Packet Listener, rollback to StoppedState");
		deferMessage(obtainMessage(CMD_STOP_DHCP_SERVER, null));
		maybeNotifyStatus(mOnStartCallback, STATUS_UNKNOWN_ERROR);
		mOnStartCallback = null;
		return;
		}

		private class ServerHandler extends Handler {
		ServerHandler(@NonNull Looper looper) {
		super(looper);
		if (mEventCallbacks != null) {
		mLeaseRepo.addLeaseCallbacks(mEventCallbacks);
		}
		maybeNotifyStatus(mOnStartCallback, STATUS_SUCCESS);
		mOnStartCallback = null;
		}

		@Override
		public void handleMessage(@NonNull Message msg) {
		final INetworkStackStatusCallback cb;
		public boolean processMessage(Message msg) {
		switch (msg.what) {
		case CMD_UPDATE_PARAMS:
		final Pair<DhcpServingParams, INetworkStackStatusCallback> pair =
		@@ -349,40 +434,45 @@ public class DhcpServer extends IDhcpServer.Stub {
		final DhcpServingParams params = pair.first;
		mServingParams = params;
		mLeaseRepo.updateParams(
		DhcpServingParams.makeIpPrefix(mServingParams.serverAddr),
		DhcpServingParams.makeIpPrefix(params.serverAddr),
		params.excludedAddrs,
		params.dhcpLeaseTimeSecs,
		params.dhcpLeaseTimeSecs * 1000,
		params.singleClientAddr);
		maybeNotifyStatus(pair.second, STATUS_SUCCESS);
		return HANDLED;

		cb = pair.second;
		break;
		case CMD_START_DHCP_SERVER:
		final Pair<INetworkStackStatusCallback, IDhcpEventCallbacks> obj =
		(Pair<INetworkStackStatusCallback, IDhcpEventCallbacks>) msg.obj;
		cb = obj.first;
		if (obj.second != null) {
		mLeaseRepo.addLeaseCallbacks(obj.second);
		}
		mPacketListener = mDeps.makePacketListener();
		mPacketListener.start();
		break;
		mLog.e("ALERT: START received in StartedState. Please fix caller.");
		return HANDLED;

		case CMD_STOP_DHCP_SERVER:
		if (mPacketListener != null) {
		mPacketListener.stop();
		mPacketListener = null;
		}
		mHandlerThread.quitSafely();
		cb = (INetworkStackStatusCallback) msg.obj;
		break;
		mStoppedState.mOnStopCallback = (INetworkStackStatusCallback) msg.obj;
		transitionTo(mStoppedState);
		return HANDLED;

		default:
		return;
		return NOT_HANDLED;
		}
		if (cb != null) {
		try {
		cb.onStatusAvailable(STATUS_SUCCESS);
		} catch (RemoteException e) {
		mLog.e("Could not send status back to caller", e);
		}

		@Override
		public void exit() {
		mPacketListener.stop();
		mLog.logf("DHCP Packet Listener stopped");
		}
		}

		class RunningState extends State {
		@Override
		public boolean processMessage(Message msg) {
		switch (msg.what) {
		case CMD_SUSPEND_DHCP_SERVER:
		// TODO: transition to the state which waits for IpServer to reconfigure the
		// new selected prefix.
		return HANDLED;
		default:
		// Fall through to StartedState.
		return NOT_HANDLED;
		}
		}
		}
		@@ -651,8 +741,8 @@ public class DhcpServer extends IDhcpServer.Stub {
		}

		private class PacketListener extends DhcpPacketListener {
		PacketListener() {
		super(mHandler);
		PacketListener(Handler handler) {
		super(handler);
		}

		@Override
		@@ -686,21 +776,10 @@ public class DhcpServer extends IDhcpServer.Stub {
		return mSocket;
		} catch (IOException \| ErrnoException e) {
		mLog.e("Error creating UDP socket", e);
		DhcpServer.this.stop(null);
		return null;
		} finally {
		TrafficStats.setThreadStatsTag(oldTag);
		}
		}
		}

		@Override
		public int getInterfaceVersion() {
		return this.VERSION;
		}

		@Override
		public String getInterfaceHash() {
		return this.HASH;
		}
		}

src/com/android/server/NetworkStackService.java

+1 −1

Original line number	Diff line number	Diff line
		@@ -356,7 +356,7 @@ public class NetworkStackService extends Service {
		cb.onDhcpServerCreated(STATUS_UNKNOWN_ERROR, null);
		return;
		}
		cb.onDhcpServerCreated(STATUS_SUCCESS, server);
		cb.onDhcpServerCreated(STATUS_SUCCESS, server.makeConnector());
		}

		@Override

tests/unit/src/android/net/dhcp/DhcpServerTest.java

+10 −19

Original line number	Diff line number	Diff line
		@@ -36,7 +36,6 @@ import static org.mockito.ArgumentMatchers.eq;
		import static org.mockito.ArgumentMatchers.isNull;
		import static org.mockito.Mockito.doNothing;
		import static org.mockito.Mockito.never;
		import static org.mockito.Mockito.spy;
		import static org.mockito.Mockito.times;
		import static org.mockito.Mockito.verify;
		import static org.mockito.Mockito.when;
		@@ -52,13 +51,12 @@ import android.net.dhcp.DhcpLeaseRepository.OutOfAddressesException;
		import android.net.dhcp.DhcpServer.Clock;
		import android.net.dhcp.DhcpServer.Dependencies;
		import android.net.util.SharedLog;
		import android.os.HandlerThread;
		import android.testing.AndroidTestingRunner;
		import android.testing.TestableLooper;
		import android.testing.TestableLooper.RunWithLooper;

		import androidx.test.filters.SmallTest;

		import com.android.testutils.HandlerUtilsKt;

		import org.junit.After;
		import org.junit.Before;
		import org.junit.Test;
		@@ -76,7 +74,6 @@ import java.util.Set;

		@RunWith(AndroidTestingRunner.class)
		@SmallTest
		@RunWithLooper
		public class DhcpServerTest {
		private static final String TEST_IFACE = "testiface";

		@@ -107,6 +104,7 @@ public class DhcpServerTest {
		private static final DhcpLease TEST_LEASE_WITH_HOSTNAME = new DhcpLease(null, TEST_CLIENT_MAC,
		TEST_CLIENT_ADDR, TEST_PREFIX_LENGTH, TEST_LEASE_EXPTIME_SECS * 1000L + TEST_CLOCK_TIME,
		TEST_HOSTNAME);
		private static final int TEST_TIMEOUT_MS = 10000;

		@NonNull @Mock
		private Context mContext;
		@@ -126,10 +124,6 @@ public class DhcpServerTest {
		@NonNull @Captor
		private ArgumentCaptor<Inet4Address> mResponseDstAddrCaptor;

		@NonNull
		private HandlerThread mHandlerThread;
		@NonNull
		private TestableLooper mLooper;
		@NonNull
		private DhcpServer mServer;

		@@ -167,7 +161,7 @@ public class DhcpServerTest {

		private void startServer() throws Exception {
		mServer.start(mAssertSuccessCallback);
		mLooper.processAllMessages();
		HandlerUtilsKt.waitForIdle(mServer.getHandler(), TEST_TIMEOUT_MS);
		}

		@Before
		@@ -176,16 +170,14 @@ public class DhcpServerTest {

		when(mDeps.makeLeaseRepository(any(), any(), any())).thenReturn(mRepository);
		when(mDeps.makeClock()).thenReturn(mClock);
		when(mDeps.makePacketListener()).thenReturn(mPacketListener);
		when(mDeps.makePacketListener(any())).thenReturn(mPacketListener);
		when(mDeps.isFeatureEnabled(eq(mContext), eq(DHCP_RAPID_COMMIT_VERSION))).thenReturn(true);
		doNothing().when(mDeps)
		.sendPacket(any(), mSentPacketCaptor.capture(), mResponseDstAddrCaptor.capture());
		when(mClock.elapsedRealtime()).thenReturn(TEST_CLOCK_TIME);
		when(mPacketListener.start()).thenReturn(true);

		mLooper = TestableLooper.get(this);
		mHandlerThread = spy(new HandlerThread("TestDhcpServer"));
		when(mHandlerThread.getLooper()).thenReturn(mLooper.getLooper());
		mServer = new DhcpServer(mContext, mHandlerThread, TEST_IFACE, makeServingParams(),
		mServer = new DhcpServer(mContext, TEST_IFACE, makeServingParams(),
		new SharedLog(DhcpServerTest.class.getSimpleName()), mDeps);
		}

		@@ -193,9 +185,8 @@ public class DhcpServerTest {
		public void tearDown() throws Exception {
		verify(mRepository, never()).addLeaseCallbacks(eq(null));
		mServer.stop(mAssertSuccessCallback);
		mLooper.processMessages(1);
		HandlerUtilsKt.waitForIdle(mServer.getHandler(), TEST_TIMEOUT_MS);
		verify(mPacketListener, times(1)).stop();
		verify(mHandlerThread, times(1)).quitSafely();
		}

		@Test
		@@ -207,8 +198,8 @@ public class DhcpServerTest {

		@Test
		public void testStartWithCallbacks() throws Exception {
		mServer.startWithCallbacks(mAssertSuccessCallback, mEventCallbacks);
		mLooper.processAllMessages();
		mServer.start(mAssertSuccessCallback, mEventCallbacks);
		HandlerUtilsKt.waitForIdle(mServer.getHandler(), TEST_TIMEOUT_MS);
		verify(mRepository).addLeaseCallbacks(eq(mEventCallbacks));
		}