diff --git a/src/mctpd.c b/src/mctpd.c index 523cda5f..093bd504 100644 --- a/src/mctpd.c +++ b/src/mctpd.c @@ -292,6 +292,38 @@ struct ctx { uint64_t endpoint_poll; }; +/* data for a command + response. + * + * req is populated by the caller. resp is allocated during command submission, + * and must be freed by the caller. + */ +struct mctp_ctrl_cmd { + /* populated by caller */ + void *req; + size_t req_len; + bool disable_retry; + + /* populated on response RX */ + void *resp; + size_t resp_len; + struct sockaddr_mctp_ext resp_addr; +}; + +#define mctp_ctrl_cmd_init_from_req_type(cmd, req) \ + do { \ + (cmd)->req = &req; \ + (cmd)->req_len = sizeof(req); \ + } while (0) + +/* frees response data, but not cmd itself, as this will typically be on the + * stack + */ +static void mctp_ctrl_cmd_free(struct mctp_ctrl_cmd *cmd) +{ + free(cmd->resp); + cmd->resp = NULL; +} + static int emit_endpoint_added(const struct peer *peer); static int emit_endpoint_removed(const struct peer *peer); static int emit_interface_added(struct link *link); @@ -305,7 +337,7 @@ static int add_peer_from_addr(struct ctx *ctx, struct peer **ret_peer); static int remove_peer(struct peer *peer); static int remove_bridged_peers(struct peer *bridge); -static int query_peer_properties(struct peer *peer); +static void query_peer_properties(struct peer *peer); static int setup_added_peer(struct peer *peer); static void add_peer_route(struct peer *peer); static int publish_peer(struct peer *peer); @@ -1595,9 +1627,9 @@ static const char *peer_cmd_prefix(const char *peer, uint8_t cmd) /* Common checks for responses: that we have enough data for a response, * the expected IID and opcode, and that the response indicated success. */ -static int mctp_ctrl_validate_response(uint8_t *buf, size_t rsp_size, +static int mctp_ctrl_validate_response(struct mctp_ctrl_cmd *cmd, size_t exp_size, const char *peer, - uint8_t iid, uint8_t cmd) + uint8_t iid, uint8_t cmd_id) { struct mctp_ctrl_resp *rsp; @@ -1607,38 +1639,39 @@ static int mctp_ctrl_validate_response(uint8_t *buf, size_t rsp_size, } /* Error responses only need to include the completion code */ - if (rsp_size < MCTP_CTRL_ERROR_RESP_LEN) { + if (cmd->resp_len < MCTP_CTRL_ERROR_RESP_LEN) { warnx("%s: Wrong reply length (%zu bytes)", - peer_cmd_prefix(peer, cmd), rsp_size); + peer_cmd_prefix(peer, cmd_id), cmd->resp_len); return -ENOMSG; } /* we have enough for the smallest common response message */ - rsp = (void *)buf; + rsp = cmd->resp; if ((rsp->ctrl_hdr.rq_dgram_inst & RQDI_IID_MASK) != iid) { warnx("%s: Wrong IID (0x%02x, expected 0x%02x)", - peer_cmd_prefix(peer, cmd), + peer_cmd_prefix(peer, cmd_id), rsp->ctrl_hdr.rq_dgram_inst & RQDI_IID_MASK, iid); return -ENOMSG; } - if (rsp->ctrl_hdr.command_code != cmd) { + if (rsp->ctrl_hdr.command_code != cmd_id) { warnx("%s: Wrong opcode (0x%02x) in response", - peer_cmd_prefix(peer, cmd), rsp->ctrl_hdr.command_code); + peer_cmd_prefix(peer, cmd_id), + rsp->ctrl_hdr.command_code); return -ENOMSG; } if (rsp->completion_code) { warnx("%s: Command failed, completion code 0x%02x", - peer_cmd_prefix(peer, cmd), rsp->completion_code); + peer_cmd_prefix(peer, cmd_id), rsp->completion_code); return -ECONNREFUSED; } /* Non-error responses must be full sized */ - if (rsp_size < exp_size) { + if (cmd->resp_len < exp_size) { warnx("%s: Wrong reply length (%zu bytes)", - peer_cmd_prefix(peer, cmd), rsp_size); + peer_cmd_prefix(peer, cmd_id), cmd->resp_len); return -ENOMSG; } @@ -1651,19 +1684,21 @@ static int mctp_ctrl_validate_response(uint8_t *buf, size_t rsp_size, * Extended addressing is used optionally, depending on ext_addr arg. */ static int endpoint_query_addr(struct ctx *ctx, const struct sockaddr_mctp_ext *req_addr, - bool ext_addr, const void *req, size_t req_len, - uint8_t **resp, size_t *resp_len, - struct sockaddr_mctp_ext *resp_addr) + bool ext_addr, struct mctp_ctrl_cmd *cmd) { + unsigned int max_retries = 4; size_t req_addr_len; + unsigned int retry; int sd = -1, val; ssize_t rc; size_t buf_size; uint8_t *buf = NULL; - *resp = NULL; - *resp_len = 0; + cmd->resp = NULL; + cmd->resp_len = 0; + if (cmd->disable_retry) + max_retries = 1; sd = mctp_ops.mctp.socket(); if (sd < 0) { @@ -1688,45 +1723,57 @@ static int endpoint_query_addr(struct ctx *ctx, req_addr_len = sizeof(struct sockaddr_mctp); } - if (req_len == 0) { + if (cmd->req_len == 0) { bug_warn("zero length request"); rc = -EPROTO; goto out; } - rc = mctp_ops.mctp.sendto(sd, req, req_len, 0, - (struct sockaddr *)req_addr, req_addr_len); - if (rc < 0) { - rc = -errno; - if (ctx->verbose) { - warnx("%s: sendto(%s) %zu bytes failed. %s", __func__, - ext_addr_tostr(req_addr), req_len, strerror(-rc)); + + for (retry = 0; retry < max_retries; retry++) { + rc = mctp_ops.mctp.sendto(sd, cmd->req, cmd->req_len, 0, + (struct sockaddr *)req_addr, + req_addr_len); + if (rc < 0) { + rc = -errno; + if (ctx->verbose) { + warnx("%s: sendto(%s) %zu bytes failed. %s", + __func__, ext_addr_tostr(req_addr), + cmd->req_len, strerror(-rc)); + } + break; + } + if ((size_t)rc != cmd->req_len) { + bug_warn("incorrect sendto %zd, expected %zu", rc, + cmd->req_len); + rc = -EPROTO; + break; } - goto out; - } - if ((size_t)rc != req_len) { - bug_warn("incorrect sendto %zd, expected %zu", rc, req_len); - rc = -EPROTO; - goto out; - } - rc = wait_fd_timeout(sd, EPOLLIN, ctx->mctp_timeout); - if (rc < 0) { - if (rc == -ETIMEDOUT && ctx->verbose) { - warnx("%s: receive timed out from %s", __func__, + rc = wait_fd_timeout(sd, EPOLLIN, ctx->mctp_timeout); + if (rc == 0) + break; + + if (rc == -ETIMEDOUT) { + warnx("receive timed out from %s, attempt %d/%d", + ext_addr_tostr(req_addr), retry + 1, max_retries); + } else { + warnx("receive error from %s", ext_addr_tostr(req_addr)); + break; } - goto out; } - rc = read_message(ctx, sd, &buf, &buf_size, resp_addr); - if (rc < 0) { + if (rc || retry == max_retries) + goto out; + + rc = read_message(ctx, sd, &buf, &buf_size, &cmd->resp_addr); + if (rc < 0) goto out; - } - if (resp_addr->smctp_base.smctp_type != + if (cmd->resp_addr.smctp_base.smctp_type != req_addr->smctp_base.smctp_type) { warnx("Mismatching response type %d for request type %d. dest %s", - resp_addr->smctp_base.smctp_type, + cmd->resp_addr.smctp_base.smctp_type, req_addr->smctp_base.smctp_type, ext_addr_tostr(req_addr)); rc = -ENOMSG; @@ -1738,8 +1785,8 @@ static int endpoint_query_addr(struct ctx *ctx, if (rc) { free(buf); } else { - *resp = buf; - *resp_len = buf_size; + cmd->resp = buf; + cmd->resp_len = buf_size; } return rc; @@ -1747,10 +1794,8 @@ static int endpoint_query_addr(struct ctx *ctx, /* Queries an endpoint peer. Addressing is standard eid/net. */ -static int endpoint_query_peer(const struct peer *peer, uint8_t req_type, - const void *req, size_t req_len, uint8_t **resp, - size_t *resp_len, - struct sockaddr_mctp_ext *resp_addr) +static int endpoint_query_peer(const struct peer *peer, + struct mctp_ctrl_cmd *cmd) { struct sockaddr_mctp_ext addr = { 0 }; @@ -1763,19 +1808,16 @@ static int endpoint_query_peer(const struct peer *peer, uint8_t req_type, addr.smctp_base.smctp_network = peer->net; addr.smctp_base.smctp_addr.s_addr = peer->eid; - addr.smctp_base.smctp_type = req_type; + addr.smctp_base.smctp_type = MCTP_CTRL_HDR_MSG_TYPE; addr.smctp_base.smctp_tag = MCTP_TAG_OWNER; - return endpoint_query_addr(peer->ctx, &addr, false, req, req_len, resp, - resp_len, resp_addr); + return endpoint_query_addr(peer->ctx, &addr, false, cmd); } /* Queries an endpoint using physical addressing, null EID. */ static int endpoint_query_phys(struct ctx *ctx, const dest_phys *dest, - uint8_t req_type, const void *req, - size_t req_len, uint8_t **resp, size_t *resp_len, - struct sockaddr_mctp_ext *resp_addr) + struct mctp_ctrl_cmd *cmd) { struct sockaddr_mctp_ext addr = { 0 }; @@ -1793,11 +1835,10 @@ static int endpoint_query_phys(struct ctx *ctx, const dest_phys *dest, addr.smctp_halen = dest->hwaddr_len; memcpy(addr.smctp_haddr, dest->hwaddr, dest->hwaddr_len); - addr.smctp_base.smctp_type = req_type; + addr.smctp_base.smctp_type = MCTP_CTRL_HDR_MSG_TYPE; addr.smctp_base.smctp_tag = MCTP_TAG_OWNER; - return endpoint_query_addr(ctx, &addr, true, req, req_len, resp, - resp_len, resp_addr); + return endpoint_query_addr(ctx, &addr, true, cmd); } /* returns -ECONNREFUSED if the endpoint returns failure. @@ -1809,15 +1850,13 @@ static int endpoint_send_set_endpoint_id(const struct peer *peer, mctp_eid_t *new_eidp, uint8_t *req_pool_size) { - struct sockaddr_mctp_ext addr; - struct mctp_ctrl_cmd_set_eid req = { 0 }; struct mctp_ctrl_resp_set_eid *resp = NULL; - int rc; - uint8_t *buf = NULL; - size_t buf_size; + struct mctp_ctrl_cmd_set_eid req = { 0 }; uint8_t iid, stat, alloc, pool_size = 0; const dest_phys *dest = &peer->phys; + struct mctp_ctrl_cmd cmd = { 0 }; mctp_eid_t new_eid; + int rc; rc = -1; @@ -1829,18 +1868,19 @@ static int endpoint_send_set_endpoint_id(const struct peer *peer, req.operation = mctp_ctrl_cmd_set_eid_set_eid; // TODO: do we want Force? req.eid = peer->eid; - rc = endpoint_query_phys(peer->ctx, dest, MCTP_CTRL_HDR_MSG_TYPE, &req, - sizeof(req), &buf, &buf_size, &addr); + + mctp_ctrl_cmd_init_from_req_type(&cmd, req); + rc = endpoint_query_phys(peer->ctx, dest, &cmd); if (rc < 0) goto out; - rc = mctp_ctrl_validate_response(buf, buf_size, sizeof(*resp), + rc = mctp_ctrl_validate_response(&cmd, sizeof(*resp), dest_phys_tostr(dest), iid, MCTP_CTRL_CMD_SET_ENDPOINT_ID); if (rc) goto out; - resp = (void *)buf; + resp = (void *)cmd.resp; stat = resp->status >> 4 & 0x3; new_eid = resp->eid_set; @@ -1887,7 +1927,7 @@ static int endpoint_send_set_endpoint_id(const struct peer *peer, rc = 0; out: - free(buf); + mctp_ctrl_cmd_free(&cmd); return rc; } @@ -2432,13 +2472,12 @@ static void set_berr(struct ctx *ctx, int errcode, sd_bus_error *berr) static int query_get_endpoint_id(struct ctx *ctx, const dest_phys *dest, mctp_eid_t *ret_eid, uint8_t *ret_ep_type, - uint8_t *ret_media_spec, struct peer *peer) + uint8_t *ret_media_spec, struct peer *peer, + bool retry) { - struct sockaddr_mctp_ext addr; struct mctp_ctrl_cmd_get_eid req = { 0 }; struct mctp_ctrl_resp_get_eid *resp = NULL; - uint8_t *buf = NULL; - size_t buf_size; + struct mctp_ctrl_cmd cmd = { 0 }; uint8_t iid; int rc; @@ -2446,30 +2485,29 @@ static int query_get_endpoint_id(struct ctx *ctx, const dest_phys *dest, mctp_ctrl_msg_hdr_init_req(&req.ctrl_hdr, iid, MCTP_CTRL_CMD_GET_ENDPOINT_ID); + mctp_ctrl_cmd_init_from_req_type(&cmd, req); + cmd.disable_retry = !retry; if (peer) - rc = endpoint_query_peer(peer, MCTP_CTRL_HDR_MSG_TYPE, &req, - sizeof(req), &buf, &buf_size, &addr); + rc = endpoint_query_peer(peer, &cmd); else - rc = endpoint_query_phys(ctx, dest, MCTP_CTRL_HDR_MSG_TYPE, - &req, sizeof(req), &buf, &buf_size, - &addr); + rc = endpoint_query_phys(ctx, dest, &cmd); if (rc < 0) goto out; - rc = mctp_ctrl_validate_response(buf, buf_size, sizeof(*resp), + rc = mctp_ctrl_validate_response(&cmd, sizeof(*resp), dest_phys_tostr(dest), iid, MCTP_CTRL_CMD_GET_ENDPOINT_ID); if (rc) goto out; - resp = (void *)buf; + resp = cmd.resp; *ret_eid = resp->eid; *ret_ep_type = resp->eid_type; *ret_media_spec = resp->medium_data; out: - free(buf); + mctp_ctrl_cmd_free(&cmd); return rc; } @@ -2489,7 +2527,7 @@ static int get_endpoint_peer(struct ctx *ctx, sd_bus_error *berr, *ret_peer = NULL; rc = query_get_endpoint_id(ctx, dest, &eid, &ep_type, &medium_spec, - /*peer=*/NULL); + /*peer=*/NULL, /*retry=*/true); if (rc) return rc; @@ -2542,11 +2580,10 @@ static int get_endpoint_peer(struct ctx *ctx, sd_bus_error *berr, static int query_get_peer_msgtypes(struct peer *peer) { - struct sockaddr_mctp_ext addr; - struct mctp_ctrl_cmd_get_msg_type_support req; struct mctp_ctrl_resp_get_msg_type_support *resp = NULL; - uint8_t *buf = NULL; - size_t buf_size, expect_size; + struct mctp_ctrl_cmd_get_msg_type_support req; + struct mctp_ctrl_cmd cmd = { 0 }; + size_t expect_size; uint8_t iid; int rc; @@ -2557,23 +2594,23 @@ static int query_get_peer_msgtypes(struct peer *peer) mctp_ctrl_msg_hdr_init_req(&req.ctrl_hdr, iid, MCTP_CTRL_CMD_GET_MESSAGE_TYPE_SUPPORT); + mctp_ctrl_cmd_init_from_req_type(&cmd, req); - rc = endpoint_query_peer(peer, MCTP_CTRL_HDR_MSG_TYPE, &req, - sizeof(req), &buf, &buf_size, &addr); + rc = endpoint_query_peer(peer, &cmd); if (rc < 0) goto out; rc = mctp_ctrl_validate_response( - buf, buf_size, sizeof(*resp), peer_tostr_short(peer), iid, + &cmd, sizeof(*resp), peer_tostr_short(peer), iid, MCTP_CTRL_CMD_GET_MESSAGE_TYPE_SUPPORT); if (rc) goto out; - resp = (void *)buf; + resp = cmd.resp; expect_size = sizeof(*resp) + resp->msg_type_count; - if (buf_size != expect_size) { + if (cmd.resp_len != expect_size) { warnx("%s: bad reply length. got %zu, expected %zu, %d entries. dest %s", - __func__, buf_size, expect_size, resp->msg_type_count, + __func__, cmd.resp_len, expect_size, resp->msg_type_count, peer_tostr(peer)); rc = -ENOMSG; goto out; @@ -2588,20 +2625,20 @@ static int query_get_peer_msgtypes(struct peer *peer) memcpy(peer->message_types, (void *)(resp + 1), resp->msg_type_count); rc = 0; out: - free(buf); + mctp_ctrl_cmd_free(&cmd); return rc; } static int query_get_peer_vdm_types(struct peer *peer) { struct vdm_type_support *cur_vdm_type, *new_vdm, *vdm_types = NULL; - size_t buf_size, expect_size, new_size, num_vdm_types = 0; struct mctp_ctrl_resp_get_vdm_support *resp = NULL; + size_t expect_size, new_size, num_vdm_types = 0; struct mctp_ctrl_cmd_get_vdm_support req; - uint8_t selector, iid, fmt, *buf = NULL; struct mctp_vdm_pcie_data *vdm_pcie; struct mctp_vdm_iana_data *vdm_iana; - struct sockaddr_mctp_ext addr; + uint8_t selector, iid, fmt; + struct mctp_ctrl_cmd cmd; int rc; req.ctrl_hdr.command_code = MCTP_CTRL_CMD_GET_VENDOR_MESSAGE_SUPPORT; @@ -2613,23 +2650,21 @@ static int query_get_peer_vdm_types(struct peer *peer) mctp_ctrl_msg_hdr_init_req( &req.ctrl_hdr, iid, MCTP_CTRL_CMD_GET_VENDOR_MESSAGE_SUPPORT); - if (buf) { - free(buf); - buf = NULL; - } - rc = endpoint_query_peer(peer, MCTP_CTRL_HDR_MSG_TYPE, &req, - sizeof(req), &buf, &buf_size, &addr); + + memset(&cmd, 0, sizeof(cmd)); + mctp_ctrl_cmd_init_from_req_type(&cmd, req); + rc = endpoint_query_peer(peer, &cmd); if (rc < 0) break; expect_size = sizeof(*resp); rc = mctp_ctrl_validate_response( - buf, buf_size, expect_size, peer_tostr_short(peer), iid, + &cmd, expect_size, peer_tostr_short(peer), iid, MCTP_CTRL_CMD_GET_VENDOR_MESSAGE_SUPPORT); if (rc) break; - resp = (void *)buf; + resp = cmd.resp; fmt = resp->vendor_id_format; if (fmt == MCTP_GET_VDM_SUPPORT_PCIE_FORMAT_ID) { expect_size = sizeof(*resp) + @@ -2643,9 +2678,9 @@ static int query_get_peer_vdm_types(struct peer *peer) rc = -ENOMSG; break; } - if (buf_size != expect_size) { + if (cmd.resp_len != expect_size) { warnx("%s: bad reply length. got %zu, expected %zu dest %s", - __func__, buf_size, expect_size, + __func__, cmd.resp_len, expect_size, peer_tostr(peer)); rc = -ENOMSG; break; @@ -2691,9 +2726,10 @@ static int query_get_peer_vdm_types(struct peer *peer) rc = -EPROTO; break; } + mctp_ctrl_cmd_free(&cmd); } - free(buf); + mctp_ctrl_cmd_free(&cmd); free(vdm_types); return rc; } @@ -2712,11 +2748,9 @@ static int peer_set_uuid(struct peer *peer, const uint8_t uuid[16]) static int query_get_peer_uuid_by_phys(struct ctx *ctx, const dest_phys *dest, uint8_t uuid[16]) { - struct sockaddr_mctp_ext addr; - struct mctp_ctrl_cmd_get_uuid req; struct mctp_ctrl_resp_get_uuid *resp = NULL; - uint8_t *buf = NULL; - size_t buf_size; + struct mctp_ctrl_cmd_get_uuid req; + struct mctp_ctrl_cmd cmd = { 0 }; uint8_t iid; int rc; @@ -2724,33 +2758,31 @@ static int query_get_peer_uuid_by_phys(struct ctx *ctx, const dest_phys *dest, mctp_ctrl_msg_hdr_init_req(&req.ctrl_hdr, iid, MCTP_CTRL_CMD_GET_ENDPOINT_UUID); + mctp_ctrl_cmd_init_from_req_type(&cmd, req); - rc = endpoint_query_phys(ctx, dest, MCTP_CTRL_HDR_MSG_TYPE, &req, - sizeof(req), &buf, &buf_size, &addr); + rc = endpoint_query_phys(ctx, dest, &cmd); if (rc < 0) goto out; - rc = mctp_ctrl_validate_response(buf, buf_size, sizeof(*resp), + rc = mctp_ctrl_validate_response(&cmd, sizeof(*resp), dest_phys_tostr(dest), iid, MCTP_CTRL_CMD_GET_ENDPOINT_UUID); if (rc) goto out; - resp = (void *)buf; + resp = cmd.resp; memcpy(uuid, resp->uuid, 16); out: - free(buf); + mctp_ctrl_cmd_free(&cmd); return rc; } static int query_get_peer_uuid(struct peer *peer) { - struct sockaddr_mctp_ext addr; - struct mctp_ctrl_cmd_get_uuid req; struct mctp_ctrl_resp_get_uuid *resp = NULL; - uint8_t *buf = NULL; - size_t buf_size; + struct mctp_ctrl_cmd_get_uuid req; + struct mctp_ctrl_cmd cmd = { 0 }; uint8_t iid; int rc; @@ -2764,27 +2796,26 @@ static int query_get_peer_uuid(struct peer *peer) mctp_ctrl_msg_hdr_init_req(&req.ctrl_hdr, iid, MCTP_CTRL_CMD_GET_ENDPOINT_UUID); + mctp_ctrl_cmd_init_from_req_type(&cmd, req); - rc = endpoint_query_peer(peer, MCTP_CTRL_HDR_MSG_TYPE, &req, - sizeof(req), &buf, &buf_size, &addr); + rc = endpoint_query_peer(peer, &cmd); if (rc < 0) goto out; - rc = mctp_ctrl_validate_response(buf, buf_size, sizeof(*resp), + rc = mctp_ctrl_validate_response(&cmd, sizeof(*resp), peer_tostr_short(peer), iid, MCTP_CTRL_CMD_GET_ENDPOINT_UUID); if (rc) goto out; - resp = (void *)buf; - + resp = cmd.resp; rc = peer_set_uuid(peer, resp->uuid); if (rc < 0) goto out; rc = 0; out: - free(buf); + mctp_ctrl_cmd_free(&cmd); return rc; } @@ -2862,7 +2893,7 @@ static int method_setup_endpoint(sd_bus_message *call, void *data, /* Get Endpoint ID */ rc = query_get_endpoint_id(ctx, dest, &eid, &ep_type, &medium_spec, - /*peer=*/NULL); + /*peer=*/NULL, true); if (rc) goto err; @@ -3129,36 +3160,15 @@ static int method_learn_endpoint(sd_bus_message *call, void *data, // Query various properties of a peer. // To be called when a new peer is discovered/assigned, once an EID is known // and routable. -static int query_peer_properties(struct peer *peer) +static void query_peer_properties(struct peer *peer) { - const unsigned int max_retries = 4; bool supports_vdm = false; int rc; - for (unsigned int i = 0; i < max_retries; i++) { - rc = query_get_peer_msgtypes(peer); - - // Success - if (rc == 0) - break; - - // On timeout, retry - if (rc == -ETIMEDOUT) { - if (peer->ctx->verbose) - warnx("Retrying to get endpoint types for %s. Attempt %u", - peer_tostr(peer), i + 1); - rc = 0; - continue; - } - - // On other errors, warn and ignore - if (rc < 0) { - if (peer->ctx->verbose) - warnx("Error getting endpoint types for %s. Ignoring error %d %s", - peer_tostr(peer), -rc, strerror(-rc)); - rc = 0; - break; - } + rc = query_get_peer_msgtypes(peer); + if (rc < 0 && peer->ctx->verbose) { + errno = -rc; + warn("Error getting endpoint types for %s", peer_tostr(peer)); } for (unsigned int i = 0; i < peer->num_message_types; i++) { @@ -3170,58 +3180,21 @@ static int query_peer_properties(struct peer *peer) } if (supports_vdm) { - for (unsigned int i = 0; i < max_retries; i++) { - rc = query_get_peer_vdm_types(peer); - - if (rc == 0) - break; - - // On timeout, retry - if (rc == -ETIMEDOUT) { - if (peer->ctx->verbose) - warnx("Retrying to get vendor message types for %s. Attempt %u", - peer_tostr(peer), i + 1); - rc = 0; - continue; - } - - if (rc < 0) { - warnx("Error getting vendor message types for %s. Ignoring error %d %s", - peer_tostr(peer), rc, strerror(-rc)); - rc = 0; - break; - } + rc = query_get_peer_vdm_types(peer); + if (rc < 0 && peer->ctx->verbose) { + errno = -rc; + warn("Error getting vendor message types for %s", + peer_tostr(peer)); } } - for (unsigned int i = 0; i < max_retries; i++) { - rc = query_get_peer_uuid(peer); - - // Success - if (rc == 0) - break; - - // On timeout, retry - if (rc == -ETIMEDOUT) { - if (peer->ctx->verbose) - warnx("Retrying to get peer UUID for %s. Attempt %u", - peer_tostr(peer), i + 1); - rc = 0; - continue; - } - - // On other errors, warn and ignore - if (rc < 0) { - if (peer->ctx->verbose) - warnx("Error getting UUID for %s. Ignoring error %d %s", - peer_tostr(peer), -rc, strerror(-rc)); - rc = 0; - break; - } + rc = query_get_peer_uuid(peer); + if (rc < 0 && peer->ctx->verbose) { + errno = -rc; + warn("Error getting UUID for %s", peer_tostr(peer)); } // TODO: emit property changed? Though currently they are all const. - return rc; } static int peer_neigh_update(struct peer *peer, uint16_t type) @@ -3302,12 +3275,9 @@ static int setup_added_peer(struct peer *peer) if (!is_eid_in_bridge_pool(n, peer->ctx, peer->eid)) add_peer_route(peer); - rc = query_peer_properties(peer); - if (rc < 0) - goto out; + query_peer_properties(peer); rc = publish_peer(peer); -out: if (rc < 0) { remove_peer(peer); } @@ -3517,7 +3487,8 @@ static int peer_endpoint_recover(sd_event_source *s, uint64_t usec, */ rc = query_get_endpoint_id(ctx, &peer->phys, &peer->recovery.eid, &peer->recovery.endpoint_type, - &peer->recovery.medium_spec, /*peer=*/NULL); + &peer->recovery.medium_spec, /*peer=*/NULL, + /*retry=*/false); if (rc < 0) { goto reschedule; } @@ -3735,7 +3706,7 @@ static int method_net_learn_endpoint(sd_bus_message *call, void *data, } rc = query_get_endpoint_id(peer->ctx, &dest, &ret_eid, &ret_ep_type, - &ret_medium_spec, peer); + &ret_medium_spec, peer, true); if (rc) { warnx("Error getting endpoint id for %s. error %d %s", peer_tostr(peer), rc, strerror(-rc)); @@ -5481,11 +5452,9 @@ static int endpoint_send_allocate_endpoint_ids( mctp_ctrl_cmd_allocate_eids_op op, uint8_t *allocated_pool_size, mctp_eid_t *allocated_pool_start) { - struct sockaddr_mctp_ext addr; - struct mctp_ctrl_cmd_allocate_eids req = { 0 }; struct mctp_ctrl_resp_allocate_eids *resp = NULL; - uint8_t *buf = NULL; - size_t buf_size; + struct mctp_ctrl_cmd_allocate_eids req = { 0 }; + struct mctp_ctrl_cmd cmd = { 0 }; uint8_t iid, stat; int rc; @@ -5495,19 +5464,19 @@ static int endpoint_send_allocate_endpoint_ids( req.alloc_eid_op = (uint8_t)(op & 0x03); req.pool_size = eid_pool_size; req.start_eid = eid_start; - rc = endpoint_query_peer(peer, MCTP_CTRL_HDR_MSG_TYPE, &req, - sizeof(req), &buf, &buf_size, &addr); + mctp_ctrl_cmd_init_from_req_type(&cmd, req); + rc = endpoint_query_peer(peer, &cmd); if (rc < 0) goto out; - rc = mctp_ctrl_validate_response(buf, buf_size, sizeof(*resp), + rc = mctp_ctrl_validate_response(&cmd, sizeof(*resp), peer_tostr_short(peer), iid, MCTP_CTRL_CMD_ALLOCATE_ENDPOINT_IDS); if (rc) goto out; - resp = (void *)buf; + resp = cmd.resp; if (!resp) { warnx("Invalid response buffer"); return -ENOMEM; @@ -5542,7 +5511,7 @@ static int endpoint_send_allocate_endpoint_ids( } out: - free(buf); + mctp_ctrl_cmd_free(&cmd); return rc; } @@ -5574,17 +5543,15 @@ static int peer_reschedule_poll(sd_event_source *source, uint64_t usec) static int peer_endpoint_poll(sd_event_source *s, uint64_t usec, void *userdata) { - struct sockaddr_mctp_ext resp_addr = { 0 }; struct mctp_ctrl_resp_get_eid *resp = NULL; struct sockaddr_mctp_ext req_addr = { 0 }; struct mctp_ctrl_cmd_get_eid req = { 0 }; mctp_eid_t pool_start, idx, ret_eid = 0; struct ep_poll_ctx *pctx = userdata; struct peer *bridge = pctx->bridge; + struct mctp_ctrl_cmd cmd = { 0 }; sd_event_source *source = NULL; struct peer *peer = NULL; - uint8_t *buf = NULL; - size_t buf_size; struct net *n; uint8_t iid; int rc = 0; @@ -5628,15 +5595,18 @@ static int peer_endpoint_poll(sd_event_source *s, uint64_t usec, void *userdata) mctp_ctrl_msg_hdr_init_req(&req.ctrl_hdr, iid, MCTP_CTRL_CMD_GET_ENDPOINT_ID); - rc = endpoint_query_addr(bridge->ctx, &req_addr, false, &req, - sizeof(req), &buf, &buf_size, &resp_addr); + mctp_ctrl_cmd_init_from_req_type(&cmd, req); + /* don't retry here, we're just probing */ + cmd.disable_retry = true; + + rc = endpoint_query_addr(bridge->ctx, &req_addr, false, &cmd); if (rc < 0) { - free(buf); + mctp_ctrl_cmd_free(&cmd); peer_reschedule_poll(source, bridge->ctx->endpoint_poll); return 0; } - resp = (void *)buf; + resp = cmd.resp; if (!resp) { warnx("Invalid response buffer"); return -ENOMEM; @@ -5664,7 +5634,7 @@ static int peer_endpoint_poll(sd_event_source *s, uint64_t usec, void *userdata) rc = setup_added_peer(peer); if (rc < 0) { - free(buf); + mctp_ctrl_cmd_free(&cmd); peer_reschedule_poll(source, bridge->ctx->endpoint_poll); return 0; } @@ -5674,7 +5644,7 @@ static int peer_endpoint_poll(sd_event_source *s, uint64_t usec, void *userdata) sd_event_source_unref(source); bridge->bridge_ep_poll.sources[idx] = NULL; free(pctx); - free(buf); + mctp_ctrl_cmd_free(&cmd); return rc; } diff --git a/tests/test_mctpd.py b/tests/test_mctpd.py index 4f3d484b..2bff0b41 100644 --- a/tests/test_mctpd.py +++ b/tests/test_mctpd.py @@ -1765,6 +1765,129 @@ async def handle_mctp_control(self, sock, addr, data): assert res == 0 +async def test_query_peer_properties_unimplemented(dbus, mctpd): + """Test that any of the optional commands being unimplemented do not + prevent endpoint setup.""" + + class CommandUnimplementedEndpoint(Endpoint): + """Always returns Command Not Implemented for the specified opcode""" + + def __init__(self, opcode, *args, **kwargs): + self.opcode = opcode + super().__init__(*args, **kwargs) + + async def handle_mctp_control(self, sock, addr, data): + flags, opcode = data[0:2] + rq = flags & 0x80 + if rq and opcode == self.opcode: + raddr = MCTPSockAddr.for_ep_resp(self, addr, sock.addr_ext) + cc = 0x05 # unsupported command + resp = bytes([flags & 0x1F, opcode, cc]) + await sock.send(raddr, resp) + return + return await super().handle_mctp_control(sock, addr, data) + + class CommandDropEndpoint(Endpoint): + """Always ignores commands of the specified opcode""" + + def __init__(self, opcode, *args, **kwargs): + self.opcode = opcode + super().__init__(*args, **kwargs) + + async def handle_mctp_control(self, sock, addr, data): + rq = data[0] & 0x80 + opcode = data[1] + if rq and opcode == self.opcode: + return + return await super().handle_mctp_control(sock, addr, data) + + optional_commands = [ + (0x03, "Get Endpoint UUID"), + (0x04, "Get Message Type Support"), + (0x06, "Get Vendor Defined Message Support"), + ] + + iface = mctpd.system.interfaces[0] + mctp = await mctpd_mctp_iface_obj(dbus, iface) + # Setting VDM types will trigger a Get VDM Support query + vdm_types = [ + VDMType(VDMType.TYPE_PCI, 0x1234, 0x5678), + VDMType(VDMType.TYPE_IANA, 0xABCDEF12, 0x3456), + ] + for idx, cmd in enumerate(optional_commands): + lladdr = 0xA1 + (idx * 2) + + ep = CommandUnimplementedEndpoint( + cmd[0], iface, bytes([lladdr]), vdm_msg_types=vdm_types + ) + mctpd.network.add_endpoint(ep) + + try: + await mctp.call_setup_endpoint(ep.lladdr) + except Exception: + raise AssertionError( + f"query failed with unimplemented {cmd[1]} command" + ) + + ep = CommandDropEndpoint( + cmd[0], iface, bytes([lladdr + 1]), vdm_msg_types=vdm_types + ) + mctpd.network.add_endpoint(ep) + + try: + await mctp.call_setup_endpoint(ep.lladdr) + except Exception: + raise AssertionError(f"query failed with dropped {cmd[1]} command") + + +async def test_query_peer_properties_same_iid_on_retry(nursery, dbus, sysnet): + """Verify that retries for query_peer_properties reuse the same IID. + + Per DSP0236 Table 10, a retry is a retransmission of the same MCTP control + message and must use the same instance ID (IID) within MT4. + """ + + class IIDTrackingEndpoint(Endpoint): + """Drop the first Get Message Type Support request, record all IIDs seen.""" + + def __init__(self, *args, **kwargs): + super().__init__(*args, **kwargs) + self.seen_iids = [] + self.drop_next = True + + async def handle_mctp_control(self, sock, addr, data): + rq = data[0] & 0x80 + opcode = data[1] + iid = data[0] & 0x1F + if rq and opcode == 0x05: # Get Message Type Support + self.seen_iids.append(iid) + if self.drop_next: + self.drop_next = False + return # simulate timeout + return await super().handle_mctp_control(sock, addr, data) + + mctpd = MctpdWrapper(dbus, sysnet) + await mctpd.start_mctpd(nursery) + + iface = mctpd.system.interfaces[0] + mctp = await mctpd_mctp_iface_obj(dbus, iface) + + ep = IIDTrackingEndpoint(iface, bytes([0x1A]), eid=15, types=[0, 1, 2]) + mctpd.network.add_endpoint(ep) + + await mctp.call_setup_endpoint(ep.lladdr) + + # Two Get Message Type Support requests: initial + one retry + assert len(ep.seen_iids) == 2 + # Both must carry the same IID + assert ep.seen_iids[0] == ep.seen_iids[1], ( + f"IID changed across retry: {ep.seen_iids[0]:#04x} -> {ep.seen_iids[1]:#04x}" + ) + + res = await mctpd.stop_mctpd() + assert res == 0 + + async def test_bridged_endpoint_poll(dbus, sysnet, nursery, autojump_clock): """Test that we use endpoint poll interval from the config and that we discover bridged endpoints via polling