-
Notifications
You must be signed in to change notification settings - Fork 2
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
every parts are wroking, implemented ndnsd-tool completly and also th… #2
base: test
Are you sure you want to change the base?
Conversation
// send back the response | ||
static const std::string content("Update Successful"); | ||
// Create Data packet | ||
auto data = make_shared<ndn::Data>(interest.getName()); |
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.
shared_ptr is not needed here.
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.
done
|
||
auto dataContent = makeDataContent(); | ||
auto& data = wireEncode(dataContent, status); | ||
replyData->setContent(data); |
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.
Combine:
replyData->setContent(wireEncode(makeDataContent(), status));
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.
done
auto status = (timeDiff >= m_producerState.serviceLifetime*1000) | ||
? EXPIRED : ACTIVE; | ||
|
||
std::shared_ptr<ndn::Data> replyData = std::make_shared<ndn::Data>(name); |
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.
No need for shared_ptr
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.
done
replyData->setContent(data); | ||
m_keyChain.sign(*replyData); | ||
|
||
try |
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.
Remove try catch -have try catch on processEvents for failure of face.
// reset the wire first | ||
if(m_wire.hasWire()) | ||
m_wire.reset(); | ||
// |service-name|<applicationPrefix>|<key>|<val>|<key>|<val>|...and so on |
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.
And so on:
|service-name||(||)*
bind(&ServiceDiscovery::onData, this, _1, _2), | ||
bind(&ServiceDiscovery::onTimeout, this, _1), | ||
bind(&ServiceDiscovery::onTimeout, this, _1)); | ||
|
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.
Extra line.
|
||
template<ndn::encoding::Tag TAG> | ||
size_t | ||
ServiceDiscovery::wireEncode(ndn::EncodingImpl<TAG>& encoder, |
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.
Never seen a wireEncode like this. Usually info and status comes from a class member.
If this is the case then you are right, in this program having m_wire as a class member does not make sense either.
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.
Yes, this wireEncode is a bit weird. I kept m_wire member variable because I need to return wire from a wireEncode. If wire is not a member variable, it will throw "a reference to stack memory associated with local variable 'wire' returned". So, either I have to dynamically allocate memory for "wire" or pass wire to wireEncode from callee, and I don't like both of these solutions.
What do you suggest?
@brief Process flags send by consumer and producer application. | ||
*/ | ||
uint8_t | ||
processFalgs(const std::map<char, uint8_t>& flags, const char type); |
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.
Type Flags
wscript
Outdated
Copyright (c) 2014-2019, The University of Memphis, | ||
Regents of the University of California, | ||
Arizona Board of Regents. | ||
Copyright (c) 2014-2019, The University of Memphis |
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.
Copyright year
wscript
Outdated
Copyright (c) 2014-2019, The University of Memphis, | ||
Regents of the University of California, | ||
Arizona Board of Regents. | ||
Copyright (c) 2014-2019, The University of Memphis | ||
|
||
This file is part of NLSR (Named-data Link State Routing). |
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.
Perhaps not true.
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.
Overall seems okay to me. Some confusion in ServiceDiscovery wireEncode/Decode.
Don't forget to push to appropriate branch your changes (so I am able to see diff here).
std::string serviceName; | ||
int contFlag = -1; | ||
|
||
namespace po = boost::program_options; |
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.
Why use boost::program_options here instead of just usual c++?
|
||
try | ||
{ | ||
std::cout << "Fetching service info for: " << serviceName << std::endl; |
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.
Redundant?
std::cerr << "ERROR: " << e.what() << std::endl; | ||
usage(visibleOptDesc); | ||
} | ||
// TODO: protocol shouldn't be hard-coded. | ||
std::map<char, uint8_t> flags; |
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.
What I meant was to replay this std::map with an options class such that you pass that to your discovery stuff. Where are the new changes?
@@ -63,14 +66,16 @@ int | |||
main(int argc, char* argv[]) | |||
{ | |||
std::map<char, uint8_t> flags; | |||
flags.insert(std::pair<char, uint8_t>('p', ndnsd::SYNC_PROTOCOL_CHRONOSYNC)); //protocol choice | |||
flags.insert(std::pair<char, uint8_t>('p', ndnsd::SYNC_PROTOCOL_PSYNC)); //protocol choice |
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.
still hard coded protocol choice?
@@ -19,6 +19,12 @@ | |||
|
|||
#include "file-processor.hpp" | |||
|
|||
#include<iostream> |
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.
Space before <
#include "ndnsd/discovery/service-discovery.hpp" | ||
#include <ndn-cxx/util/logger.hpp> | ||
|
||
#include<iostream> |
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.
Space before <
#include "ndnsd/discovery/service-discovery.hpp" | ||
#include <ndn-cxx/util/logger.hpp> | ||
|
||
#include<iostream> | ||
#include <list> |
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.
Where is list used?
// reload file. | ||
m_fileProcessor.processFile(); | ||
setUpdateProducerState(true); | ||
|
||
// if change is detected, call doUpdate to notify sync about the update | ||
doUpdate(m_producerState.applicationPrefix); | ||
// send back the response | ||
static const std::string content("Update Successful"); |
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.
On second thoughts, just send an empty data here. Why even set a content?
catch (const std::exception& ex) { | ||
std::cerr << ex.what() << std::endl; | ||
} | ||
auto dataContent = makeDataContent(); |
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.
Perhaps move the content of makeDataContent to wireEncode?
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.
So that you can remove arguments from wireEncode and get the status and make the content within it. Simpliyfing this function.
Also if status and data has not changed then the old m_wire should suffice.
So probably status can be a member of the class.
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.
yes, this can be done.
@@ -343,8 +370,8 @@ ServiceDiscovery::wireDecode(const ndn::Block& wire) | |||
|
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.
If this wireDecode function just returns the consumerReply it has nothing to do with this class. Make it a free/static function. Why are you saving the m_wire - what do you gain out of it? You are not initializing any other class members...
I guess question is how is this wireDecode used? As far as I can tell it is static.
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 have made wireDecode static for now. It can be static as long as we find a better use case.
@@ -248,6 +255,7 @@ class ServiceDiscovery | |||
uint8_t m_counter; | |||
|
|||
uint32_t m_syncProtocol; | |||
uint32_t m_continuousDiscovery; |
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.
What does this mean?
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.
Its a flag specific to a consumer application, if set, will not kill consumer application after fetching service info and so consumer will keep listening for future updates.
@@ -4,7 +4,7 @@ required | |||
{ | |||
serviceName printer | |||
appPrefix /printer2/service-info | |||
lifetime 100 | |||
lifetime 20 |
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.
Probably comment is required whether seconds, ms?
Also have a test.info.sample and don't modify that file in commits.
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.
done.
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.
Need to address how wireDecode is used.
Usual way is to have something like this to initialize a class and not return a class:
MyClass(Block block)
: wireDecode(block)
…e modified the callbacks