integrate the mDNS client into the publisher. Start mDNS client, if there is a existing server running on the system. #16

LeZhang2016 · 2017-06-04T13:34:22Z

No description provided.

bukepo · 2017-06-04T15:44:56Z

src/web/mdns-publisher/mdns_publisher.cpp


+#include "common/code_utils.hpp"
 #include "mdns_publisher.hpp"


"mdns_publisher.hpp" should be kept as the first included file.

Preprocessor #include directives should be grouped, ordered, or sorted as follows:
This compilation unit's corresponding header, if any.
C++ Standard Library headers
C Standard Library headers
Third-party library headers
First-party library headers
Private or local headers
Alphanumeric order within each subgroup

bukepo · 2017-06-04T15:51:27Z

src/web/mdns-publisher/mdns_publisher.cpp

@@ -222,20 +376,58 @@ void Publisher::SetServiceName(const char *aServiceName)
    mServiceName = avahi_strdup(aServiceName);


Is it possible to leak memory here? It seems the previous memory of mServiceNames is not freed.

bukepo · 2017-06-04T15:59:11Z

src/web/web-service/web_service.cpp

                sNetworkName = "nn=" + sNetworkName;
                sExtPanId = "xp=" + sExtPanId;
-                ot::Mdns::Publisher::SetNetworkNameTxT(sNetworkName.c_str());
-                ot::Mdns::Publisher::SetEPANIDTxT(sExtPanId.c_str());
+                ot::Mdns::Publisher::Instance().SetNetworkNameTxT(sNetworkName.c_str());


Is it safe to let Publisher save the pointer returned by std::string::c_str()? Below is the doc of std::string::c_str()

The pointer returned may be invalidated by further calls to other member functions that modify the object.

bukepo · 2017-06-04T16:05:35Z

src/web/mdns-publisher/mdns_publisher.cpp

+        avahi_simple_poll_quit(mSimplePoll);
+        if (mServer)
+            avahi_server_free(mServer);
+        StartClient();


missing break; below?

jwhui · 2017-06-05T23:06:53Z

src/web/mdns-publisher/mdns_publisher.hpp

+    void PeriodicallyPublish(AVAHI_GCC_UNUSED AvahiTimeout *aTimeout);
+    void Free(void);
+
+    static Publisher &Instance(void)


Methods should have a verb (e.g. GetInstance()).

jwhui · 2017-06-05T23:07:34Z

src/web/mdns-publisher/mdns_publisher.cpp

@@ -31,6 +31,9 @@
 *   This file implements starting an mDNS server, and publish border router service.
 */

+#include <avahi-client/publish.h>
+#include <avahi-core/publish.h>
+
 #include "common/code_utils.hpp"

 #include "mdns_publisher.hpp"


The compilation unit's corresponding header should appear first.

jwhui · 2017-06-05T23:08:28Z

src/web/mdns-publisher/mdns_publisher.cpp

+        break;
+
+    case AVAHI_CLIENT_CONNECTING:
+        ;


I know break is not needed, but it is cleaner to include it.

jwhui · 2017-06-05T23:08:42Z

src/web/mdns-publisher/mdns_publisher.cpp

        break;

    case AVAHI_SERVER_FAILURE:

        syslog(LOG_ERR, "Server failure: %s\n",
               avahi_strerror(avahi_server_errno(aServer)));
        avahi_simple_poll_quit(mSimplePoll);
-        isStart = false;
        break;

    case AVAHI_SERVER_INVALID:
        ;


Include break here.

jwhui · 2017-06-05T23:08:59Z

src/web/mdns-publisher/mdns_publisher.cpp

