From 5851524d7a2bff6df9873ca51c8f3790ebd7a03b Mon Sep 17 00:00:00 2001 From: Abtin Keshavarzian Date: Fri, 13 Aug 2021 18:14:57 -0700 Subject: [PATCH] [srp-client] add `aSendUnregToServer` param to `RemoveHostAndServices()` (#6927) This commit adds a new parameter `aSendUnregToServer` to SRP client's `RemoveHostAndServices()` API. This parameter determines the behavior when the host info is not yet registered with the server. If it is set to `false` (which is the default/expected value) then the SRP client will immediately remove the host info and services without sending an update message to server (no need to update the server if nothing is yet registered with it). If it is set to `true` then the SRP client will send an update message to the server. Note that if the host info is registered then the value of `aSendUnregToServer` does not matter and the SRP client will always send an update message to the server requesting removal of all info. One situation where this parameter can be useful is on a device reset/reboot where the caller may want to remove any previously registered services with the server. In this case, caller can `SetHostName()` and then request `RemoveHostAndServices()` with `aSendUnregToServer` as `true`. This commit also adds `test_srp_client_remove_host.py` which verifies the behavior the newly added mechanism between client and server. --- include/openthread/instance.h | 2 +- include/openthread/srp_client.h | 22 ++- src/cli/README_SRP_CLIENT.md | 7 +- src/cli/cli_srp_client.cpp | 12 +- src/core/api/srp_client_api.cpp | 4 +- src/core/net/srp_client.cpp | 18 +- src/core/net/srp_client.hpp | 20 +- src/lib/spinel/spinel.h | 3 +- src/ncp/ncp_base_mtd.cpp | 4 +- tests/scripts/thread-cert/Makefile.am | 2 + tests/scripts/thread-cert/node.py | 4 +- .../test_srp_client_remove_host.py | 175 ++++++++++++++++++ 12 files changed, 244 insertions(+), 29 deletions(-) create mode 100755 tests/scripts/thread-cert/test_srp_client_remove_host.py diff --git a/include/openthread/instance.h b/include/openthread/instance.h index 9de34c640..62cae648a 100644 --- a/include/openthread/instance.h +++ b/include/openthread/instance.h @@ -53,7 +53,7 @@ extern "C" { * @note This number versions both OpenThread platform and user APIs. * */ -#define OPENTHREAD_API_VERSION (155) +#define OPENTHREAD_API_VERSION (156) /** * @addtogroup api-instance diff --git a/include/openthread/srp_client.h b/include/openthread/srp_client.h index fda9f435f..ba7801e34 100644 --- a/include/openthread/srp_client.h +++ b/include/openthread/srp_client.h @@ -180,8 +180,8 @@ typedef void (*otSrpClientCallback)(otError aError, * This callback is invoked when auto-start mode is enabled and the SRP client is either automatically started or * stopped. * - * @param[in] aSeverSockAddress A non-NULL pointer indicates SRP sever was started and pointer will give the - * selected server socket address. A NULL pointer indicates SRP sever was stopped. + * @param[in] aServerSockAddress A non-NULL pointer indicates SRP server was started and pointer will give the + * selected server socket address. A NULL pointer indicates SRP server was stopped. * @param[in] aContext A pointer to an arbitrary context (provided when callback was registered). * */ @@ -515,15 +515,27 @@ const otSrpClientService *otSrpClientGetServices(otInstance *aInstance); * that the server holds the host name in reserve for when the client is once again able to provide and register its * service(s). * - * @param[in] aInstance A pointer to the OpenThread instance. - * @param[in] aRemoveKeyLease A boolean indicating whether or not the host key lease should also be removed. + * The @p aSendUnregToServer determines the behavior when the host info is not yet registered with the server. If + * @p aSendUnregToServer is set to `false` (which is the default/expected value) then the SRP client will immediately + * remove the host info and services without sending an update message to server (no need to update the server if + * nothing is yet registered with it). If @p aSendUnregToServer is set to `true` then the SRP client will send an + * update message to the server. Note that if the host info is registered then the value of @p aSendUnregToServer does + * not matter and the SRP client will always send an update message to server requesting removal of all info. + * + * One situation where @p aSendUnregToServer can be useful is on a device reset/reboot, caller may want to remove any + * previously registered services with the server. In this case, caller can `otSrpClientSetHostName()` and then request + * `otSrpClientRemoveHostAndServices()` with `aSendUnregToServer` as `true`. + * + * @param[in] aInstance A pointer to the OpenThread instance. + * @param[in] aRemoveKeyLease A boolean indicating whether or not the host key lease should also be removed. + * @param[in] aSendUnregToServer A boolean indicating whether to send update to server when host info is not registered. * * @retval OT_ERROR_NONE The removal of host info and services started successfully. The `otSrpClientCallback` * will be called to report the status. * @retval OT_ERROR_ALREADY The host info is already removed. * */ -otError otSrpClientRemoveHostAndServices(otInstance *aInstance, bool aRemoveKeyLease); +otError otSrpClientRemoveHostAndServices(otInstance *aInstance, bool aRemoveKeyLease, bool aSendUnregToServer); /** * This function clears all host info and all the services. diff --git a/src/cli/README_SRP_CLIENT.md b/src/cli/README_SRP_CLIENT.md index 40d0c599b..5cbe94fac 100644 --- a/src/cli/README_SRP_CLIENT.md +++ b/src/cli/README_SRP_CLIENT.md @@ -201,9 +201,12 @@ The possible states are (same value for service state): ### host remove -Usage: `srp client host remove [removekeylease]` +Usage: `srp client host remove [removekeylease] [sendunregtoserver]` -Remove host info and all services from server. `removekeylease` is boolean value indicating whether or not the host key lease should also be removed +Remove host info and all services from server. + +- `removekeylease` is an optional boolean value indicating whether or not the host key lease should also be removed (default is false). +- `sendunregtoserver` is a another optional boolean value indicating whether or not to send an update message to the server when host info is not yet registered (default is false). ```bash > srp client host remove 1 diff --git a/src/cli/cli_srp_client.cpp b/src/cli/cli_srp_client.cpp index 3608c58fe..7703d3b31 100644 --- a/src/cli/cli_srp_client.cpp +++ b/src/cli/cli_srp_client.cpp @@ -238,15 +238,21 @@ otError SrpClient::ProcessHost(Arg aArgs[]) } else if (aArgs[0] == "remove") { - bool removeKeyLease = false; + bool removeKeyLease = false; + bool sendUnregToServer = false; if (!aArgs[1].IsEmpty()) { SuccessOrExit(error = aArgs[1].ParseAsBool(removeKeyLease)); - VerifyOrExit(aArgs[2].IsEmpty(), error = OT_ERROR_INVALID_ARGS); + + if (!aArgs[2].IsEmpty()) + { + SuccessOrExit(error = aArgs[2].ParseAsBool(sendUnregToServer)); + VerifyOrExit(aArgs[3].IsEmpty(), error = OT_ERROR_INVALID_ARGS); + } } - error = otSrpClientRemoveHostAndServices(mInterpreter.mInstance, removeKeyLease); + error = otSrpClientRemoveHostAndServices(mInterpreter.mInstance, removeKeyLease, sendUnregToServer); } else if (aArgs[0] == "clear") { diff --git a/src/core/api/srp_client_api.cpp b/src/core/api/srp_client_api.cpp index 6bd17f4ea..979fdcd05 100644 --- a/src/core/api/srp_client_api.cpp +++ b/src/core/api/srp_client_api.cpp @@ -179,11 +179,11 @@ const otSrpClientService *otSrpClientGetServices(otInstance *aInstance) return instance.Get().GetServices().GetHead(); } -otError otSrpClientRemoveHostAndServices(otInstance *aInstance, bool aRemoveKeyLease) +otError otSrpClientRemoveHostAndServices(otInstance *aInstance, bool aRemoveKeyLease, bool aSendUnregToServer) { Instance &instance = *static_cast(aInstance); - return instance.Get().RemoveHostAndServices(aRemoveKeyLease); + return instance.Get().RemoveHostAndServices(aRemoveKeyLease, aSendUnregToServer); } void otSrpClientClearHostAndServices(otInstance *aInstance) diff --git a/src/core/net/srp_client.cpp b/src/core/net/srp_client.cpp index 1f7d5e287..987d452e5 100644 --- a/src/core/net/srp_client.cpp +++ b/src/core/net/srp_client.cpp @@ -501,7 +501,7 @@ exit: return error; } -Error Client::RemoveHostAndServices(bool aShouldRemoveKeyLease) +Error Client::RemoveHostAndServices(bool aShouldRemoveKeyLease, bool aSendUnregToServer) { Error error = kErrorNone; @@ -523,7 +523,7 @@ Error Client::RemoveHostAndServices(bool aShouldRemoveKeyLease) UpdateServiceStateToRemove(service); } - if (mHostInfo.GetState() == kToAdd) + if ((mHostInfo.GetState() == kToAdd) && !aSendUnregToServer) { // Host info is not added yet (not yet registered with // server), so we can remove it and all services immediately. @@ -1260,7 +1260,7 @@ void Client::ProcessResponse(Message &aMessage) offset += sizeof(header); // Skip over all sections till Additional Data section - // SPEC ENHANCEMENT: Sever can echo the request back or not + // SPEC ENHANCEMENT: Server can echo the request back or not // include any of RRs. Would be good to explicitly require SRP server // to not echo back RRs. @@ -1446,7 +1446,7 @@ void Client::UpdateState(void) bool shouldUpdate = false; VerifyOrExit((GetState() != kStateStopped) && (GetState() != kStatePaused)); - VerifyOrExit((mHostInfo.GetName() != nullptr) && (mHostInfo.GetNumAddresses() > 0)); + VerifyOrExit(mHostInfo.GetName() != nullptr); // Go through the host info and all the services to check if there // are any new changes (i.e., anything new to add or remove). This @@ -1474,11 +1474,11 @@ void Client::UpdateState(void) case kToAdd: case kToRefresh: - // Make sure we have at least one service otherwise no need to - // send SRP update message with host info only. The exception - // is when removing host info where we allow for empty - // service list. - VerifyOrExit(!mServices.IsEmpty()); + // Make sure we have at least one service and at least one + // host address, otherwise no need to send SRP update message. + // The exception is when removing host info where we allow + // for empty service list. + VerifyOrExit(!mServices.IsEmpty() && (mHostInfo.GetNumAddresses() > 0)); // Fall through diff --git a/src/core/net/srp_client.hpp b/src/core/net/srp_client.hpp index 87eeddf64..df5271878 100644 --- a/src/core/net/srp_client.hpp +++ b/src/core/net/srp_client.hpp @@ -556,7 +556,7 @@ public: /** * This method starts the remove process of the host info and all services. * - * After retuning from this method, `Callback` will be called to report the status of remove request with + * After returning from this method, `Callback` will be called to report the status of remove request with * SRP server. * * If the host info is to be permanently removed from server, @p aRemoveKeyLease should be set to `true` which @@ -564,14 +564,28 @@ public: * ensures that the server holds the host name in reserve for when the client once again able to provide and * register its service(s). * - * @param[in] aRemoveKeyLease A boolean indicating whether or not the host key lease should also be removed. + * The @p aSendUnregToServer determines the behavior when the host info is not yet registered with the server. If + * @p aSendUnregToServer is set to `false` (which is the default/expected value) then the SRP client will + * immediately remove the host info and services without sending an update message to server (no need to update the + * server if nothing is yet registered with it). If @p aSendUnregToServer is set to `true` then the SRP client will + * send an update message to the server. Note that if the host info is registered then the value of + * @p aSendUnregToServer does not matter and the SRP client will always send an update message to server requesting + * removal of all info. + * + * One situation where @p aSendUnregToServer can be useful is on a device reset/reboot, caller may want to remove + * any previously registered services with the server. In this case, caller can `SetHostName()` and then request + * `RemoveHostAndServices()` with `aSendUnregToServer` as `true`. + * + * @param[in] aRemoveKeyLease A boolean indicating whether or not the host key lease should also be removed. + * @param[in] aSendUnregToServer A boolean indicating whether to send update to server when host info is not + * registered. * * @retval kErrorNone The removal of host and services started successfully. The `Callback` will be called * to report the status. * @retval kErrorAlready The host is already removed. * */ - Error RemoveHostAndServices(bool aShouldRemoveKeyLease); + Error RemoveHostAndServices(bool aShouldRemoveKeyLease, bool aSendUnregToServer = false); /** * This method clears all host info and all the services. diff --git a/src/lib/spinel/spinel.h b/src/lib/spinel/spinel.h index ba2442e9c..19809976f 100644 --- a/src/lib/spinel/spinel.h +++ b/src/lib/spinel/spinel.h @@ -4137,7 +4137,7 @@ enum SPINEL_PROP_SRP_CLIENT_SERVICES = SPINEL_PROP_OPENTHREAD__BEGIN + 23, /// SRP Client Host And Services Remove - /** Format: `b` : Write only + /** Format: `bb` : Write only * Required capability: `SPINEL_CAP_SRP_CLIENT`. * * Writing to this property with starts the remove process of the host info and all services. @@ -4146,6 +4146,7 @@ enum * Format is: * * `b` : A boolean indicating whether or not the host key lease should also be cleared. + * `b` : A boolean indicating whether or not to send update to server when host info is not registered. * */ SPINEL_PROP_SRP_CLIENT_HOST_SERVICES_REMOVE = SPINEL_PROP_OPENTHREAD__BEGIN + 24, diff --git a/src/ncp/ncp_base_mtd.cpp b/src/ncp/ncp_base_mtd.cpp index 62d3046ea..4f03ec659 100644 --- a/src/ncp/ncp_base_mtd.cpp +++ b/src/ncp/ncp_base_mtd.cpp @@ -3932,10 +3932,12 @@ template <> otError NcpBase::HandlePropertySet