On Fri, Oct 30, 2009 at 5:48 PM, Sasha Khapyorsky <[email protected]> wrote: > 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.
OK; I'll update the patch then. -- Hal > 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