+
+    if ((mClient != NULL) && (avahi_client_get_state(mClient) == AVAHI_CLIENT_S_RUNNING))
+    {
+


Unnecessary newline.

jwhui · 2017-06-05T23:09:11Z

src/web/mdns-publisher/mdns_publisher.cpp

+
+    Free();
+    return ret;
+


Unnecessary newline.

jwhui · 2017-06-05T23:09:31Z

src/web/mdns-publisher/mdns_publisher.cpp

+
+    case AVAHI_ENTRY_GROUP_UNCOMMITED:
+    case AVAHI_ENTRY_GROUP_REGISTERING:
+        ;


Include break.

jwhui · 2017-06-05T23:10:13Z

src/web/mdns-publisher/mdns_publisher.hpp

-    static uint16_t          mPort;
-    static char             *mServiceName;
-    static bool              isStart;
+    void ServerCallback(AvahiServer *aServer, AvahiServerState aState);


Methods should include verbs. For example HandleServerCallback(), or something more appropriate.

jwhui · 2017-06-05T23:11:02Z

src/web/mdns-publisher/mdns_publisher.hpp

+                               AVAHI_GCC_UNUSED void *aUserData);
+    void ClientCallback(AvahiClient *aClient, AvahiClientState aState);
+
+    static void PeriodicallyPublish(AVAHI_GCC_UNUSED AvahiTimeout *aTimeout, AVAHI_GCC_UNUSED void *aUserData);


Methods should include verbs. PeriodicallyPublish does not tell me what this method actually does.

LeZhang2016 · 2017-06-07T07:18:19Z

src/web/mdns-publisher/mdns_publisher.cpp

+
+    if (avahi_entry_group_is_empty(mClientGroup))
+    {
+


bukepo

Thanks for addressing this issue. I think we should consider if core api of avahi should be avoided later, as suggested in avahi's doc.

bukepo · 2017-06-07T07:41:47Z

src/web/mdns-publisher/mdns_publisher.cpp

@@ -70,6 +101,11 @@ void Publisher::Free(void)
        avahi_server_free(mServer);
    }

