diff --git a/README.md b/README.md index 0a1c2c28..6648d6b4 100644 --- a/README.md +++ b/README.md @@ -126,6 +126,11 @@ cd obj pytest ``` +To run without an existing dbus session: +```sh +dbus-run-session env DBUS_STARTER_BUS_TYPE=user pytest +``` + The test infrastructure depends on a few python packages, including the pytest binary. You can use a python `venv` to provide these: diff --git a/src/mctp-netlink.c b/src/mctp-netlink.c index 51a76577..b05cd9f2 100644 --- a/src/mctp-netlink.c +++ b/src/mctp-netlink.c @@ -28,6 +28,7 @@ struct linkmap_entry { uint32_t min_mtu; uint32_t max_mtu; + uint32_t hwaddr_len; mctp_eid_t *local_eids; size_t num_local; @@ -57,7 +58,7 @@ static int fill_linkmap(mctp_nl *nl); static void sort_linkmap(mctp_nl *nl); static int linkmap_add_entry(mctp_nl *nl, struct ifinfomsg *info, const char *ifname, size_t ifname_len, uint32_t net, - bool up, uint32_t min_mtu, uint32_t max_mtu); + bool up, uint32_t min_mtu, uint32_t max_mtu, size_t hwaddr_len); static struct linkmap_entry *entry_byindex(const mctp_nl *nl, int index); @@ -330,13 +331,13 @@ void mctp_nl_changes_dump(mctp_nl *nl, mctp_nl_change *changes, size_t num_chang "ADD_EID", "DEL_EID", }; - printf("%zu changes:\n", num_changes); + fprintf(stderr, "%zu changes:\n", num_changes); for (size_t i = 0; i < num_changes; i++) { mctp_nl_change *ch = &changes[i]; const char* ifname = mctp_nl_if_byindex(nl, ch->ifindex); if (!ifname) ifname = "deleted"; - printf("%3zd %-12s ifindex %3d (%-20s) eid %3d old_net %4d old_up %d\n", + fprintf(stderr, "%3zd %-12s ifindex %3d (%-20s) eid %3d old_net %4d old_up %d\n", i, ops[ch->op], ch->ifindex, ifname, ch->eid, ch->old_net, ch->old_up); } @@ -683,7 +684,7 @@ static int parse_getlink_dump(mctp_nl *nl, struct nlmsghdr *nlh, uint32_t len) for (; NLMSG_OK(nlh, len); nlh = NLMSG_NEXT(nlh, len)) { struct rtattr *rta = NULL, *rt_nest = NULL, *rt_mctp = NULL; char *ifname = NULL; - size_t ifname_len, rlen, nlen, mlen; + size_t ifname_len, rlen, nlen, mlen, hwaddr_len; uint32_t net, min_mtu = 0, max_mtu = 0; bool up; @@ -734,10 +735,15 @@ static int parse_getlink_dump(mctp_nl *nl, struct nlmsghdr *nlh, uint32_t len) continue; } + /* Treat missing address as 0 length */ + hwaddr_len = 0; + mctp_get_rtnlmsg_attr(IFLA_ADDRESS, rta, rlen, &hwaddr_len); + /* TODO: media type */ up = info->ifi_flags & IFF_UP; - linkmap_add_entry(nl, info, ifname, ifname_len, net, up, min_mtu, max_mtu); + linkmap_add_entry(nl, info, ifname, ifname_len, net, up, + min_mtu, max_mtu, hwaddr_len); } // Not done. return 1; @@ -859,7 +865,7 @@ static int fill_local_addrs(mctp_nl *nl) entry = entry_byindex(nl, ifa->ifa_index); if (!entry) { - warnx("kernel returned address for unknown if"); + warnx("kernel returned address for unknown if %d", ifa->ifa_index); continue; } tmp = realloc(entry->local_eids, @@ -904,19 +910,19 @@ void mctp_nl_linkmap_dump(const mctp_nl *nl) { size_t i, j; - printf("linkmap\n"); + fprintf(stderr, "linkmap\n"); for (i = 0; i < nl->linkmap_count; i++) { struct linkmap_entry *entry = &nl->linkmap[i]; const char* updown = entry->up ? "up" : "DOWN"; - printf(" %2d: %s, net %d %s local addrs [", + fprintf(stderr, " %2d: %s, net %d %s local addrs [", entry->ifindex, entry->ifname, entry->net, updown); for (j = 0; j < entry->num_local; j++) { if (j != 0) - printf(", "); - printf("%d", entry->local_eids[j]); + fprintf(stderr, ", "); + fprintf(stderr, "%d", entry->local_eids[j]); } - printf("]\n"); + fprintf(stderr, "]\n"); } } @@ -1003,6 +1009,16 @@ uint32_t mctp_nl_max_mtu_byindex(const mctp_nl *nl, int index) return 0; } +int mctp_nl_hwaddr_len_byindex(const mctp_nl *nl, int index, size_t *ret_hwaddr_len) +{ + struct linkmap_entry *entry = entry_byindex(nl, index); + if (!entry) { + return -ENOENT; + } + *ret_hwaddr_len = entry->hwaddr_len; + return 0; +} + mctp_eid_t *mctp_nl_addrs_byindex(const mctp_nl *nl, int index, size_t *ret_num) { @@ -1082,7 +1098,7 @@ int *mctp_nl_if_list(const mctp_nl *nl, size_t *ret_num_ifs) static int linkmap_add_entry(mctp_nl *nl, struct ifinfomsg *info, const char *ifname, size_t ifname_len, uint32_t net, - bool up, uint32_t min_mtu, uint32_t max_mtu) + bool up, uint32_t min_mtu, uint32_t max_mtu, size_t hwaddr_len) { struct linkmap_entry *entry; size_t newsz; @@ -1120,6 +1136,7 @@ static int linkmap_add_entry(mctp_nl *nl, struct ifinfomsg *info, entry->up = up; entry->max_mtu = max_mtu; entry->min_mtu = min_mtu; + entry->hwaddr_len = hwaddr_len; return 0; } diff --git a/src/mctp-netlink.h b/src/mctp-netlink.h index 88bbe725..ce0d9818 100644 --- a/src/mctp-netlink.h +++ b/src/mctp-netlink.h @@ -68,6 +68,8 @@ bool mctp_nl_up_byindex(const mctp_nl *nl, int index); uint32_t mctp_nl_min_mtu_byindex(const mctp_nl *nl, int index); /* Returns interface max_mtu, or 0 if bad index */ uint32_t mctp_nl_max_mtu_byindex(const mctp_nl *nl, int index); +/* Returns negative errno on failure */ +int mctp_nl_hwaddr_len_byindex(const mctp_nl *nl, int index, size_t *ret_hwaddr_len); /* Caller to free */ mctp_eid_t *mctp_nl_addrs_byindex(const mctp_nl *nl, int index, size_t *ret_num); diff --git a/src/mctp-ops.c b/src/mctp-ops.c index b5536e48..ecd0ada4 100644 --- a/src/mctp-ops.c +++ b/src/mctp-ops.c @@ -9,6 +9,7 @@ #include #include +#include #include "mctp.h" #include "mctp-ops.h" @@ -51,7 +52,12 @@ static int mctp_op_close(int sd) return close(sd); } -struct mctp_ops mctp_ops = { +static void mctp_bug_warn(const char* fmt, va_list args) +{ + vwarnx(fmt, args); +} + +const struct mctp_ops mctp_ops = { .mctp = { .socket = mctp_op_mctp_socket, .setsockopt = mctp_op_setsockopt, @@ -68,6 +74,7 @@ struct mctp_ops mctp_ops = { .recvfrom = mctp_op_recvfrom, .close = mctp_op_close, }, + .bug_warn = mctp_bug_warn, }; void mctp_ops_init(void) { } diff --git a/src/mctp-ops.h b/src/mctp-ops.h index c5d4636a..7f8a815a 100644 --- a/src/mctp-ops.h +++ b/src/mctp-ops.h @@ -8,6 +8,7 @@ #pragma once #include +#include #define _GNU_SOURCE @@ -26,8 +27,9 @@ struct socket_ops { struct mctp_ops { struct socket_ops mctp; struct socket_ops nl; + void (*bug_warn)(const char* fmt, va_list args); }; -extern struct mctp_ops mctp_ops; +extern const struct mctp_ops mctp_ops; void mctp_ops_init(void); diff --git a/src/mctpd.c b/src/mctpd.c index 290b3790..f19a763a 100644 --- a/src/mctpd.c +++ b/src/mctpd.c @@ -198,12 +198,8 @@ struct ctx { // Configuration const char *config_filename; - // Main instance for link/address state and listening for updates mctp_nl *nl; - // Second instance for sending mctp socket requests. State is unused. - mctp_nl *nl_query; - // Default BMC role in All of MCTP medium interface enum endpoint_role default_role; @@ -254,6 +250,20 @@ static const sd_bus_vtable bus_endpoint_obmc_vtable[]; static const sd_bus_vtable bus_endpoint_cc_vtable[]; static const sd_bus_vtable bus_endpoint_uuid_vtable[]; +__attribute__((format(printf, 1, 2))) +static void bug_warn(const char* fmt, ...) { + char *bug_fmt = NULL; + va_list ap; + + asprintf(&bug_fmt, "BUG: %s", fmt); + + va_start(ap, fmt); + mctp_ops.bug_warn(bug_fmt, ap); + va_end(ap); + + free(bug_fmt); +} + mctp_eid_t local_addr(const struct ctx *ctx, int ifindex) { mctp_eid_t *eids, ret = 0; size_t num; @@ -469,7 +479,7 @@ static const char *path_from_peer(const struct peer *peer) { if (!peer->published) { - warnx("BUG: %s on peer %s", __func__, peer_tostr(peer)); + bug_warn("%s on peer %s", __func__, peer_tostr(peer)); return NULL; } return peer->path; @@ -531,7 +541,7 @@ static int read_message(struct ctx *ctx, int sd, uint8_t **ret_buf, size_t *ret_ goto out; } if ((size_t)len != buf_size) { - warnx("BUG: incorrect recvfrom %zd, expected %zu", len, buf_size); + bug_warn("incorrect recvfrom %zd, expected %zu", len, buf_size); rc = -EPROTO; goto out; } @@ -572,7 +582,7 @@ static int reply_message(struct ctx *ctx, int sd, const void *resp, size_t resp_ if (reply_addr.smctp_addr.s_addr == 0 || reply_addr.smctp_addr.s_addr == 0xff) { - warnx("BUG: reply_message can't take EID %d", + bug_warn("reply_message can't take EID %d", reply_addr.smctp_addr.s_addr); return -EPROTO; } @@ -585,7 +595,7 @@ static int reply_message(struct ctx *ctx, int sd, const void *resp, size_t resp_ } if ((size_t)len != resp_len) { - warnx("BUG: short sendto %zd, expected %zu", len, resp_len); + bug_warn("short sendto %zd, expected %zu", len, resp_len); return -EPROTO; } return 0; @@ -768,9 +778,6 @@ static int handle_control_resolve_endpoint_id(struct ctx *ctx, resp_len = sizeof(*resp) + peer->phys.hwaddr_len; } - printf("resp_len %zu ... 0x%02x 0x%02x\n", resp_len, - ((uint8_t*)resp)[resp_len-2], - ((uint8_t*)resp)[resp_len-1]); return reply_message(ctx, sd, resp, resp_len, addr); } @@ -815,7 +822,7 @@ static int cb_listen_control_msg(sd_event_source *s, int sd, uint32_t revents, errx(EXIT_FAILURE, "Control socket returned EOF"); if (addr.smctp_base.smctp_type != MCTP_CTRL_HDR_MSG_TYPE) { - warnx("BUG: Wrong message type for listen socket"); + bug_warn("Wrong message type for listen socket"); rc = -EINVAL; goto out; } @@ -994,7 +1001,7 @@ static int cb_listen_monitor(sd_event_source *s, int sd, uint32_t revents, } if (ctx->verbose && any_error) { - printf("Error handling netlink update\n"); + warnx("Error handling netlink update"); mctp_nl_changes_dump(ctx->nl, changes, num_changes); mctp_nl_linkmap_dump(ctx->nl); } @@ -1175,7 +1182,7 @@ static int endpoint_query_addr(struct ctx *ctx, } if (req_len == 0) { - warnx("BUG: zero length request"); + bug_warn("zero length request"); rc = -EPROTO; goto out; } @@ -1191,7 +1198,7 @@ static int endpoint_query_addr(struct ctx *ctx, goto out; } if ((size_t)rc != req_len) { - warnx("BUG: incorrect sendto %zd, expected %zu", rc, req_len); + bug_warn("incorrect sendto %zd, expected %zu", rc, req_len); rc = -EPROTO; goto out; } @@ -1240,7 +1247,7 @@ static int endpoint_query_peer(const struct peer *peer, struct sockaddr_mctp_ext addr = {0}; if (peer->state != REMOTE) { - warnx("BUG: %s bad peer %s", __func__, peer_tostr(peer)); + bug_warn("%s bad peer %s", __func__, peer_tostr(peer)); return -EPROTO; } @@ -1354,7 +1361,7 @@ static int add_peer(struct ctx *ctx, const dest_phys *dest, mctp_eid_t eid, n = lookup_net(ctx, net); if (!n) { - warnx("BUG: %s Bad net %u", __func__, net); + bug_warn("%s Bad net %u", __func__, net); return -EPROTO; } @@ -1402,13 +1409,13 @@ static int add_peer(struct ctx *ctx, const dest_phys *dest, mctp_eid_t eid, static int check_peer_struct(const struct peer *peer, const struct net *n) { if (n->net != peer->net) { - warnx("BUG: Mismatching net %d vs peer net %u", + bug_warn("Mismatching net %d vs peer net %u", n->net, peer->net); return -1; } if (peer != n->peers[peer->eid]) { - warnx("BUG: Bad peer: net %u eid %02x", peer->net, peer->eid); + bug_warn("Bad peer: net %u eid %02x", peer->net, peer->eid); return -1; } @@ -1424,12 +1431,12 @@ static int remove_peer(struct peer *peer) n = lookup_net(peer->ctx, peer->net); if (!n) { - warnx("BUG: %s: Bad net %u", __func__, peer->net); + bug_warn("%s: Bad net %u", __func__, peer->net); return -EPROTO; } if (check_peer_struct(peer, n) != 0) { - warnx("BUG: %s: Inconsistent state", __func__); + bug_warn("%s: Inconsistent state", __func__); return -EPROTO; } @@ -1457,7 +1464,7 @@ static int remove_peer(struct peer *peer) } if (idx == ctx->num_peers) { - warnx("BUG: peer net %u, eid %d not found on remove!", + bug_warn("peer net %u, eid %d not found on remove!", peer->net, peer->eid); return -EPROTO; } @@ -1501,12 +1508,12 @@ static int change_peer_eid(struct peer *peer, mctp_eid_t new_eid) n = lookup_net(peer->ctx, peer->net); if (!n) { - warnx("BUG: %s: Bad net %u", __func__, peer->net); + bug_warn("%s: Bad net %u", __func__, peer->net); return -EPROTO; } if (check_peer_struct(peer, n) != 0) { - warnx("BUG: %s: Inconsistent state", __func__); + bug_warn("%s: Inconsistent state", __func__); return -EPROTO; } @@ -1531,12 +1538,12 @@ static int peer_set_mtu(struct ctx *ctx, struct peer *peer, uint32_t mtu) { ifname = mctp_nl_if_byindex(ctx->nl, peer->phys.ifindex); if (!ifname) { - warnx("BUG %s: no interface for ifindex %d", + bug_warn("%s: no interface for ifindex %d", __func__, peer->phys.ifindex); return -EPROTO; } - rc = mctp_nl_route_del(ctx->nl_query, peer->eid, ifname); + rc = mctp_nl_route_del(ctx->nl, peer->eid, ifname); if (rc < 0 && rc != -ENOENT) { warnx("%s, Failed removing existing route for eid %d %s", __func__, @@ -1544,7 +1551,7 @@ static int peer_set_mtu(struct ctx *ctx, struct peer *peer, uint32_t mtu) { // Continue regardless, route_add will likely fail with EEXIST } - rc = mctp_nl_route_add(ctx->nl_query, peer->eid, ifname, mtu); + rc = mctp_nl_route_add(ctx->nl, peer->eid, ifname, mtu); if (rc >= 0) { peer->mtu = mtu; } @@ -1562,13 +1569,13 @@ static int endpoint_assign_eid(struct ctx *ctx, sd_bus_error *berr, const dest_p net = mctp_nl_net_byindex(ctx->nl, dest->ifindex); if (!net) { - warnx("BUG: No net known for ifindex %d", dest->ifindex); + bug_warn("No net known for ifindex %d", dest->ifindex); return -EPROTO; } n = lookup_net(ctx, net); if (!n) { - warnx("BUG: Unknown net %d", net); + bug_warn("Unknown net %d", net); return -EPROTO; } @@ -2228,7 +2235,7 @@ static int peer_neigh_update(struct peer *peer, uint16_t type) NDA_DST, &peer->eid, sizeof(peer->eid)); msg.nh.nlmsg_len += mctp_put_rtnlmsg_attr(&rta, &rta_len, NDA_LLADDR, peer->phys.hwaddr, peer->phys.hwaddr_len); - return mctp_nl_send(peer->ctx->nl_query, &msg.nh); + return mctp_nl_send(peer->ctx->nl, &msg.nh); } // type is RTM_NEWROUTE or RTM_DELROUTE @@ -2236,20 +2243,20 @@ static int peer_route_update(struct peer *peer, uint16_t type) { const char * link; - link = mctp_nl_if_byindex(peer->ctx->nl_query, peer->phys.ifindex); + link = mctp_nl_if_byindex(peer->ctx->nl, peer->phys.ifindex); if (!link) { - warnx("BUG %s: Unknown ifindex %d", __func__, peer->phys.ifindex); + bug_warn("%s: Unknown ifindex %d", __func__, peer->phys.ifindex); return -ENODEV; } if (type == RTM_NEWROUTE) { - return mctp_nl_route_add(peer->ctx->nl_query, + return mctp_nl_route_add(peer->ctx->nl, peer->eid, link, peer->mtu); } else if (type == RTM_DELROUTE) { - return mctp_nl_route_del(peer->ctx->nl_query, peer->eid, link); + return mctp_nl_route_del(peer->ctx->nl, peer->eid, link); } - warnx("BUG %s: bad type %d", __func__, type); + bug_warn("%s: bad type %d", __func__, type); return -EPROTO; } @@ -2260,7 +2267,7 @@ static int setup_added_peer(struct peer *peer) // Set minimum MTU by default for compatibility. Clients can increase // this with .SetMTU as needed - peer->mtu = mctp_nl_min_mtu_byindex(peer->ctx->nl_query, peer->phys.ifindex); + peer->mtu = mctp_nl_min_mtu_byindex(peer->ctx->nl, peer->phys.ifindex); // add route before querying add_peer_route(peer); @@ -2277,15 +2284,22 @@ static int setup_added_peer(struct peer *peer) return rc; } -/* Adds routes/neigh. This is separate from - publish_peer() because we want a two stage setup of querying - properties (routed packets) then emitting dbus once finished */ -static void add_peer_route(struct peer *peer) +static void add_peer_neigh(struct peer *peer) { + size_t if_hwaddr_len; int rc; - // We always try to add routes/neighs, ignoring if they - // already exist. + rc = mctp_nl_hwaddr_len_byindex(peer->ctx->nl, peer->phys.ifindex, &if_hwaddr_len); + if (rc) { + warnx("Missing neigh ifindex %d", peer->phys.ifindex); + return; + } + + if (peer->phys.hwaddr_len == 0 && if_hwaddr_len == 0) { + // Don't add neigh entries for address-less transports + // We'll let the kernel reject mismatching entries. + return; + } if (peer->ctx->verbose) { fprintf(stderr, "Adding neigh to %s\n", peer_tostr(peer)); @@ -2297,6 +2311,19 @@ static void add_peer_route(struct peer *peer) } else { peer->have_neigh = true; } +} + +/* Adds routes/neigh. This is separate from + publish_peer() because we want a two stage setup of querying + properties (routed packets) then emitting dbus once finished */ +static void add_peer_route(struct peer *peer) +{ + int rc; + + // We always try to add routes/neighs, ignoring if they + // already exist. + + add_peer_neigh(peer); if (peer->ctx->verbose) { fprintf(stderr, "Adding route to %s\n", peer_tostr(peer)); @@ -2744,7 +2771,7 @@ static int bus_endpoint_get_prop(sd_bus *bus, } else if (strcmp(property, "Connectivity") == 0) { rc = sd_bus_message_append(reply, "s", peer->degraded ? "Degraded" : "Available"); } else { - printf("Unknown property '%s' for %s iface %s\n", property, path, interface); + warnx("Unknown property '%s' for %s iface %s", property, path, interface); rc = -ENOENT; } @@ -2782,11 +2809,8 @@ static int bus_link_get_prop(sd_bus *bus, if (link->published && strcmp(property, "Role") == 0) { rc = sd_bus_message_append(reply, "s", roles[link->role].dbus_val); } else if (strcmp(property, "NetworkId") == 0) { - uint32_t net = mctp_nl_net_byindex(link->ctx->nl, - link->ifindex); - + uint32_t net = mctp_nl_net_byindex(link->ctx->nl, link->ifindex); rc = sd_bus_message_append_basic(reply, 'u', &net); - } else { sd_bus_error_setf(berr, SD_BUS_ERROR_INVALID_ARGS, "Unknown property."); @@ -2808,7 +2832,7 @@ static int bus_link_set_prop(sd_bus *bus, int rc = -1; if (strcmp(property, "Role") != 0) { - printf("Unknown property '%s' for %s iface %s\n", property, path, interface); + warnx("Unknown property '%s' for %s iface %s", property, path, interface); rc = -ENOENT; goto out; } @@ -2829,7 +2853,7 @@ static int bus_link_set_prop(sd_bus *bus, rc = get_role(state, &role); if (rc < 0) { - printf("Invalid property value '%s' for property '%s' from interface '%s' on object '%s'\n", + warnx("Invalid property value '%s' for property '%s' from interface '%s' on object '%s'", state, property, interface, path); rc = -EINVAL; goto out; @@ -2865,7 +2889,7 @@ static int bus_endpoint_set_prop(sd_bus *bus, const char *path, } else if (strcmp(connectivity, "Degraded") == 0) { peer->degraded = true; } else { - printf("Invalid property value '%s' for property '%s' from interface '%s' on object '%s'\n", + warnx("Invalid property value '%s' for property '%s' from interface '%s' on object '%s'", connectivity, property, interface, path); rc = -EINVAL; goto out; @@ -2875,7 +2899,7 @@ static int bus_endpoint_set_prop(sd_bus *bus, const char *path, rc = sd_bus_emit_properties_changed(bus, path, interface, "Connectivity", NULL); } } else { - printf("Unknown property '%s' in interface '%s' on object '%s'\n", property, + warnx("Unknown property '%s' in interface '%s' on object '%s'", property, interface, path); rc = -ENOENT; } @@ -3058,17 +3082,9 @@ static int emit_net_removed(struct ctx *ctx, struct net *net) static int emit_interface_removed(struct link *link) { - int ifindex = link->ifindex; struct ctx *ctx = link->ctx; - const char* ifname = NULL; int rc; - ifname = mctp_nl_if_byindex(ctx->nl_query, ifindex); - if (!ifname) { - warnx("BUG %s: no interface for ifindex %d", __func__, ifindex); - return -EPROTO; - } - if (link->ctx->verbose) warnx("emitting interface remove: %s", link->path); @@ -3164,18 +3180,18 @@ static int del_local_eid(struct ctx *ctx, uint32_t net, int eid) peer = find_peer_by_addr(ctx, eid, net); if (!peer) { - warnx("BUG: local eid %d net %d to delete is missing", eid, net); + bug_warn("local eid %d net %d to delete is missing", eid, net); return -ENOENT; } if (peer->state != LOCAL) { - warnx("BUG: local eid %d net %d to delete is incorrect", eid, net); + bug_warn("local eid %d net %d to delete is incorrect", eid, net); return -EPROTO; } peer->local_count--; if (peer->local_count < 0) { - warnx("BUG: local eid %d net %d bad refcount %d", + bug_warn("local eid %d net %d bad refcount %d", eid, net, peer->local_count); } @@ -3216,7 +3232,7 @@ static int prune_old_nets(struct ctx *ctx) for (size_t p = 0; p < 256; p++) { // Sanity check that no peers are used if (ctx->nets[i]->peers[p]) { - warnx("BUG: stale entry for eid %zd in deleted net %d", + bug_warn("stale entry for eid %zd in deleted net %d", p, net->net); } } @@ -3242,6 +3258,9 @@ static int del_interface(struct link *link) for (size_t i = 0; i < ctx->num_peers; i++) { struct peer *p = ctx->peers[i]; if (p->state == REMOTE && p->phys.ifindex == ifindex) { + // Linux removes routes to deleted links, so no need to request removal. + p->have_neigh = false; + p->have_route = false; remove_peer(p); } } @@ -3288,13 +3307,13 @@ static int change_net_interface(struct ctx *ctx, int ifindex, uint32_t old_net) if (new_net == old_net) { // Logic below may assume they differ - warnx("BUG: %s called with new=old=%d", __func__, old_net); + bug_warn("%s called with new=old=%d", __func__, old_net); return -EPROTO; } old_n = lookup_net(ctx, old_net); if (!old_n) { - warnx("BUG: %s: Bad old net %d", __func__, old_net); + bug_warn("%s: Bad old net %d", __func__, old_net); return -EPROTO; } new_n = lookup_net(ctx, new_net); @@ -3317,13 +3336,13 @@ static int change_net_interface(struct ctx *ctx, int ifindex, uint32_t old_net) } if (peer->net != old_net) { - warnx("BUG: %s: Mismatch old net %d vs %d, new net %d", + bug_warn("%s: Mismatch old net %d vs %d, new net %d", __func__, peer->net, old_net, new_net); continue; } if (check_peer_struct(peer, old_n) != 0) { - warnx("BUG: %s: Inconsistent state", __func__); + bug_warn("%s: Inconsistent state", __func__); return -EPROTO; } @@ -3377,7 +3396,7 @@ static int add_local_eid(struct ctx *ctx, uint32_t net, int eid) rc = add_peer(ctx, &local_phys, eid, net, &peer); if (rc < 0) { - warn("BUG: Error adding local eid %d net %d", eid, net); + bug_warn("Error adding local eid %d net %d", eid, net); return rc; } peer->state = LOCAL; @@ -3457,7 +3476,7 @@ static int add_net(struct ctx *ctx, uint32_t net_id) struct net *net, **tmp; if (lookup_net(ctx, net_id) != NULL) { - warnx("BUG: add_net for existing net %d", net_id); + bug_warn("add_net for existing net %d", net_id); return -EEXIST; } @@ -3514,13 +3533,13 @@ static int add_interface(struct ctx *ctx, int ifindex) uint32_t net = mctp_nl_net_byindex(ctx->nl, ifindex); if (!net) { - warnx("Can't find link index %d\n", ifindex); + warnx("Can't find link index %d", ifindex); return -ENOENT; } const char *ifname = mctp_nl_if_byindex(ctx->nl, ifindex); if (!ifname) { - warnx("Can't find link name for index %d\n", ifindex); + warnx("Can't find link name for index %d", ifindex); return -ENOENT; } @@ -3805,13 +3824,7 @@ int main(int argc, char **argv) warnx("Failed creating netlink object"); return 1; } - - ctx->nl_query = mctp_nl_new(false); - if (!ctx->nl_query) { - warnx("Failed creating 2nd netlink object"); - return 1; - } - mctp_nl_warn_eexist(ctx->nl_query, false); + mctp_nl_warn_eexist(ctx->nl, false); /* D-Bus needs to be set up before setup_nets() so we can populate D-Bus objects for interfaces */ @@ -3856,7 +3869,6 @@ int main(int argc, char **argv) sd_bus_close(ctx->bus); mctp_nl_close(ctx->nl); - mctp_nl_close(ctx->nl_query); free_peers(ctx); free_nets(ctx); diff --git a/tests/mctp-ops-test.c b/tests/mctp-ops-test.c index 3cd7d5e8..e1638213 100644 --- a/tests/mctp-ops-test.c +++ b/tests/mctp-ops-test.c @@ -216,7 +216,14 @@ static int mctp_op_close(int sd) return close(sd); } -struct mctp_ops mctp_ops = { +static void mctp_bug_warn(const char* fmt, va_list args) +{ + vwarnx(fmt, args); + warnx("Aborting on bug in tests"); + abort(); +} + +const struct mctp_ops mctp_ops = { .mctp = { .socket = mctp_op_mctp_socket, .setsockopt = mctp_op_setsockopt, @@ -233,6 +240,7 @@ struct mctp_ops mctp_ops = { .recvfrom = mctp_op_recvfrom, .close = mctp_op_close, }, + .bug_warn = mctp_bug_warn, }; void mctp_ops_init(void) diff --git a/tests/mctpd/__init__.py b/tests/mctpd/__init__.py index b03cc086..e9eeef65 100644 --- a/tests/mctpd/__init__.py +++ b/tests/mctpd/__init__.py @@ -46,7 +46,7 @@ def __init__(self, name, ifindex, net, lladdr, min_mtu, max_mtu, up = False): self.name = name self.ifindex = ifindex self.net = net - self.lladdr = lladdr, + self.lladdr = lladdr self.min_mtu = min_mtu self.max_mtu = max_mtu self.mtu = max_mtu @@ -116,6 +116,16 @@ async def notify_interface(self, iface): await self.nl.notify_newlink(iface) async def del_interface(self, iface): + routes = list(filter(lambda x: x.iface == iface, self.routes)) + for x in routes: + await self.del_route(x) + neighbours = list(filter(lambda x: x.iface == iface, self.neighbours)) + for x in neighbours: + await self.del_neighbour(x) + addresses = list(filter(lambda x: x.iface == iface, self.addresses)) + for x in addresses: + await self.del_address(x) + self.interfaces.remove(iface) if self.nl: await self.nl.notify_dellink(iface) @@ -143,7 +153,7 @@ async def add_neighbour(self, neigh): async def del_neighbour(self, neigh): neigh = self.lookup_neighbour(neigh.iface, neigh.eid) if not neigh: - raise NetlinkError(errno.EENOENT) + raise NetlinkError(errno.ENOENT) self.neighbours.remove(neigh) if self.nl: await self.nl.notify_delneigh(neigh) @@ -199,7 +209,8 @@ def find_endpoint(self, addr): iface = route.iface neigh = self.lookup_neighbour(route.iface, addr.eid) - lladdr = neigh.lladdr + # if no neighbour, return an empty lladdr (eg mctpusb) + lladdr = neigh.lladdr if neigh else bytes() if iface is None or lladdr is None: return None @@ -387,7 +398,7 @@ class af_spec_mctp(netlink.nla): ) class l2addr(netlink.nla_base): - fields = [('value', 'c')] + fields = [('value', 's')] class ifaddrmsg_mctp(rtnl.ifaddrmsg.ifaddrmsg): nla_map = ( diff --git a/tests/pytest.ini.in b/tests/pytest.ini.in index 681985b4..31856226 100644 --- a/tests/pytest.ini.in +++ b/tests/pytest.ini.in @@ -1,3 +1,6 @@ [pytest] trio_mode = true testpaths = @testpaths@ +filterwarnings = + ignore:.*wait_readable:DeprecationWarning:asyncdbus + ignore:.*wait_writable:DeprecationWarning:asyncdbus diff --git a/tests/test_mctpd.py b/tests/test_mctpd.py index e2550b16..5ed5994e 100644 --- a/tests/test_mctpd.py +++ b/tests/test_mctpd.py @@ -636,3 +636,56 @@ async def test_del_interface_last(dbus, mctpd): # network should be gone with pytest.raises(asyncdbus.errors.DBusError): await mctpd_mctp_network_obj(dbus, iface.net) + +""" Remove and re-add an interface """ +async def test_add_interface(dbus, mctpd): + net = 1 + # Create a new netdevice + iface = mctpd.system.Interface('mctpnew', 10, net, bytes([]), 68, 254, True) + await mctpd.system.add_interface(iface) + await mctpd.system.add_address(mctpd.system.Address(iface, 88)) + mctp = await mctpd_mctp_iface_obj(dbus, iface) + + # Add an endpoint on the interface + mctpd.network.add_endpoint(Endpoint(iface, bytes([]), types = [0, 1])) + + static_eid = 30 + (eid, _, _, new) = await mctp.call_assign_endpoint_static( + bytes([]), + static_eid + ) + assert eid == static_eid + assert new + print(f"routes {mctpd.system.routes}") + assert mctpd.system.lookup_route(net, static_eid).iface == iface + + # Remove the netdevice + await mctpd.system.del_interface(iface) + + # Interface should be gone + with pytest.raises(asyncdbus.errors.DBusError): + await mctpd_mctp_iface_obj(dbus, iface) + assert mctpd.system.lookup_route(net, static_eid) is None + + # Re-add the same interface name again, with a new ifindex 11 + iface = mctpd.system.Interface('mctpnew', 11, net, bytes([]), 68, 254, True) + await mctpd.system.add_interface(iface) + await mctpd.system.add_address(mctpd.system.Address(iface, 89)) + mctp = await mctpd_mctp_iface_obj(dbus, iface) + + # Add an endpoint on the interface + mctpd.network.add_endpoint(Endpoint(iface, bytes([]), types = [0, 1])) + + # Old route should still be gone + assert mctpd.system.lookup_route(net, static_eid) is None + + static_eid = 40 + (eid, _, _, new) = await mctp.call_assign_endpoint_static( + bytes([]), + static_eid + ) + assert eid == static_eid + assert new + assert mctpd.system.lookup_route(net, static_eid).iface == iface + + print(mctpd)