NFD

See original complaint in #2993 note-16.

FaceManager Channel logs can be improved easily, and FaceTable warning can be eliminated easily.

nfdc's indistinguishable log is a deeper problem: in FaceMgmt protocol, the response does not indicate whether the face is existing or newly created.

The straightforward solution is to use different StatusCodes: 201 for new face, 202 for changing properties on existing face.
But I feel this solution is too limited:

• Semantically, creating face and changing properties are two different operations.
• There's still no way to change the persistency on permanent -> persistent -> on-demand direction. Although it's less useful, this operation should be allowed.
  A use case is: a core link on an overlay topology was created as a permanent face; due to a topology change, this link is no longer part of the topology. Instead of destroying the face, it's better to change the persistency to on-demand, so that pending Interests on this face can still be satisfied.
• There's no way to change the persistency setting of a face that cannot be created by faces/create command.

A better solution is:

• faces/create command considers static properties, such as persistency, only for creating new face.
• faces/create command does not change static properties on an existing face, but will return StatusCode 409 (conflict), with ControlParameters struct that describes the properties of the existing face.
• A separate faces/update command can update static properties on an existing face. It requires FaceId parameter.

20151013 conference call, Alex approves note-1 design.

NFD code is finished, but to make it work and to enable unit-tests can pass, I think we need to update ndn-cxx first. For at least two reasons:

1) we must add support for FaceScope and LinkType in ndn::nfd::ControlParameters.

2) we must add request validation logics for FaceUpdate command in ndn::nfd::ControlCommand.

So, shall I add a separate commit for above updates first?

we need to update ndn-cxx first

Of course. It should be a commit on ndn-cxx repository, under the same Redmine issue.

ndn-cxx and NFD commits should be uploaded together. NFD commit will initially have a build failure, but that's okay.

I notice https://gerrit.named-data.net/3012 patchset4 introduces an nfdc update command with questionable arguments.
This is not right. This issue refs #3232 only needs to solve the inaccurate log message problem, and should otherwise maintain backwards compatibility on the semantics of nfdc create command. The logic for nfdc create command should be:

1. invoke faces/create command
2. if a new face is created, report successful face creation then exit
3. if an existing face is found, check whether persistency setting is greater than or equal to the requested persistency; if so, report face already exists then exit
4. invoke faces/update command to upgrade (but not downgrade) face persistency (from persistent to permanent)
5. if upgrading is successful, report successful persistency upgrading then exit
6. if step1 or step4 fails, report the error

I'm confused about two issues:

1/ What are valid transitions?

from note-1 I know both onDemand -> persistent -> permanent and permanent -> persistent -> onDemand are valid.

but note-12 indicates only one way to update, namely persistent -> permanent.

2/ Where to check the validation of persistency transitions?

I think it should be performed in FaceManager.

FaceManager allows all transitions. It does not check transitions, but must catch exception when invoking setPersistency because each Transport defines what transitions are permitted.

nfdc create only allows on-demand => persistent, on-demand => permanent, persistent => permanent.

There's no nfdc update command.

There is a design issue need to address:

Currently, the validation of changing persistency will not be checked before setting persistency.
If we detect other invalid parameters after setting persistency, we have to roll back the setting. While such rolling-back is unsafe.

Here is an potential solution:
1/ expose an interface (something like validateChaningPersistency) in the transport class to check the validation of changing persistency (only simple checks are performed here).
2/ make beforeChaningPersistency call validateChaningPersistency to perform the required check, leaving back any other operations (e.g. changing the status).
3/ expose an interface in the Face class that calls transport::validateChaningPersistency to perform the required check.
4/ in updateCommand, we just need to call Face::validateChangingPersistency instead of Face::setPersistency to check the validation of parameters. And then call Face::setPersistency after all parameters are validated.

I agree with note-22 idea but disagree with naming and requiring beforeChangingPersistency to call validateChangingPersistency. I suggest the following:

class Transport
{
public:
  bool
  canChangePersistencyTo(FacePersistency newPersistency) const;
  // This function invokes canChangePersistencyToImpl if newPersistency differs from current persistency.
  // The reason for separation is single responsibility principle: this function serves as external API, while Impl is internal.

  void
  setPersistency(FacePersistency newPersistency);
  // 1. assert(canChangePersistencyTo(newPersistency))
  // 2. change the persistency
  // 3. invoke afterChangePersistency

private:
  virtual bool
  canChangePersistencyToImpl(FacePersistency newPersistency) const;
  // This function can safely assume newPersistency differs from current persistency.
  // Base class implementation returns true.

  virtual void
  afterChangePersistency(FacePersistency oldPersistency);
  // Instead of beforeChangePersistency, this is morphed to afterChangePersistency
  // to clarify that this function should not check whether the persistency change
  // should be performed, but should focus on updating the internal state after
  // the persistency change is already performed.
  // Base class implementation does nothing.
};

I agree with note-23.

Another question: shall I submit a new commit? or leaving it within the current one?

Another question: shall I submit a new commit? or leaving it within the current one?

It's always preferable to use smaller commits, which are easier to review.