Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[posix] Add Cli Daemon module #2676

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

yangsong-cnyn
Copy link
Contributor

@yangsong-cnyn yangsong-cnyn commented Jan 20, 2025

This PR introduces a new daemon implementation for OTBR on POSIX systems. This daemon uses socket to facilitate CLI interaction between the client and OTBR running as a service.

Details:

The Daemon class manages the Unix domain socket, CLI sessions, and Future Spinel communication with the NCP.

  • Daemon::Init() creates and binds the listening socket.

  • Daemon::Process() handles incoming CLI commands and forwards them to the NCP.

  • Daemon::OutputFormat() and Daemon::OutputFormatV() format and send responses back to the CLI client.

  • (TODO) Spinel frame encoding and decoding will be performed using existing Spinel utilities.

Test:
ot-ctl
state
netdata show
test command

syslog
2025-01-20T06:16:39.825758+00:00 yangsongcn.c.googlers.com otbr-agent[1494841]: [WARN]-DAEMON--: Received CLI command: state
2025-01-20T06:16:44.223787+00:00 yangsongcn.c.googlers.com otbr-agent[1494841]: [WARN]-DAEMON--: Received CLI command: netdata show
2025-01-20T06:16:48.046883+00:00 yangsongcn.c.googlers.com otbr-agent[1494841]: [WARN]-DAEMON--: Received CLI command: test command

Copy link

codecov bot commented Jan 20, 2025

Codecov Report

Attention: Patch coverage is 27.40741% with 98 lines in your changes missing coverage. Please review.

Project coverage is 43.52%. Comparing base (2b41187) to head (4cc806f).
Report is 921 commits behind head on main.

Files with missing lines Patch % Lines
src/host/posix/daemon.cpp 25.00% 87 Missing and 9 partials ⚠️
src/host/ncp_host.cpp 60.00% 2 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##             main    #2676       +/-   ##
===========================================
- Coverage   55.77%   43.52%   -12.25%     
===========================================
  Files          87      107       +20     
  Lines        6890    13403     +6513     
  Branches        0      976      +976     
===========================================
+ Hits         3843     5834     +1991     
- Misses       3047     7253     +4206     
- Partials        0      316      +316     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@yangsong-cnyn yangsong-cnyn requested a review from abtink January 20, 2025 07:19
@yangsong-cnyn yangsong-cnyn marked this pull request as draft January 20, 2025 08:02
@yangsong-cnyn yangsong-cnyn removed the request for review from abtink January 20, 2025 08:02
Copy link
Contributor

@Irving-cl Irving-cl left a comment

Choose a reason for hiding this comment

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

Thanks for the quick implementation 👍

I have a few comments.

Comment on lines +64 to +90
std::string Daemon::GetSocketFilename() const
{
std::string fileName;
std::string netIfName = mNetifName.empty() ? OTBR_POSIX_CONFIG_THREAD_NETIF_DEFAULT_NAME : mNetifName;

fileName = OTBR_POSIX_CONFIG_DAEMON_SOCKET_BASENAME + netIfName + ".sock";
if (fileName.size() > kMaxSocketFilenameLength)
{
DieNow(otbrErrorString(OTBR_ERROR_INVALID_ARGS));
}

return fileName;
}

std::string Daemon::GetSocketLockFilename() const
{
std::string fileName;
std::string netIfName = mNetifName.empty() ? OTBR_POSIX_CONFIG_THREAD_NETIF_DEFAULT_NAME : mNetifName;

fileName = OTBR_POSIX_CONFIG_DAEMON_SOCKET_BASENAME + netIfName + ".lock";
if (fileName.size() > kMaxSocketFilenameLength)
{
DieNow(otbrErrorString(OTBR_ERROR_INVALID_ARGS));
}

return fileName;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

The two functions are quite similar. Can we write one function that works for both .sock and .lock?


#define OTBR_LOG_TAG "DAEMON"

#include "daemon.hpp"
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel that the name "daemon" isn't quite appropriate. Because usually we take the whole program as a "daemon". This module simply handles the cli command. So I suggest renaming all the "daemon" occurrences to something like "cli-daemon", including the class name. Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree, will update.

DieNow(otbrErrorString(OTBR_ERROR_ERRNO));
}

// TODO:: Send Spinel message to NCP to initialize CLI (otCliInit)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the CLI module on NCP should initialize at first and the initialization isn't triggered by and message from the host. Thoughts?

src/host/posix/netif.hpp Show resolved Hide resolved
buffer[rval] = '\0';

otbrLogWarning("Received CLI command: %s", reinterpret_cast<char *>(buffer));
// TODO:: Send Spinel message to NCP to process the command (otCliInputLine)
Copy link
Contributor

Choose a reason for hiding this comment

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

Please abstract this process as an interface and this interface can be implememted by NcpSpinel (or some classes under RCP mode in the future). In this way, we can use this module for both NCP and RCP (later we transition to use this module). In addition, we can have a test implementation of this interface and write unit tests.

You may refer to Netif::Dependencies and tests/gtest/test_netif.cpp. The scheme will be very similar.

Then please add a unit test similar as test_netif.cpp. In the test, we simply create an instance of the Daemon class and a Mainloop. And we can also create a client socket and write cli commands to the socket. Then run the mainloop. Next we can check the call of our test implementation of the interface.

To run the gtest, build otbr-agent with -DBUILD_TESTING=ON flag. (By default it should be turned on)
Then run ${YOUR_BUILD_DIR}/tests/gtest/${YOUR_GTEST_PROGRAM}.

@Irving-cl Irving-cl changed the title [NCP] Implement OTBR Daemon for POSIX system [posix] Add Cli Daemon module Jan 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants