Project

General

Profile

Actions

Bug #4944

open

ndnputfile does not work

Added by Chavoosh Ghasemi over 5 years ago. Updated about 1 year ago.

Status:
New
Priority:
High
Assignee:
-
Target version:
-
Start date:
Due date:
% Done:

0%

Estimated time:

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

Related issues 1 (1 open0 closed)

Related to repo-ng - Feature #4129: Management Dispatcher for repo commandsNewMuktadir Chowdhury

Actions
Actions #1

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.

Actions #2

Updated by Saurab Dulal about 5 years ago

Additionally, I have found the following problems

  1. 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"

  2. 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?

Actions #3

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.

Actions #4

Updated by Davide Pesavento about 5 years ago

  • Assignee set to Saurab Dulal
  • Start date deleted (05/29/2019)
Actions #5

Updated by Davide Pesavento about 5 years ago

Saurab volunteered to take this.

Actions #6

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.

Actions #7

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?

Actions #8

Updated by Saurab Dulal about 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.

Actions #9

Updated by Davide Pesavento about 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...

Actions #10

Updated by Ashlesh Gawande about 5 years ago

  • Related to Feature #4129: Management Dispatcher for repo commands added
Actions #11

Updated by Saurab Dulal about 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

Actions #12

Updated by Davide Pesavento about 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.

Actions #13

Updated by Davide Pesavento about 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...

Actions #14

Updated by Davide Pesavento about 1 year ago

  • Assignee deleted (Saurab Dulal)
  • Priority changed from Normal to High
Actions

Also available in: Atom PDF