support configurable interface name for wpantund. #2

LeZhang2016 · 2017-05-30T13:16:30Z

No description provided.

bukepo · 2017-05-30T13:29:54Z

src/web/web-service/web_service.hpp

@@ -57,12 +57,13 @@ namespace Web {

 typedef SimpleWeb::Server<SimpleWeb::HTTP> HttpServer;

+


nit: is this line needed?

i will remove it, thanks.

bukepo · 2017-05-30T13:30:59Z

src/web/wpan-controller/dbus_base.cpp

@@ -45,15 +45,15 @@ namespace Dbus {

 DBusConnection *DBusBase::GetConnection(void)
 {
-    dbus_error_init(&error);
-    mConnection = dbus_bus_get(DBUS_BUS_STARTER, &error);
+    dbus_error_init(&mError);


Is it better to make mError as local variable?

mError is the member of the basic class, i think sub class could use it directly and no need to define the local variable again. Thanks.

It's only meaningful in local scope. See one core component of OpenThread

bukepo · 2017-05-30T13:35:12Z

src/web/wpan-controller/dbus_scan.cpp

-    dbus_bus_add_match(dbusConnection, dbusObjectManagerMatchString, &error);
-    VerifyOrExit(error.name == NULL, ret = kWpantundStatus_Failure);
+    dbus_bus_add_match(dbusConnection, dbusObjectManagerMatchString, &mError);
+    VerifyOrExit(mError.name == NULL, ret = kWpantundStatus_Failure);


Is it better to use the documented API to check if error occurred? dbus_error_is_set()

Good suggestion, i will use this API to check the failure.

bukepo · 2017-05-30T13:35:55Z

src/web/wpan-controller/wpan_controller.cpp

 exit:
    return ret;
 }

 const WpanNetworkInfo *WPANController::GetScanNetworksInfo(void)
 {
-    return gScanedNetowrks;
+    return mScanedNetowrks;


I will modify it.

bukepo · 2017-05-30T13:37:53Z

src/web/wpan-controller/wpan_controller.hpp

+    int Leave(void);
+    int Form(const char *aNetworkname, int aChannel);
+    int Scan(void);
+    int Join(char *aNetworkname, int16_t aChannel, uint64_t aExtPanId,


const char *aNetworkName?

i will modify it.

bukepo · 2017-05-30T13:38:53Z

src/web/wpan-controller/wpan_controller.hpp

@@ -37,6 +37,8 @@
 #define WPAN_TUNNEL_DBUS_PATH   "/com/nestlabs/WPANTunnelDriver"
 #define WPANTUND_DBUS_PATH      "/org/wpantund"
 #define WPANTUND_DBUS_APIv1_INTERFACE "org.wpantund.v1"
+#define WPAN_TUNNEL_DBUS_INTERFACE  "com.nestlabs.WPANTunnelDriver"


Is it possible to use the headers files in third_party/wpantund?

bukepo · 2017-05-30T14:09:05Z

src/web/main.cpp

+#include <syslog.h>
+#include <unistd.h>
+
+#include "otbr-config.h"


This should be the first included file.

bukepo · 2017-05-30T14:11:38Z

src/web/main.cpp

 #include "web-service/web_service.hpp"
+#include "common/code_utils.hpp"


see [style guide]https://github.com/openthread/openthread/blob/master/STYLE_GUIDE.md

Preprocessor #include directives should be grouped, ordered, or sorted as follows:

..

Alphanumeric order within each subgroup

bukepo · 2017-05-30T14:12:34Z

src/web/main.cpp

-    server.StartWebServer();
-    return 0;
+exit:
+    server = NULL;


memory leak. server not deleted.

bukepo · 2017-05-30T14:15:17Z

src/web/web-service/web_service.hpp

@@ -88,6 +88,7 @@ class WebServer
    static int                       sNetworksCount;
    static std::string               sNetowrkName, sExtPanId;
    static bool                      sIsStarted;
+    char                             mIfName[DBUS_MAXIMUM_NAME_LENGTH + 1];


Is this mIfName related to DBUS?

mIfName records network interface such as "wpan0".

If it's not related to D-Bus, refer http://pubs.opengroup.org/onlinepubs/009696699/functions/if_indextoname.html

mIfName is used to generate the parameters of the dbus_message_new_method_call

According to source of web_service.cpp, it seems mIfName only holds a network interface name, which is independent of d-bus.

bukepo · 2017-05-30T14:21:07Z

src/web/wpan-controller/wpan_controller.cpp

+    dbusIfName.SetInterfaceName(mIfName);
+    VerifyOrExit(dbusIfName.ProcessReply() == kWpantundStatus_Ok, ret = kWpantundStatus_InvalidDBusName);
+exit:
+    return ret ? NULL : dbusIfName.GetDBusName();


Is this actually returning a temporary stack address of dbusIfName::GetDBusName()?

…e-options to staging/fix/cmake-package-options * commit '82943e84c02ed2c0fc6022942807b92ebbee5e01': Remove defaults for package name and version for the OpenThread border router, so that vendors can set their own values for it.

bukepo reviewed May 30, 2017

View reviewed changes

LeZhang2016 force-pushed the feature-configurable-interfaceName branch from ea69ea0 to ca22cbb Compare May 30, 2017 14:00

bukepo reviewed May 30, 2017

View reviewed changes

LeZhang2016 force-pushed the feature-configurable-interfaceName branch 3 times, most recently from 0af9094 to 6931529 Compare May 30, 2017 14:57

support configurable interface name for wpantund.

a2bf4eb

LeZhang2016 force-pushed the feature-configurable-interfaceName branch from 6931529 to a2bf4eb Compare May 30, 2017 15:13

LeZhang2016 closed this May 30, 2017

chatref mentioned this pull request Apr 13, 2023

otbr docker arm64 image shutting down wpan0 interface after network is formed successfully #1825

Closed

sherysheng mentioned this pull request Jul 8, 2024

[pd] add pd daemon service to support the automation of dhcpcd PD management #2360

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

support configurable interface name for wpantund. #2

support configurable interface name for wpantund. #2

LeZhang2016 commented May 30, 2017

bukepo May 30, 2017

LeZhang2016 May 30, 2017

LeZhang2016 May 30, 2017

bukepo May 30, 2017

LeZhang2016 May 30, 2017

bukepo May 30, 2017

LeZhang2016 May 30, 2017

bukepo May 30, 2017

LeZhang2016 May 30, 2017

bukepo May 30, 2017

LeZhang2016 May 30, 2017

bukepo May 30, 2017

LeZhang2016 May 30, 2017

bukepo May 30, 2017

bukepo May 30, 2017

bukepo May 30, 2017

bukepo May 30, 2017

bukepo May 30, 2017

LeZhang2016 May 30, 2017

bukepo May 30, 2017

LeZhang2016 May 30, 2017

bukepo May 30, 2017

bukepo May 30, 2017

		@@ -57,12 +57,13 @@ namespace Web {

		typedef SimpleWeb::Server<SimpleWeb::HTTP> HttpServer;

		#include "web-service/web_service.hpp"
		#include "common/code_utils.hpp"

support configurable interface name for wpantund. #2

support configurable interface name for wpantund. #2

Conversation

LeZhang2016 commented May 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment