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

every parts are wroking, implemented ndnsd-tool completly and also th… #2

Open
wants to merge 44 commits into
base: test
Choose a base branch
from

Conversation

dulalsaurab
Copy link
Owner

…e modified the callbacks

// send back the response
static const std::string content("Update Successful");
// Create Data packet
auto data = make_shared<ndn::Data>(interest.getName());
Copy link
Collaborator

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.

Copy link
Owner Author

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);
Copy link
Collaborator

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));

Copy link
Owner Author

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);
Copy link
Collaborator

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

Copy link
Owner Author

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
Copy link
Collaborator

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
Copy link
Collaborator

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));

Copy link
Collaborator

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,
Copy link
Collaborator

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.

Copy link
Owner Author

@dulalsaurab dulalsaurab Jun 3, 2020

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);
Copy link
Collaborator

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
Copy link
Collaborator

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).
Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps not true.

Copy link
Collaborator

@agawande agawande left a 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;
Copy link
Collaborator

@agawande agawande Jun 4, 2020

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;
Copy link
Collaborator

@agawande agawande Jun 4, 2020

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;
Copy link
Collaborator

@agawande agawande Jun 4, 2020

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
Copy link
Collaborator

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>
Copy link
Collaborator

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>
Copy link
Collaborator

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>
Copy link
Collaborator

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");
Copy link
Collaborator

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();
Copy link
Collaborator

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?

Copy link
Collaborator

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.

Copy link
Owner Author

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)

Copy link
Collaborator

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.

Copy link
Owner Author

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;
Copy link
Collaborator

Choose a reason for hiding this comment

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

What does this mean?

Copy link
Owner Author

@dulalsaurab dulalsaurab Jun 4, 2020

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
Copy link
Collaborator

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.

Copy link
Owner Author

Choose a reason for hiding this comment

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

done.

Copy link
Collaborator

@agawande agawande left a 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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants