Bug #4944
openndnputfile does not work
0%
Description
I installed repo-ng
with the following commands:
./waf configure
./waf
sudo ./waf install
Then I ran ndn-repo-ng
with the following commands:
sudo cp /usr/local/etc/ndn/repo-ng.conf.sample /usr/local/etc/ndn/repo-ng.conf
sudo ndn-repo-ng
Then I tried to put a file into the repo by using the following command that comes from Wiki page:
echo "This is a test" > test.txt
$ndnputfile /example/repo/1 /example/data/1/ test.txt
However, I get an error that says:
ERROR: RepoCommandResponse malformed
Here are the commit IDs of each package while doing this test:
repo-ng: 5c16cc2f3
NFD: 47c343b9f
ndn-cxx: 2ad2fbeb2
Updated by Zhiyi Zhang over 5 years ago
We also observed the same issue.
We found that the error is raised by the RepoCommandReponse's wireDecode: the type is supposed to be 207 but which in fact is 101.
101 is the type number of the parent class of RepoCommandReponse -- ControlReponse, so the problem is actually caused by the class inheritance.
Yufeng Zhang from UCLA has been assigned to fix on this error.
Updated by Saurab Dulal about 5 years ago
Additionally, I have found the following problems
ndnputfile command doesn't append EndBlockID by default, which is causing command validation failure, and so the write-handle insertion is failing.
starting here, https://github.com/named-data/repo-ng/blob/master/src/handles/write-handle.cpp#L48
and finally here https://github.com/named-data/repo-ng/blob/master/src/handles/command-base-handle.hpp#L68, with following error messages "EndBlockId is required but missing"When a response is created inside processSegmentedInsertCommand (write-hanldle.cpp #L256, done(response) is called ----- this goes all the way to ndn-cxx Dispatcher::processControlCommandInterest. Now, interest component is assigned to cxx parameter (shared_ptr).
dispatcher.cpp #L175
// /<prefix>/<relPrefix>/<parameters>
size_t parametersLoc = prefix.size() + relPrefix.size();
const name::Component& pc = interest.getName().get(parametersLoc);
shared_ptr<ControlParameters> parameters;
try {
parameters = parser(pc);
}
catch (const tlv::Error&) {
return;
}
this creates a mismatch. And thus, everything gets messed up.
Probably, ndn-cxx -- Dispatcher::processControlCommandInterest(...) function should be Template?
Updated by Junxiao Shi about 5 years ago
Yufeng Zhang from UCLA has been assigned to fix on this error.
Please have this developer join this project and then update Assignee field.
Updated by Davide Pesavento about 5 years ago
- Assignee set to Saurab Dulal
- Start date deleted (
05/29/2019)
Updated by Ashlesh Gawande about 5 years ago
Current proposed solution: make wireEncode and wireDecode virtual in ControlResponse command.
Seems unacceptable due to being the only wire* functions to be virtual in the code - which is unexpected.
Write-handle calls CommandContinuation (done) in dispatcher through the handler. The done takes in a ControlResponse.
Repo-ng provides the done function a RepoCommandResponse (sub-class of ControlResponse). Since wireEncode is not virtual, when Dispatcher calls wireEncode it is called on the parent class. Why the child wireEncode is not called could be because of dynamic_cast in CommandBaseHandle::validateParameters (Saurab found this).
The wireEncode in ControlResponse encodes the body within it. The body is set to be the wireEncode of the RepoCommandResponse. So another solution could be to change wireDecode of RepoCommandResponse to be process the body of the response.
Updated by Davide Pesavento about 5 years ago
Ashlesh Gawande wrote:
Write-handle calls CommandContinuation (done) in dispatcher through the handler. The done takes in a ControlResponse.
Repo-ng provides the done function a RepoCommandResponse (sub-class of ControlResponse). Since wireEncode is not virtual, when Dispatcher calls wireEncode it is called on the parent class.
Correct.
Why the child wireEncode is not called could be because of dynamic_cast in CommandBaseHandle::validateParameters (Saurab found this).
Not following this part. What has validateParameters
got to do with ControlResponse?
The wireEncode in ControlResponse encodes the body within it. The body is set to be the wireEncode of the RepoCommandResponse. So another solution could be to change wireDecode of RepoCommandResponse to be process the body of the response.
But this encoding violates the protocol, doesn't it?
Updated by Saurab Dulal almost 5 years ago
Davide Pesavento wrote:
Ashlesh Gawande wrote:
Write-handle calls CommandContinuation (done) in dispatcher through the handler. The done takes in a ControlResponse.
Repo-ng provides the done function a RepoCommandResponse (sub-class of ControlResponse). Since wireEncode is not virtual, when Dispatcher calls wireEncode it is called on the parent class.Correct.
Why the child wireEncode is not called could be because of dynamic_cast in CommandBaseHandle::validateParameters (Saurab found this).
Not following this part. What has
validateParameters
got to do with ControlResponse?
Please forget about this, I was lost in something else.
The wireEncode in ControlResponse encodes the body within it. The body is set to be the wireEncode of the RepoCommandResponse. So another solution could be to change wireDecode of RepoCommandResponse to be process the body of the response.
But this encoding violates the protocol, doesn't it?
Yes, this encoding is violating the protocol.
tl;dr;
Here is a scenario of how the problem is occurring.
class Base
{
public:
void aa() const
{
cout << "this is from parent" << endl;
}
};
class Derived: public Base {
public:
void aa() const
{
cout << "This is from child" << endl;
}
};
static void f (const Base* a)
{
a->aa();
}
int main () {
Derived* d = new Derived;
f(d);
return 0;
}
The function "f" which is similar to the dispatcher, when get derived class pointer as the base class pointer will always call base class method. Dispatcher here is falsely assuming that the derived class method will be called, and this is creating the problem. This problem will persist for any other similar implementation. But unfortunately, I don't see any other classes inheriting ControlResponse except repo.
Actual code flow:
https://github.com/named-data/repo-ng/blob/master/src/handles/write-handle.cpp#L127 (sends RepoCommandResponse:: response) --- > https://github.com/named-data/ndn-cxx/blob/master/ndn-cxx/mgmt/dispatcher.cpp#L202 (gets as resp) ----> https://github.com/named-data/ndn-cxx/blob/master/ndn-cxx/mgmt/dispatcher.cpp#L218 (and here resp.wireEncode() calls base method).
And I don't know how to solve this problem without making the base method virtual.
Updated by Davide Pesavento almost 5 years ago
Saurab Dulal wrote:
Dispatcher here is falsely assuming that the derived class method will be called, and this is creating the problem.
Dispatcher is not assuming anything. It was simply not designed to support different "response" types, and ControlResponse was not designed to support inheritance. The mistake here was using this machinery in repo-ng with a custom type used as response.
But unfortunately, I don't see any other classes inheriting ControlResponse except repo.
Because ControlResponse was not designed to be inherited from.
And I don't know how to solve this problem without making the base method virtual.
Adding more template parameters will probably solve this, but it could get messy...
Updated by Ashlesh Gawande almost 5 years ago
- Related to Feature #4129: Management Dispatcher for repo commands added
Updated by Saurab Dulal almost 5 years ago
Davide Pesavento wrote:
Ashlesh Gawande wrote:
Write-handle calls CommandContinuation (done) in dispatcher through the handler. The done takes in a ControlResponse.
Repo-ng provides the done function a RepoCommandResponse (sub-class of ControlResponse). Since wireEncode is not virtual, when Dispatcher calls wireEncode it is called on the parent class.Correct.
I quite don't understand why the wire E/D are virtual on the control-parameters? It was argued earlier on the NFD call/gerrit that encodable types cannot be virtual.
https://github.com/named-data/ndn-cxx/blob/master/ndn-cxx/mgmt/control-parameters.hpp
Updated by Davide Pesavento almost 5 years ago
Saurab Dulal wrote:
I quite don't understand why the wire E/D are virtual on the control-parameters? It was argued earlier on the NFD call/gerrit that encodable types cannot be virtual.
https://github.com/named-data/ndn-cxx/blob/master/ndn-cxx/mgmt/control-parameters.hpp
That may simply be a historical mistake. As I mentioned in the call, similar mistakes have been made with other classes.
That being said, it's worth noting that mgmt::ControlParameters
is just an interface, not a complete type, it doesn't define any fields or anything like that, and concrete classes are supposed to derive directly from the abstract interface. That is quite different from the ControlResponse
situation and not as bad. We haven't considered this approach during the call, but I think I would not disagree if you wanted to follow it (assuming it's feasible), and it may be simpler than the template-based solution. Essentially you'd have an interface class for ControlResponse, and both NFD's ControlResponse and repo-ng's RepoCommandResponse would directly derive from it.
Updated by Davide Pesavento almost 5 years ago
Actually, you'd probably still have to add an additional template parameter to some Dispatcher
methods, in order to tell it what concrete class should be instantiated for the response...
Updated by Davide Pesavento about 1 year ago
- Assignee deleted (
Saurab Dulal) - Priority changed from Normal to High