improve "get port" mechanism #2331
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,93 @@ | ||
| /** | ||
| * Copyright (c) 2014-2016 by the respective copyright holders. | ||
| * 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.eclipse.smarthome.core.net; | ||
|
|
||
| import org.osgi.framework.BundleContext; | ||
| import org.osgi.framework.Constants; | ||
| import org.osgi.framework.InvalidSyntaxException; | ||
| import org.osgi.framework.ServiceReference; | ||
|
|
||
| /** | ||
| * Some utility functions related to the http service | ||
| * | ||
| * @author Markus Rathgeb - Initial contribution and API | ||
| */ | ||
| public class HttpServiceUtil { | ||
|
|
||
| private HttpServiceUtil() { | ||
| } | ||
|
|
||
| /** | ||
| * Get the port that is used by the HTTP service. | ||
| * | ||
| * @param bc the bundle context used for lookup | ||
| * @return the port if used, -1 if no port could be found | ||
| */ | ||
| public static int getHttpServicePort(final BundleContext bc) { | ||
| return getHttpServicePortProperty(bc, "org.osgi.service.http.port"); | ||
| } | ||
|
|
||
| public static int getHttpServicePortSecure(final BundleContext bc) { | ||
| return getHttpServicePortProperty(bc, "org.osgi.service.http.port.secure"); | ||
| } | ||
|
|
||
| // Utility class that could be used for non-secure and secure port. | ||
| private static int getHttpServicePortProperty(final BundleContext bc, final String propertyName) { | ||
| Object value; | ||
| int port = -1; | ||
|
|
||
| // Try to find the port by using the service property (respect service ranking). | ||
| try { | ||
| int candidate = Integer.MIN_VALUE; | ||
| final ServiceReference[] refs = bc.getAllServiceReferences("org.osgi.service.http.HttpService", null); | ||
| for (final ServiceReference ref : refs) { | ||
| value = ref.getProperty(propertyName); | ||
| if (value == null) { | ||
| continue; | ||
| } | ||
| final int servicePort; | ||
| try { | ||
| servicePort = Integer.parseInt(value.toString()); | ||
| } catch (final NumberFormatException ex) { | ||
| continue; | ||
| } | ||
| value = ref.getProperty(Constants.SERVICE_RANKING); | ||
| final int serviceRanking; | ||
| if (value == null || !(value instanceof Integer)) { | ||
| serviceRanking = 0; | ||
| } else { | ||
| serviceRanking = (Integer) value; | ||
| } | ||
| if (serviceRanking >= candidate) { | ||
| candidate = serviceRanking; | ||
| port = servicePort; | ||
| } | ||
| } | ||
| } catch (final InvalidSyntaxException ex) { | ||
|
There was a problem hiding this comment. I know, this in theory cannot happen. However, if it does for whatever crazy circumstances, it's hard do find. So IMHO it can't hurt to log it anyway.... There was a problem hiding this comment. You are right, a log is normally okay. If you have a look at the API documentation the There was a problem hiding this comment. I see your point. Nevertheless, having spent way to much time analyzing weird bugs searching in the dark, I would prefer the ad-hoc logger creation in that case. As it's a will-never-happen-if-we-can-rely-on-laws-of-physics-Exception this will not hurt here. |
||
| } | ||
| if (port > 0) { | ||
| return port; | ||
| } | ||
|
|
||
| // If the service does not provide the port, try to use the system property. | ||
| value = bc.getProperty(propertyName); | ||
| if (value != null) { | ||
| if (value instanceof String) { | ||
| try { | ||
| return Integer.parseInt(value.toString()); | ||
| } catch (final NumberFormatException ex) { | ||
| } | ||
|
There was a problem hiding this comment. Same here: it's a property set by the user - if this property is set with bogus, somebody will be pretty disappointed if it gets ignored silently There was a problem hiding this comment. We would like to parse the port number used by the HTTP servlet. There was a problem hiding this comment.
good point |
||
| } else if (value instanceof Integer) { | ||
| return (Integer) value; | ||
| } | ||
| } | ||
|
|
||
| return -1; | ||
| } | ||
|
|
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,63 @@ | ||
| /** | ||
| * Copyright (c) 2014-2016 by the respective copyright holders. | ||
| * 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.eclipse.smarthome.core.net; | ||
|
|
||
| import java.net.Inet6Address; | ||
| import java.net.InetAddress; | ||
| import java.net.NetworkInterface; | ||
| import java.net.SocketException; | ||
| import java.util.Enumeration; | ||
|
|
||
| import org.slf4j.Logger; | ||
| import org.slf4j.LoggerFactory; | ||
|
|
||
| /** | ||
| * Some utility functions related to network interfaces etc. | ||
| * | ||
| * @author Markus Rathgeb - Initial contribution and API | ||
| */ | ||
| public class NetUtil { | ||
|
|
||
| private static final Logger LOGGER = LoggerFactory.getLogger(NetUtil.class); | ||
|
There was a problem hiding this comment. Isn't the convention in ESH to use non-static loggers? There was a problem hiding this comment. Yes, definitely. We agreed that in static context we can use a static logger. |
||
|
|
||
| private NetUtil() { | ||
| } | ||
|
|
||
| /** | ||
| * Get the first candidate for a local IPv4 host address (non loopback, non localhost). | ||
|
There was a problem hiding this comment. Running the stuff on a server with several network interfaces, such automatisms tend to always pick the wrong one. Shouldn't we also provide some configuration options (i.e. ConfigAdmin + system property) to override this? There was a problem hiding this comment. As written above and comment in the PR itself, this method (and its whole logic) has been migrated from #2323 |
||
| */ | ||
| public static String getLocalIpv4HostAddress() { | ||
|
There was a problem hiding this comment. somehow this looks weird to me, IMHO getLocalIPv4HostAddress is more brain-friendly, but that could just be personal preference... There was a problem hiding this comment. I don't care much. There are different meanings about correct camel case usage. |
||
| try { | ||
| String hostAddress = null; | ||
| final Enumeration<NetworkInterface> interfaces = NetworkInterface.getNetworkInterfaces(); | ||
| while (interfaces.hasMoreElements()) { | ||
| final NetworkInterface current = interfaces.nextElement(); | ||
| if (!current.isUp() || current.isLoopback() || current.isVirtual()) { | ||
|
There was a problem hiding this comment. are you sure that ignoring virtual interfaces is correct? There was a problem hiding this comment. Oh, just realized I'm asking the wrong one... #2323 ;-) There was a problem hiding this comment.
😉 There was a problem hiding this comment. I merged it nonetheless; if we want to include virtual interfaces, we can change this logic in a new PR. @gerrieg: You might want to update the homematic binding to use the new NetUtil class (which now holds your method :-)) |
||
| continue; | ||
| } | ||
| final Enumeration<InetAddress> addresses = current.getInetAddresses(); | ||
| while (addresses.hasMoreElements()) { | ||
| final InetAddress current_addr = addresses.nextElement(); | ||
| if (current_addr.isLoopbackAddress() || (current_addr instanceof Inet6Address)) { | ||
| continue; | ||
| } | ||
| if (hostAddress != null) { | ||
| LOGGER.warn("Found multiple local interfaces - ignoring {}", current_addr.getHostAddress()); | ||
| } else { | ||
| hostAddress = current_addr.getHostAddress(); | ||
| } | ||
| } | ||
| } | ||
| return hostAddress; | ||
| } catch (SocketException ex) { | ||
| LOGGER.error("Could not retrieve network interface: " + ex.getMessage(), ex); | ||
| return null; | ||
| } | ||
| } | ||
|
|
||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't get that comment...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logic for "org.osgi.service.http.port" and "org.osgi.service.http.port.secure" is the same only the property name differs. So a private method
getHttpServicePortPropertyhas been used that takes the property as argument.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, okay, got it. Then please change class to method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, didn't see that type. Thanks.