NFD » ndn-cxx

When retrieving a public key of certificate from PIB, if the requested object does not exist, return a nullptr rather than throwing an exception.

When retrieving the default setting from PIB (e.g., default key name or default certificate name), return an empty name rather than throwing an exception.

When checking the existence of a key or a certificate in PIB, if the search name is malformed, return false rather than throwing an exception.

When signing a packet, if the required key or certificate does not exist, sign packets using sha-256 rather than throw an exception.

When retrieving a public key of certificate from PIB, if the requested object does not exist, return a nullptr rather than throwing an exception.

This is reasonable.

When retrieving the default setting from PIB (e.g., default key name or default certificate name), return an empty name rather than throwing an exception.

This is ambiguous because ndn:/ is a valid identity Name.

A better approach is changing return type to shared_ptr<Name> or std::pair<bool, Name>, and explicitly indicate the non-existence of a setting by returning nullptr or {false, ignored}.

When checking the existence of a key or a certificate in PIB, if the search name is malformed, return false rather than throwing an exception.

This looks okay.

When signing a packet, if the required key or certificate does not exist, sign packets using sha-256 rather than throw an exception.

This is dangerous. The caller expects the packet to be signed with a strong signature when identity isn't set to #1705 special value, but the library fails to deliver this result.

For the second change, since key name and certificate name can never be empty, therefore, it's safe to use an empty name as the non-existing result.

For the fourth change, I can revert it back to have throw exception, if the explicitly specified key or certificate does not exist.

For the second change, since key name and certificate name can never be empty, therefore, it's safe to use an empty name as the non-existing result.

I'm specifically talking about KeyChain::getDefaultIdentity(). ndn:/ is a valid identity.

In any case, this is a potentially breaking change because it changes the semantics of these methods. Once the design is finalized, send a notice to nfd-dev and ndn-lib, and give 7 days for dependent projects to make necessary changes.

For that particular one, I did not change the behavior (note that I only mentioned key and certificate, not identity in this case), if there is no default identity, error will still be thrown.

I would like to send the notification a little bit later (to include the changes on validation callback), so that people do not have to change their code twice.

This is a breaking change that needs a 5-day notice. Given that there are more related breaking changes, I suggest creating a separate branch for all these breaking changes.

PIB API is also in need of some refactoring:

• [out] vectors shall be moved to return value, because C++11 supports move semantics on vectors, so returning a vector is no longer inefficient.
• Most "public virtual" methods should be avoided. Instead, declare a public non-virtual method that invokes a private virtual method, to separate external API from internal structure. See http://www.spatial.com/blog/public-virtual-methods-bad-idea

• Declarations shall be reordered:

  public:
    constructors
    methods that don't fit in any category below
  public: // identity
    doesIdentityExist
    getDefaultIdentity
    addIdentity
    removeIdentity
    setDefaultIdentity
  public: // key
    ..
  public: // certificate
    ..
  

Given there could a lot of API changes (not only this handling and those Junixiao mentioned, but also some other changes I would like to make), I agree it would be better to open a new branch for this API change. I will break the gerrit commit http://gerrit.named-data.net/#/c/1536/ into two, the one for sha-256 identity will be left in master branch, the rest (for exception handling) will be moved to the API branch.