From 3e31da0016795397bfeacb2f3d76ecfe35cc1f67 Mon Sep 17 00:00:00 2001 From: Ken Gaillot Date: Mon, 17 Jul 2023 14:52:42 -0500 Subject: [PATCH] Fix: libcrmcommon: wait for reply from appropriate controller commands ipc_controld.c:reply_expected() wrongly omitted PCMK__CONTROLD_CMD_NODES (which hasn't been a problem because crm_node uses a mainloop instead of sync dispatch for that) and CRM_OP_RM_NODE_CACHE (which can be sent via ipc_client.c:pcmk_ipc_purge_node()). Because CRM_OP_RM_NODE_CACHE gets only an ack and no further replies, we now have to be careful not to return true from the controller's dispatch() function, otherwise crm_node -R would wait forever for more data. That means we have to check for whether any replies are expected, which means we have to increment expected replies *before* sending a request (in case it's sync). Regression introduced in 2.0.5 by ae14fa4a Fixes T681 --- lib/common/ipc_controld.c | 49 ++++++++++++++------------------------- 1 file changed, 17 insertions(+), 32 deletions(-) diff --git a/lib/common/ipc_controld.c b/lib/common/ipc_controld.c index 3c3a98964..405fd0518 100644 --- a/lib/common/ipc_controld.c +++ b/lib/common/ipc_controld.c @@ -177,18 +177,16 @@ set_nodes_data(pcmk_controld_api_reply_t *data, xmlNode *msg_data) static bool reply_expected(pcmk_ipc_api_t *api, xmlNode *request) { - const char *command = crm_element_value(request, F_CRM_TASK); - - if (command == NULL) { - return false; - } - - // We only need to handle commands that functions in this file can send - return !strcmp(command, CRM_OP_REPROBE) - || !strcmp(command, CRM_OP_NODE_INFO) - || !strcmp(command, CRM_OP_PING) - || !strcmp(command, CRM_OP_LRM_FAIL) - || !strcmp(command, CRM_OP_LRM_DELETE); + // We only need to handle commands that API functions can send + return pcmk__str_any_of(crm_element_value(request, F_CRM_TASK), + PCMK__CONTROLD_CMD_NODES, + CRM_OP_LRM_DELETE, + CRM_OP_LRM_FAIL, + CRM_OP_NODE_INFO, + CRM_OP_PING, + CRM_OP_REPROBE, + CRM_OP_RM_NODE_CACHE, + NULL); } static bool @@ -202,22 +200,12 @@ dispatch(pcmk_ipc_api_t *api, xmlNode *reply) pcmk_controld_reply_unknown, NULL, NULL, }; - /* If we got an ACK, return true so the caller knows to expect more responses - * from the IPC server. We do this before decrementing replies_expected because - * ACKs are not going to be included in that value. - * - * Note that we cannot do the same kind of status checking here that we do in - * ipc_pacemakerd.c. The ACK message we receive does not necessarily contain - * a status attribute. That is, we may receive this: - * - * - * - * Instead of this: - * - * - */ if (pcmk__str_eq(crm_element_name(reply), "ack", pcmk__str_none)) { - return true; // More replies needed + /* ACKs are trivial responses that do not count toward expected replies, + * and do not have all the fields that validation requires, so skip that + * processing. + */ + return private->replies_expected > 0; } if (private->replies_expected > 0) { @@ -344,18 +332,15 @@ static int send_controller_request(pcmk_ipc_api_t *api, xmlNode *request, bool reply_is_expected) { - int rc; - if (crm_element_value(request, XML_ATTR_REFERENCE) == NULL) { return EINVAL; } - rc = pcmk__send_ipc_request(api, request); - if ((rc == pcmk_rc_ok) && reply_is_expected) { + if (reply_is_expected) { struct controld_api_private_s *private = api->api_data; private->replies_expected++; } - return rc; + return pcmk__send_ipc_request(api, request); } static xmlNode * -- 2.41.0