From: Christian Hopps Date: Fri, 9 Jun 2023 15:03:54 +0000 (-0400) Subject: lib: mgmtd: make error handling more robust X-Git-Tag: base_9.1~364^2 X-Git-Url: https://git.puffer.fish/?a=commitdiff_plain;h=d694cd40f2cb566245ca2c7851730095bf90b1a8;p=mirror%2Ffrr.git lib: mgmtd: make error handling more robust Signed-off-by: Christian Hopps --- diff --git a/lib/mgmt_be_client.c b/lib/mgmt_be_client.c index 5c875204f7..62b013a03f 100644 --- a/lib/mgmt_be_client.c +++ b/lib/mgmt_be_client.c @@ -217,14 +217,16 @@ static void mgmt_be_cleanup_all_batches(struct mgmt_be_txn_ctx *txn) } static struct mgmt_be_txn_ctx * -mgmt_be_find_txn_by_id(struct mgmt_be_client *client_ctx, uint64_t txn_id) +mgmt_be_find_txn_by_id(struct mgmt_be_client *client_ctx, uint64_t txn_id, + bool warn) { struct mgmt_be_txn_ctx *txn = NULL; - FOREACH_BE_TXN_IN_LIST (client_ctx, txn) { + FOREACH_BE_TXN_IN_LIST (client_ctx, txn) if (txn->txn_id == txn_id) return txn; - } + if (warn) + MGMTD_BE_CLIENT_ERR("Unknown txn-id: %" PRIu64, txn_id); return NULL; } @@ -234,20 +236,21 @@ mgmt_be_txn_create(struct mgmt_be_client *client_ctx, uint64_t txn_id) { struct mgmt_be_txn_ctx *txn = NULL; - txn = mgmt_be_find_txn_by_id(client_ctx, txn_id); - if (!txn) { - txn = XCALLOC(MTYPE_MGMTD_BE_TXN, - sizeof(struct mgmt_be_txn_ctx)); - assert(txn); + txn = mgmt_be_find_txn_by_id(client_ctx, txn_id, false); + if (txn) { + MGMTD_BE_CLIENT_ERR("Can't create existing txn-id: %" PRIu64, + txn_id); + return NULL; + } - txn->txn_id = txn_id; - txn->client = client_ctx; - mgmt_be_batches_init(&txn->cfg_batches); - mgmt_be_batches_init(&txn->apply_cfgs); - mgmt_be_txns_add_tail(&client_ctx->txn_head, txn); + txn = XCALLOC(MTYPE_MGMTD_BE_TXN, sizeof(struct mgmt_be_txn_ctx)); + txn->txn_id = txn_id; + txn->client = client_ctx; + mgmt_be_batches_init(&txn->cfg_batches); + mgmt_be_batches_init(&txn->apply_cfgs); + mgmt_be_txns_add_tail(&client_ctx->txn_head, txn); - MGMTD_BE_CLIENT_DBG("Added new txn-id: %" PRIu64, txn_id); - } + MGMTD_BE_CLIENT_DBG("Created new txn-id: %" PRIu64, txn_id); return txn; } @@ -297,7 +300,7 @@ static void mgmt_be_cleanup_all_txns(struct mgmt_be_client *client_ctx) } static int mgmt_be_send_txn_reply(struct mgmt_be_client *client_ctx, - uint64_t txn_id, bool create, bool success) + uint64_t txn_id, bool create) { Mgmtd__BeMessage be_msg; Mgmtd__BeTxnReply txn_reply; @@ -305,7 +308,7 @@ static int mgmt_be_send_txn_reply(struct mgmt_be_client *client_ctx, mgmtd__be_txn_reply__init(&txn_reply); txn_reply.create = create; txn_reply.txn_id = txn_id; - txn_reply.success = success; + txn_reply.success = true; mgmtd__be_message__init(&be_msg); be_msg.message_case = MGMTD__BE_MESSAGE__MESSAGE_TXN_REPLY; @@ -321,44 +324,29 @@ static int mgmt_be_process_txn_req(struct mgmt_be_client *client_ctx, { struct mgmt_be_txn_ctx *txn; - txn = mgmt_be_find_txn_by_id(client_ctx, txn_id); if (create) { - if (txn) { - /* - * Transaction with same txn-id already exists. - * Should not happen under any circumstances. - */ - MGMTD_BE_CLIENT_ERR( - "txn-id: %" PRIu64 " already exists", txn_id); - mgmt_be_send_txn_reply(client_ctx, txn_id, create, - false); - } + MGMTD_BE_CLIENT_DBG("Creating new txn-id %" PRIu64, txn_id); - MGMTD_BE_CLIENT_DBG("Created new txn-id %" PRIu64, txn_id); txn = mgmt_be_txn_create(client_ctx, txn_id); + if (!txn) + goto failed; if (client_ctx->cbs.txn_notify) - (void)(*client_ctx->cbs.txn_notify)( - client_ctx, client_ctx->user_data, - &txn->client_data, false); + (*client_ctx->cbs.txn_notify)(client_ctx, + client_ctx->user_data, + &txn->client_data, false); } else { - if (!txn) { - /* - * Transaction with same txn-id does not exists. - * Return sucess anyways. - */ - MGMTD_BE_CLIENT_DBG("txn-id: %" PRIu64 - " for delete does NOT exists", - txn_id); - } else { - MGMTD_BE_CLIENT_DBG("Delete txn-id: %" PRIu64, txn_id); + MGMTD_BE_CLIENT_DBG("Deleting txn-id: %" PRIu64, txn_id); + txn = mgmt_be_find_txn_by_id(client_ctx, txn_id, false); + if (txn) mgmt_be_txn_delete(client_ctx, &txn); - } } - mgmt_be_send_txn_reply(client_ctx, txn_id, create, true); + return mgmt_be_send_txn_reply(client_ctx, txn_id, create); - return 0; +failed: + msg_conn_disconnect(&client_ctx->client.conn, true); + return -1; } static int mgmt_be_send_cfgdata_create_reply(struct mgmt_be_client *client_ctx, @@ -610,25 +598,23 @@ static int mgmt_be_process_cfgdata_req(struct mgmt_be_client *client_ctx, { struct mgmt_be_txn_ctx *txn; - txn = mgmt_be_find_txn_by_id(client_ctx, txn_id); - if (!txn) { - MGMTD_BE_CLIENT_ERR("Invalid txn-id: %" PRIu64 - " from MGMTD server", - txn_id); - mgmt_be_send_cfgdata_create_reply( - client_ctx, txn_id, batch_id, false, - "Transaction context not created yet"); - } else { - mgmt_be_update_setcfg_in_batch(client_ctx, txn, batch_id, - cfg_req, num_req); - } + txn = mgmt_be_find_txn_by_id(client_ctx, txn_id, true); + if (!txn) + goto failed; + + mgmt_be_update_setcfg_in_batch(client_ctx, txn, batch_id, cfg_req, + num_req); if (txn && end_of_data) { - MGMTD_BE_CLIENT_DBG("Triggering CFG_PREPARE_REQ processing"); - mgmt_be_txn_cfg_prepare(txn); + MGMTD_BE_CLIENT_DBG("End of data; CFG_PREPARE_REQ processing"); + if (mgmt_be_txn_cfg_prepare(txn)) + goto failed; } return 0; +failed: + msg_conn_disconnect(&client_ctx->client.conn, true); + return -1; } static int mgmt_be_send_apply_reply(struct mgmt_be_client *client_ctx, @@ -731,23 +717,28 @@ static int mgmt_be_process_cfg_apply(struct mgmt_be_client *client_ctx, { struct mgmt_be_txn_ctx *txn; - txn = mgmt_be_find_txn_by_id(client_ctx, txn_id); - if (!txn) { - mgmt_be_send_apply_reply(client_ctx, txn_id, NULL, 0, false, - "Transaction not created yet!"); - return -1; - } + txn = mgmt_be_find_txn_by_id(client_ctx, txn_id, true); + if (!txn) + goto failed; MGMTD_BE_CLIENT_DBG("Trigger CFG_APPLY_REQ processing"); - mgmt_be_txn_proc_cfgapply(txn); + if (mgmt_be_txn_proc_cfgapply(txn)) + goto failed; return 0; +failed: + msg_conn_disconnect(&client_ctx->client.conn, true); + return -1; } + static int mgmt_be_client_handle_msg(struct mgmt_be_client *client_ctx, Mgmtd__BeMessage *be_msg) { /* + * On error we may have closed the connection so don't do anything with + * the client_ctx on return. + * * protobuf-c adds a max size enum with an internal, and changing by * version, name; cast to an int to avoid unhandled enum warnings */ @@ -888,6 +879,11 @@ static int _notify_conenct_disconnect(struct msg_client *msg_client, if (client->cbs.client_connect_notify) (void)(*client->cbs.client_connect_notify)( client, client->user_data, connected); + + /* Cleanup any in-progress TXN on disconnect */ + if (!connected) + mgmt_be_cleanup_all_txns(client); + return 0; }