Hi Florian, I agree with your last change. One goal is to keep code volumes down, and then I think a switch is more appropriate than code duplication.
As for your earlier question about the node_lock, (sorry for not responding earlier); It is there to protect against parallel access to the link object when doing tipc_link_send_proto_msg(). I can not actually see any case where it would be harmful to send two messages simultaneously, if one of them is a protocol message, or sending a protocol message while receiving another message. But it is a while since I worked with this code, and I think it is better to be safe than sorry here. At least we should analyze the code and be 100% certain that there can be no race conditions, (e.g. receiving a state-changing message at the same time as we are sending the protocol message). Otherwise I think your patch looks ok. Regards ///jon -----Original Message----- From: [EMAIL PROTECTED] [mailto:[EMAIL PROTECTED] On Behalf Of Florian Westphal Sent: February 17, 2008 3:10 PM To: [email protected] Subject: Re: [tipc-discussion] [PATCH] Allow configuration of default bearerpriority/window size/tolerance Florian Westphal <[EMAIL PROTECTED]> wrote: > This adds three helpers for pri/windowsize/tolerance, that share a lot > of common code. Basically you either have to live with the duplicate > code or add rather fragile "switch (cmd_type)" conditionals in order > to figure out what value has to be checked/changed. Actually the latter isn't as bad as i thought; so here it is: Enable configuration of default bearer priority/window size/tolerance Allows to override the compile-time default values of link tolerance, priority and window size. use tipc-config -lt=<link-name|bearer-name|bearer-type>/<tolerance> tipc-config -lp=<link-name|bearer-name|bearer-type>/<prio> tipc-config -lw=<link-name|bearer-name|bearer-type>/<window> to set the desired values. --- net/tipc/tipc_bearer.c | 10 ++- net/tipc/tipc_bearer.h | 9 +++- net/tipc/tipc_link.c | 152 ++++++++++++++++++++++++++++++++++------------- 3 files changed, 124 insertions(+), 47 deletions(-) diff --git a/net/tipc/tipc_bearer.c b/net/tipc/tipc_bearer.c index 3cbca21..033eacd 100644 --- a/net/tipc/tipc_bearer.c +++ b/net/tipc/tipc_bearer.c @@ -69,10 +69,10 @@ static int media_name_valid(const char *name) } /** - * media_find_name - locates specified media object by name + * tipc_media_find_name - locates specified media object by name */ -static struct tipc_media *media_find_name(const char *name) +struct tipc_media *tipc_media_find_name(const char *name) { u32 i; @@ -139,7 +139,7 @@ int tipc_register_media(struct tipc_media *m_ptr) if (media_count >= MAX_MEDIA) { goto exit; } - if (media_find_id(m_ptr->media_id) || media_find_name(m_ptr->name)) { + if (media_find_id(m_ptr->media_id) || +tipc_media_find_name(m_ptr->name)) { goto exit; } @@ -574,7 +574,7 @@ int tipc_enable_bearer(const char *name, u32 disc_domain, u32 priority) write_lock_bh(&tipc_net_lock); - m_ptr = media_find_name(b_name.media_name); + m_ptr = tipc_media_find_name(b_name.media_name); if (!m_ptr) { warn("Bearer <%s> rejected, media <%s> not registered\n", name, b_name.media_name); @@ -628,6 +628,8 @@ restart: b_ptr->net_plane = bearer_id + 'A'; b_ptr->active = 1; b_ptr->priority = priority; + b_ptr->tolerance = m_ptr->tolerance; + b_ptr->window = m_ptr->window; INIT_LIST_HEAD(&b_ptr->cong_links); INIT_LIST_HEAD(&b_ptr->links); INIT_LIST_HEAD(&b_ptr->disc_list); diff --git a/net/tipc/tipc_bearer.h b/net/tipc/tipc_bearer.h index 80aba31..aac5ba1 100644 --- a/net/tipc/tipc_bearer.h +++ b/net/tipc/tipc_bearer.h @@ -46,6 +46,8 @@ * @publ: bearer information available to privileged users * @media: ptr to media structure associated with bearer * @priority: default link priority for bearer + * @window: default window size for bearer + * @tolerance: default link tolerance for bearer * @identity: array index of this bearer within TIPC bearer array * @disc_list: list of neighbor discovery objects * @links: list of non-congested links associated with bearer @@ -60,12 +62,14 @@ struct bearer { struct tipc_bearer publ; struct tipc_media *media; u32 priority; + u32 window; + u32 tolerance; u32 identity; struct list_head disc_list; struct list_head links; struct list_head cong_links; u32 continue_count; - int active; + char active; char net_plane; struct node_map nodes; }; @@ -79,6 +83,8 @@ struct link; extern struct bearer *tipc_bearers; +int tipc_media_set_priority(const char *name, u32 new_value); int +tipc_media_set_window(const char *name, u32 new_value); void tipc_media_addr_printf(struct print_buf *pb, struct tipc_media_addr *a); struct sk_buff *tipc_media_get_names(void); @@ -90,6 +96,7 @@ void tipc_bearer_remove_dest(struct bearer *b_ptr, u32 dest, void tipc_bearer_remove_discoverer(struct bearer *b_ptr, u32 dest); void tipc_bearer_schedule(struct bearer *b_ptr, struct link *l_ptr); struct bearer *tipc_bearer_find(const char *name); +struct tipc_media *tipc_media_find_name(const char *name); int tipc_bearer_resolve_congestion(struct bearer *b_ptr, struct link *l_ptr); int tipc_bearer_congested(struct bearer *b_ptr, struct link *l_ptr); int tipc_bearer_init(void); diff --git a/net/tipc/tipc_link.c b/net/tipc/tipc_link.c index 79beeed..bf7a794 100644 --- a/net/tipc/tipc_link.c +++ b/net/tipc/tipc_link.c @@ -425,7 +425,7 @@ struct link *tipc_link_create(struct bearer *b_ptr, const u32 peer, memcpy(&l_ptr->media_addr, media_addr, sizeof(*media_addr)); l_ptr->checkpoint = 1; l_ptr->b_ptr = b_ptr; - link_set_supervision_props(l_ptr, b_ptr->media->tolerance); + link_set_supervision_props(l_ptr, b_ptr->tolerance); l_ptr->state = RESET_UNKNOWN; l_ptr->pmsg = (struct tipc_msg *)&l_ptr->proto_msg; msg = l_ptr->pmsg; @@ -435,7 +435,7 @@ struct link *tipc_link_create(struct bearer *b_ptr, const u32 peer, msg_set_bearer_id(msg, b_ptr->identity); strcpy((char *)msg_data(msg), if_name); l_ptr->priority = b_ptr->priority; - tipc_link_set_queue_limits(l_ptr, b_ptr->media->window); + tipc_link_set_queue_limits(l_ptr, b_ptr->window); link_init_max_pkt(l_ptr); l_ptr->next_out_no = 1; INIT_LIST_HEAD(&l_ptr->waiting_ports); @@ -456,7 +456,7 @@ struct link *tipc_link_create(struct bearer *b_ptr, const u32 peer, dbg("tipc_link_create(): tolerance = %u,cont intv = %u, abort_limit = %u\n", l_ptr->tolerance, l_ptr->continuity_interval, l_ptr->abort_limit); - + return l_ptr; } @@ -2981,13 +2981,113 @@ static struct link *link_find_link(const char *name, struct node **node) return l_ptr; } + +/** + * value_is_valid -- check if priority/link tolerance/window is within +range + * + * @cmd - value type (TIPC_CMD_SET_LINK_*) + * @new_value - the new value + * + * Returns true if value is within range. + */ +static bool value_is_valid(u16 cmd, u32 new_value) { + switch (cmd) { + case TIPC_CMD_SET_LINK_TOL: + if ((new_value >= TIPC_MIN_LINK_TOL) && + (new_value <= TIPC_MAX_LINK_TOL)) + return true; + break; + case TIPC_CMD_SET_LINK_PRI: + if ((new_value >= TIPC_MIN_LINK_PRI) && + (new_value <= TIPC_MAX_LINK_PRI)) + return true; + break; + case TIPC_CMD_SET_LINK_WINDOW: + if ((new_value >= TIPC_MIN_LINK_WIN) && + (new_value <= TIPC_MAX_LINK_WIN)) + return true; + } + return false; +} + +/** + * cmd_set_link_value - change priority/tolerance/window size of link, +bearer or media + * @name - ptr to link, bearer or media name string + * @new_value - the new link priority or new bearer default link +priority + * @cmd - which link/bearer property to set (TIPC_CMD_SET_LINK_*) + * + * Caller must hold 'tipc_net_lock' to ensure link/bearer are not deleted. + * + * Returns 0 if value updated and negative value on error. + * -EINVAL is returned if new_value is invalid. + */ +static int cmd_set_link_value(const char *name, u32 new_value, u16 cmd) +{ + struct node *node; + struct link *l_ptr; + struct bearer *b_ptr; + struct tipc_media *m_ptr; + + l_ptr = link_find_link(name, &node); + if (l_ptr) { + switch (cmd) { + case TIPC_CMD_SET_LINK_TOL: + link_set_supervision_props(l_ptr, new_value); + break; + case TIPC_CMD_SET_LINK_PRI: + l_ptr->priority = new_value; + break; + case TIPC_CMD_SET_LINK_WINDOW: + tipc_link_set_queue_limits(l_ptr, new_value); + return 0; + default: + return -EINVAL; + } + tipc_link_send_proto_msg(l_ptr, STATE_MSG, + 0, 0, new_value, 0, 0, 0); + return 0; + } + + b_ptr = tipc_bearer_find(name); + if (b_ptr) { + switch (cmd) { + case TIPC_CMD_SET_LINK_TOL: + b_ptr->tolerance = new_value; + return 0; + case TIPC_CMD_SET_LINK_PRI: + b_ptr->priority = new_value; + return 0; + case TIPC_CMD_SET_LINK_WINDOW: + b_ptr->window = new_value; + return 0; + } + return -EINVAL; + } + + m_ptr = tipc_media_find_name(name); + if (!m_ptr) + return -ENODEV; + switch (cmd) { + case TIPC_CMD_SET_LINK_TOL: + m_ptr->tolerance = new_value; + return 0; + case TIPC_CMD_SET_LINK_PRI: + m_ptr->priority = new_value; + return 0; + case TIPC_CMD_SET_LINK_WINDOW: + m_ptr->window = new_value; + return 0; + } + return -EINVAL; +} + + struct sk_buff *tipc_link_cmd_config(const void *req_tlv_area, int req_tlv_space, u16 cmd) { struct tipc_link_config *args; u32 new_value; - struct link *l_ptr; - struct node *node; int res; if (!TLV_CHECK(req_tlv_area, req_tlv_space, TIPC_TLV_LINK_CONFIG)) @@ -2996,6 +3096,9 @@ struct sk_buff *tipc_link_cmd_config(const void *req_tlv_area, int req_tlv_space args = (struct tipc_link_config *)TLV_DATA(req_tlv_area); new_value = ntohl(args->value); + if (!value_is_valid(cmd, new_value)) + return tipc_cfg_reply_error_string("cannot change, value invalid"); + if (!strcmp(args->name, tipc_bclink_name)) { if ((cmd == TIPC_CMD_SET_LINK_WINDOW) && (tipc_bclink_set_queue_limits(new_value) == 0)) @@ -3005,47 +3108,12 @@ struct sk_buff *tipc_link_cmd_config(const void *req_tlv_area, int req_tlv_space } read_lock_bh(&tipc_net_lock); - l_ptr = link_find_link(args->name, &node); - if (!l_ptr) { - read_unlock_bh(&tipc_net_lock); - return tipc_cfg_reply_error_string("link not found"); - } - node_lock(node); - res = -EINVAL; - switch (cmd) { - case TIPC_CMD_SET_LINK_TOL: - if ((new_value >= TIPC_MIN_LINK_TOL) && - (new_value <= TIPC_MAX_LINK_TOL)) { - link_set_supervision_props(l_ptr, new_value); - tipc_link_send_proto_msg(l_ptr, STATE_MSG, - 0, 0, new_value, 0, 0, 0); - res = TIPC_OK; - } - break; - case TIPC_CMD_SET_LINK_PRI: - if ((new_value >= TIPC_MIN_LINK_PRI) && - (new_value <= TIPC_MAX_LINK_PRI)) { - l_ptr->priority = new_value; - tipc_link_send_proto_msg(l_ptr, STATE_MSG, - 0, 0, 0, new_value, 0, 0); - res = TIPC_OK; - } - break; - case TIPC_CMD_SET_LINK_WINDOW: - if ((new_value >= TIPC_MIN_LINK_WIN) && - (new_value <= TIPC_MAX_LINK_WIN)) { - tipc_link_set_queue_limits(l_ptr, new_value); - res = TIPC_OK; - } - break; - } - node_unlock(node); + res = cmd_set_link_value(args->name, new_value, cmd); read_unlock_bh(&tipc_net_lock); if (res) - return tipc_cfg_reply_error_string("cannot change link setting"); - + return tipc_cfg_reply_error_string("link or bearer not found"); return tipc_cfg_reply_none(); } ------------------------------------------------------------------------ - This SF.net email is sponsored by: Microsoft Defy all challenges. Microsoft(R) Visual Studio 2008. http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/ _______________________________________________ tipc-discussion mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/tipc-discussion ------------------------------------------------------------------------- This SF.net email is sponsored by: Microsoft Defy all challenges. Microsoft(R) Visual Studio 2008. http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/ _______________________________________________ tipc-discussion mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/tipc-discussion
