On 16:51 Fri 30 Oct , Hal Rosenstock wrote: > On Fri, Oct 30, 2009 at 4:17 PM, Sasha Khapyorsky <[email protected]> wrote: > > On 11:48 Fri 30 Oct , Hal Rosenstock wrote: > >> >> @@ -819,6 +821,13 @@ osm_vendor_bind(IN osm_vendor_t * const p_vend, > >> >> p_bind->send_err_callback = send_err_callback; > >> >> p_bind->p_mad_pool = p_mad_pool; > >> >> p_bind->port_guid = port_guid; > >> >> + if (p_vend->timeout == -1) { > >> >> + p_bind->timeout = p_user_bind->timeout; > >> >> + p_bind->max_retries = p_user_bind->retries; > >> >> + } else { > >> >> + p_bind->timeout = p_vend->timeout; > >> >> + p_bind->max_retries = p_vend->max_retries; > >> >> + } > >> > > >> > Hmm, shouldn't we respect user requested data? Something like: > >> > > >> > p_bind->timeout = p_user_bind->timeout ? p_user_bind->timeout : > >> > p_vend->timeout; > >> > p_bind->retries = p_user_bind->retries ? p_user_bind->retries : > >> > p_vend->retries; > >> > > >> > ? > >> > >> The -1 is for the new ABI. > > > > Could you elaborate? > > In order to be able to deal with combinations of old/new opensm/vendor layer.
I see, you are trying to protect old OpenSM <-> new vendor case. Such combination will not work for many reasons (we changed OpenSM/vendor interaction couple of times in past few years, including configuration) and it is better to avoid such usage. > >> >> @@ -392,8 +393,7 @@ ib_api_status_t osm_opensm_init(IN osm_opensm_t * > >> >> p_osm, > >> >> if (status != IB_SUCCESS) > >> >> goto Exit; > >> >> > >> >> - p_osm->p_vendor = > >> >> - osm_vendor_new(&p_osm->log, p_opt->transaction_timeout); > >> >> + p_osm->p_vendor = osm_vendor_new(&p_osm->log, -1); > >> > > >> > Why to not make p_opt->transaction_timeout as default for newly > >> > allocated vendor object? > >> > >> To support old and new ABI. > > > > What do you mean? OpenSM will run against older vendor? > > > > Even if so how '-1' is helpful? Likely it will break things, no? > > -1 is not a valid timeout so can't this be used in this manner ? How > will it break things ? Old vendor will not work with '-1'. OTOH this enforces all vendor users to set all parameter explicitly, and ignores those values when a value other than '-1' was used in vendor creation. In fact it enforces using '-1' and effectively makes it to be mandatory part of vendor API. Seems too messy for me. Sasha -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
