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

NETCONF TCP client #476

Merged
4 commits merged into from
Jun 14, 2017
Merged

NETCONF TCP client #476

4 commits merged into from
Jun 14, 2017

Conversation

psykokwak4
Copy link
Contributor

@psykokwak4 psykokwak4 commented Jun 13, 2017

  • Add TCP client for libydk, and new python wrapper.
  • Add tests, simple proxy server and update CI scripts.
    Note TCP client does not validate any data as opposed to ssh client(via libnetconf), some exceptions will not be raised.

- add simple tcp proxy server for ConfD
- add TCP client tests, update CI scripts
@ghost ghost added the in progress label Jun 13, 2017
@codecov-io
Copy link

codecov-io commented Jun 13, 2017

Codecov Report

Merging #476 into new_python will increase coverage by 0.06%.
The diff coverage is 93.61%.

Impacted file tree graph

@@              Coverage Diff               @@
##           new_python     #476      +/-   ##
==============================================
+ Coverage       68.12%   68.19%   +0.06%     
==============================================
  Files              98       98              
  Lines           14424    14461      +37     
  Branches         2337     2338       +1     
==============================================
+ Hits             9827     9862      +35     
- Misses           3927     3929       +2     
  Partials          670      670
Impacted Files Coverage Δ
sdk/python/core/tests/test_utils.py 100% <100%> (ø) ⬆️
sdk/python/core/tests/test_sanity_netconf.py 90.37% <100%> (+1.02%) ⬆️
sdk/python/core/ydk/errors/__init__.py 77.14% <66.66%> (-2.17%) ⬇️
sdk/python/core/ydk/errors/error_handler.py 78.68% <80%> (+0.72%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ce329c1...a3a0bc5. Read the comment docs.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Looks good! Thanks. Just a few comments

@@ -38,7 +39,7 @@ typedef struct capabilities {
namespace ydk
{

class NetconfClient
class NetconfClient : public NetconfClientBase
Copy link

Choose a reason for hiding this comment

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

To avoid future confusion, can we change names as below?

class NetconfSSHClient : public NetconfClient

and

class NetconfTCPClient : public NetconfClient

class NetconfClientBase
{
public:
NetconfClientBase() {}
Copy link

Choose a reason for hiding this comment

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

Can we avoid inlining constructors and destructors? This seems like a good guideline

static void replace(std::string& subject, const std::string& search, const std::string& replace);


NetconfTCPClient::NetconfTCPClient(std::string username, std::string password,
Copy link

Choose a reason for hiding this comment

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

Can these variables be changed from std::string to const std::string & to avoid copying?

curl_global_cleanup();
}

void NetconfTCPClient::initialize(std::string address, int port)
Copy link

Choose a reason for hiding this comment

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

Can these variables be changed from std::string to const std::string & to avoid copying?

initialize_curl(address, port);
}

void NetconfTCPClient::initialize_curl(std::string address, int port)
Copy link

Choose a reason for hiding this comment

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

Can these variables be changed from std::string to const std::string & to avoid copying?

return ret;
}

static std::string trim_reply(std::string str)
Copy link

Choose a reason for hiding this comment

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

Can these variables be changed from std::string to const std::string & to avoid copying?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Falls in the category of RVO, using const won't help omit copying.

Copy link

Choose a reason for hiding this comment

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

Makes sense for return value. But for arguments, const is required

Copy link
Contributor Author

Choose a reason for hiding this comment

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

with no optimization options

@@ -485,8 +485,7 @@ PYBIND11_PLUGIN(ydk_)
class_<ydk::NetconfServiceProvider, ydk::path::ServiceProvider>(providers, "NetconfServiceProvider")
.def("__init__",
[](ydk::NetconfServiceProvider &nc_provider, ydk::path::Repository& repo, string address, string username, string password, int port, string protocol) {
Copy link

Choose a reason for hiding this comment

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

Can these variables be changed here and in netconf_provider.hpp from std::string to const std::string & to avoid copying?

- rename NetconfClient to NetconfSSHClient
- add const declaration
- fix simple tcp proxy server recv method
@ghost ghost merged commit 8f8b77e into CiscoDevNet:new_python Jun 14, 2017
@ghost ghost deleted the new_python-tcp branch June 14, 2017 01:47
ylil93 pushed a commit to ylil93/ydk-gen that referenced this pull request Sep 14, 2017
- rename NetconfClient to NetconfSSHClient
- add const declaration
- fix simple tcp proxy server recv method
This pull request was closed.
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