+    if (mClient)
+    {
+        avahi_client_free(mClient);


add mClient = NULL?

bukepo · 2017-06-07T07:41:59Z

src/web/mdns-publisher/mdns_publisher.cpp

@@ -70,6 +101,11 @@ void Publisher::Free(void)
        avahi_server_free(mServer);


add mServer = NULL?

bukepo · 2017-06-07T07:44:44Z

src/web/mdns-publisher/mdns_publisher.cpp

@@ -78,21 +114,80 @@ void Publisher::Free(void)
    avahi_free(mServiceName);


Initialize mServiceName and check if it's NULL here?

bukepo · 2017-06-07T07:48:16Z

src/web/mdns-publisher/mdns_publisher.cpp

+                                                   mNetworkNameTxt, mExtPanIdTxt, NULL) == 0,
+                     ret = kMdnsPublisher_FailedAddSevice);
+
+        syslog(LOG_INFO, " Service Name: %s \n Port: %d \n Network Name: %s \n Extended Pan ID: %s \n",


syslog should not include \n

bukepo · 2017-06-07T07:48:53Z

src/web/mdns-publisher/mdns_publisher.cpp

+        serviceName = avahi_alternative_service_name(mServiceName);
+        avahi_free(mServiceName);
+        mServiceName = serviceName;
+        syslog(LOG_INFO, "Service name collision, renaming service to '%s'\n", mServiceName);


\n as suggested above.

bukepo · 2017-06-07T07:51:29Z

src/web/mdns-publisher/mdns_publisher.cpp

+        avahi_simple_poll_quit(mSimplePoll);
+        if (mServer)
+        {
+            avahi_server_free(mServer);


Is it safer to add mServer = NULL here?

bukepo · 2017-06-07T08:01:38Z

src/web/mdns-publisher/mdns_publisher.cpp

 {
-    mNetworkNameTxt = aTxtData;
+


unnecessary new line.

bukepo · 2017-06-07T08:02:56Z

src/web/mdns-publisher/mdns_publisher.cpp

 {
-    mNetworkNameTxt = aTxtData;
+
+    avahi_free(mNetworkNameTxt);


mNetworkNameTxt is not initialized?

bukepo · 2017-06-07T08:03:13Z

src/web/mdns-publisher/mdns_publisher.cpp

 {
-    mExtPanIdTxt = aTxtData;
+    avahi_free(mExtPanIdTxt);


not initialized?

bukepo · 2017-06-07T08:03:33Z

src/web/mdns-publisher/mdns_publisher.cpp

 }

 void Publisher::SetType(const char *aType)
 {
-
-    mType = aType;
+    avahi_free(mType);


not initialized?

bukepo · 2017-06-07T13:33:13Z

script/bootstrap

@@ -44,7 +44,7 @@ install_packages_apt()
    sudo apt-get install -y autoconf-archive

    # mDNS
-    sudo apt-get install -y libavahi-core-dev
+    sudo apt-get install -y libavahi-common-dev libavahi-client-dev


Is avahi-daemon required?

bukepo · 2017-06-07T13:37:02Z

src/web/mdns-publisher/mdns_publisher.cpp

    }

-    avahi_free(mServiceName);
+    if (mServiceName)


Keep same style of checking pointer is NULL or not? I found

if (xxx) if (!xxx) if (xxx != NULL) (xxx == NULL)

bukepo · 2017-06-07T13:37:21Z

src/web/mdns-publisher/mdns_publisher.cpp

+
+    if (mClientGroup != NULL)
+    {
+        avahi_entry_group_free(mClientGroup);


set it to NULL?

bukepo · 2017-06-07T13:40:48Z

src/web/mdns-publisher/mdns_publisher.cpp

-                                          mNetworkNameTxt, mExtPanIdTxt, NULL) == 0,
-                 ret = kMdnsPublisher_FailedAddSevice);
+        VerifyOrExit(avahi_entry_group_add_service(mClientGroup, AVAHI_IF_UNSPEC,
+                                                   AVAHI_PROTO_UNSPEC, (AvahiPublishFlags) 0,


Use static_cast<AvahiPublishFlags>?

bukepo · 2017-06-07T13:41:49Z

src/web/mdns-publisher/mdns_publisher.cpp


-    syslog(LOG_INFO, " Service Name: %s Port: %d Network Name: %s Extended Pan ID: %s ",
-           mServiceName, mPort, mNetworkNameTxt, mExtPanIdTxt);
+        syslog(LOG_INFO, " Service Name: %s  Port: %d  Network Name: %s  Extended Pan ID: %s ",


unnecessary space at end of syslog format string. And is the preceding space in intended?

bukepo · 2017-06-07T13:43:43Z

src/web/mdns-publisher/mdns_publisher.cpp


 exit:
-    if (ret != kMdnsPublisher_OK)
+
+    if (ret == kMdnsPublisher_FailedAddSevice)


use switch?

bukepo · 2017-06-07T13:44:44Z

src/web/mdns-publisher/mdns_publisher.cpp

+        mServiceName = serviceName;
+        syslog(LOG_INFO, "Service name collision, renaming service to '%s'", mServiceName);
+        avahi_entry_group_reset(mClientGroup);
+        CreateService(aClient);


Is it intended to ignore error from CreateService()?

bukepo · 2017-06-07T13:45:52Z

src/web/mdns-publisher/mdns_publisher.cpp

-    {
-        avahi_simple_poll_quit(mSimplePoll);
+    case AVAHI_CLIENT_CONNECTING:
+        break;


missing default case?

…here is a server running on the system.

LeZhang2016 · 2017-06-07T15:29:50Z

@bukepo, Thanks a lot for your comments and suggestions, I have updated this RP according to your comments.

LeZhang2016 changed the title ~~integrate the mDNS client into the publisher. Start mDNS client, if there is a server running on the system.~~ integrate the mDNS client into the publisher. Start mDNS client, if there is a existing server running on the system. Jun 4, 2017

LeZhang2016 force-pushed the feature-mdns branch 2 times, most recently from 2bae93b to f62583d Compare June 4, 2017 14:58

LeZhang2016 requested a review from bukepo June 4, 2017 15:36

LeZhang2016 added the enhancement label Jun 4, 2017

bukepo reviewed Jun 4, 2017

View reviewed changes

LeZhang2016 force-pushed the feature-mdns branch 3 times, most recently from d6822be to 9113a9a Compare June 5, 2017 12:40

jwhui requested a review from bukepo June 5, 2017 23:05

jwhui reviewed Jun 5, 2017

View reviewed changes

LeZhang2016 force-pushed the feature-mdns branch 10 times, most recently from 75237de to 333ad64 Compare June 7, 2017 03:02

LeZhang2016 commented Jun 7, 2017

View reviewed changes

src/web/mdns-publisher/mdns_publisher.cpp Outdated

if (avahi_entry_group_is_empty(mClientGroup))

{

Copy link

Author

LeZhang2016 Jun 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

space line

LeZhang2016 force-pushed the feature-mdns branch from 333ad64 to abb9d08 Compare June 7, 2017 07:41

bukepo reviewed Jun 7, 2017

View reviewed changes

LeZhang2016 force-pushed the feature-mdns branch 6 times, most recently from 32e569b to 22e23ad Compare June 7, 2017 10:05

bukepo reviewed Jun 7, 2017

View reviewed changes

LeZhang2016 force-pushed the feature-mdns branch from 22e23ad to 7f2ef56 Compare June 7, 2017 15:25

Integrate the mDNS client into the publisher. Start mDNS client, if t…

6ee7563

…here is a server running on the system.

LeZhang2016 force-pushed the feature-mdns branch from 7f2ef56 to 6ee7563 Compare June 7, 2017 15:28

bukepo approved these changes Jun 7, 2017

View reviewed changes

jwhui approved these changes Jun 7, 2017

View reviewed changes

jwhui merged commit f65b859 into openthread:master Jun 7, 2017

gadekula1 mentioned this pull request Oct 30, 2023

When giving dbus send command for AddExternalRoute, otbr-agent crashing #2079

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

integrate the mDNS client into the publisher. Start mDNS client, if there is a existing server running on the system. #16

integrate the mDNS client into the publisher. Start mDNS client, if there is a existing server running on the system. #16

LeZhang2016 commented Jun 4, 2017

bukepo Jun 4, 2017

bukepo Jun 4, 2017

bukepo Jun 4, 2017

bukepo Jun 4, 2017

jwhui Jun 5, 2017

jwhui Jun 5, 2017

jwhui Jun 5, 2017

jwhui Jun 5, 2017

jwhui Jun 5, 2017

jwhui Jun 5, 2017

jwhui Jun 5, 2017

jwhui Jun 5, 2017

jwhui Jun 5, 2017

LeZhang2016 Jun 7, 2017

bukepo left a comment

bukepo Jun 7, 2017

bukepo Jun 7, 2017

bukepo Jun 7, 2017

bukepo Jun 7, 2017

bukepo Jun 7, 2017

bukepo Jun 7, 2017

bukepo Jun 7, 2017

bukepo Jun 7, 2017

bukepo Jun 7, 2017

bukepo Jun 7, 2017

bukepo Jun 7, 2017

bukepo Jun 7, 2017

bukepo Jun 7, 2017

bukepo Jun 7, 2017

bukepo Jun 7, 2017

bukepo Jun 7, 2017

bukepo Jun 7, 2017

bukepo Jun 7, 2017

LeZhang2016 commented Jun 7, 2017


		#include "common/code_utils.hpp"
		#include "mdns_publisher.hpp"

		@@ -222,20 +376,58 @@ void Publisher::SetServiceName(const char *aServiceName)
		mServiceName = avahi_strdup(aServiceName);


		if ((mClient != NULL) && (avahi_client_get_state(mClient) == AVAHI_CLIENT_S_RUNNING))
		{

		@@ -70,6 +101,11 @@ void Publisher::Free(void)
		avahi_server_free(mServer);

		@@ -78,21 +114,80 @@ void Publisher::Free(void)
		avahi_free(mServiceName);

integrate the mDNS client into the publisher. Start mDNS client, if there is a existing server running on the system. #16

integrate the mDNS client into the publisher. Start mDNS client, if there is a existing server running on the system. #16

Conversation

LeZhang2016 commented Jun 4, 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

bukepo left a 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

LeZhang2016 commented Jun 7, 2017