Minor improvements to NetworkController error reporting.
Currently, when trying to perform an operation on netId that
does not exist, we return EINVAL. This can be confusing
because lots of things can return EINVAL for many different
reasons.
Instead, change this to ENONET ("Machine is not on network"),
which was what Sreeram originally implemented before we changed
it to EINVAL. I also considered ENOENT ("No such file or
directory"), but on reflection that seems less appropriate, and
it's used by other things as well. ENONET appears to have no
uses in our tree. It is also clearly separate from the ESRCH we
return for nonexistent rules.
While I'm at it, also disambiguate some of the
if (foo || bar ) { return EFOO; } to return two different error
codes.
Bug: 16667349
Change-Id: Ief2d720a58679aa746f5ba273d545487d0825e52
diff --git a/server/NetworkController.cpp b/server/NetworkController.cpp
index 2ea71e2..ea0f4e2 100644
--- a/server/NetworkController.cpp
+++ b/server/NetworkController.cpp
@@ -143,8 +143,12 @@
if (netId != NETID_UNSET) {
Network* network = getNetworkLocked(netId);
- if (!network || network->getType() != Network::PHYSICAL) {
- ALOGE("invalid netId %u", netId);
+ if (!network) {
+ ALOGE("no such netId %u", netId);
+ return -ENONET;
+ }
+ if (network->getType() != Network::PHYSICAL) {
+ ALOGE("cannot set default to non-physical network with netId %u", netId);
return -EINVAL;
}
if (int ret = static_cast<PhysicalNetwork*>(network)->addAsDefault()) {
@@ -284,10 +288,14 @@
}
int NetworkController::destroyNetwork(unsigned netId) {
- if (netId == LOCAL_NET_ID || !isValidNetwork(netId)) {
- ALOGE("invalid netId %u", netId);
+ if (netId == LOCAL_NET_ID) {
+ ALOGE("cannot destroy local network");
return -EINVAL;
}
+ if (!isValidNetwork(netId)) {
+ ALOGE("no such netId %u", netId);
+ return -ENONET;
+ }
// TODO: ioctl(SIOCKILLADDR, ...) to kill all sockets on the old network.
@@ -315,8 +323,8 @@
int NetworkController::addInterfaceToNetwork(unsigned netId, const char* interface) {
if (!isValidNetwork(netId)) {
- ALOGE("invalid netId %u", netId);
- return -EINVAL;
+ ALOGE("no such netId %u", netId);
+ return -ENONET;
}
unsigned existingNetId = getNetworkForInterface(interface);
@@ -331,8 +339,8 @@
int NetworkController::removeInterfaceFromNetwork(unsigned netId, const char* interface) {
if (!isValidNetwork(netId)) {
- ALOGE("invalid netId %u", netId);
- return -EINVAL;
+ ALOGE("no such netId %u", netId);
+ return -ENONET;
}
android::RWLock::AutoWLock lock(mRWLock);
@@ -362,8 +370,12 @@
android::RWLock::AutoWLock lock(mRWLock);
for (unsigned netId : netIds) {
Network* network = getNetworkLocked(netId);
- if (!network || network->getType() != Network::PHYSICAL) {
- ALOGE("invalid netId %u", netId);
+ if (!network) {
+ ALOGE("no such netId %u", netId);
+ return -ENONET;
+ }
+ if (network->getType() != Network::PHYSICAL) {
+ ALOGE("cannot set permissions on non-physical network with netId %u", netId);
return -EINVAL;
}
@@ -379,8 +391,12 @@
int NetworkController::addUsersToNetwork(unsigned netId, const UidRanges& uidRanges) {
android::RWLock::AutoWLock lock(mRWLock);
Network* network = getNetworkLocked(netId);
- if (!network || network->getType() != Network::VIRTUAL) {
- ALOGE("invalid netId %u", netId);
+ if (!network) {
+ ALOGE("no such netId %u", netId);
+ return -ENONET;
+ }
+ if (network->getType() != Network::VIRTUAL) {
+ ALOGE("cannot add users to non-virtual network with netId %u", netId);
return -EINVAL;
}
if (int ret = static_cast<VirtualNetwork*>(network)->addUsers(uidRanges)) {
@@ -392,8 +408,12 @@
int NetworkController::removeUsersFromNetwork(unsigned netId, const UidRanges& uidRanges) {
android::RWLock::AutoWLock lock(mRWLock);
Network* network = getNetworkLocked(netId);
- if (!network || network->getType() != Network::VIRTUAL) {
- ALOGE("invalid netId %u", netId);
+ if (!network) {
+ ALOGE("no such netId %u", netId);
+ return -ENONET;
+ }
+ if (network->getType() != Network::VIRTUAL) {
+ ALOGE("cannot remove users from non-virtual network with netId %u", netId);
return -EINVAL;
}
if (int ret = static_cast<VirtualNetwork*>(network)->removeUsers(uidRanges)) {
@@ -485,8 +505,16 @@
int NetworkController::modifyRoute(unsigned netId, const char* interface, const char* destination,
const char* nexthop, bool add, bool legacy, uid_t uid) {
+ if (!isValidNetwork(netId)) {
+ ALOGE("no such netId %u", netId);
+ return -ENONET;
+ }
unsigned existingNetId = getNetworkForInterface(interface);
- if (netId == NETID_UNSET || existingNetId != netId) {
+ if (existingNetId == NETID_UNSET) {
+ ALOGE("interface %s not assigned to any netId", interface);
+ return -ENODEV;
+ }
+ if (existingNetId != netId) {
ALOGE("interface %s assigned to netId %u, not %u", interface, existingNetId, netId);
return -ENOENT;
}
@@ -513,10 +541,14 @@
return 0;
}
Network* network = getNetworkLocked(mDefaultNetId);
- if (!network || network->getType() != Network::PHYSICAL) {
+ if (!network) {
ALOGE("cannot find previously set default network with netId %u", mDefaultNetId);
return -ESRCH;
}
+ if (network->getType() != Network::PHYSICAL) {
+ ALOGE("inconceivable! default network must be a physical network");
+ return -EINVAL;
+ }
Permission permission = static_cast<PhysicalNetwork*>(network)->getPermission();
for (const auto& physicalInterface : network->getInterfaces()) {
if (int ret = mDelegateImpl->modifyFallthrough(vpnNetId, physicalInterface, permission,