On Tue, 11 May 2010 09:42:34 -0700 Sasha Khapyorsky <sas...@voltaire.com> wrote:
> On 13:53 Mon 10 May , Ira Weiny wrote: > > > > > > > > int ibnd_discover_fabric(ibnd_fabric_t **fabric, > > > > cosnt char *ca_name, <== could we even > > > > default this? > > > > > > I would think about ca_name and port_number. And this is of course may > > > have default (NULL, 0). > > > > Ok, ca_name and ca_port will be explicit. > > > > > > > > > struct ibnd_config *cfg); > > > > > > What is wrong with current ibdn_fabric_t *ibnd_discover_fabric(...)? Why > > > do we need an extra parameter? > > > > Well we are breaking the interface again so I figure we might as well clean > > some things up. Returning an int allows us to have a reason for the > > failure returned to the caller rather than just "NULL". We have cleaned up > > most of the internals of the library to allow for this. > > But we want to keep API simple, no? Ok, patch to follow. > > > > > > > > > > > > > > I don't mind the ibnd_config_t struct but I don't think it should be > > > > visible > > > > to the user. Make it opaque and use "set" functions. Something like. > > > > > > > > ibnd_fabric_t *fabric; > > > > ibnd_config_t cfg; > > > > ib_portid_t * from; > > > > > > > > ibnd_set_hops(&cfg, hops); <== default -1 > > > > ibnd_set_port_num(&cfg, port_num); <== default 1 > > > > ibnd_set_max_smps(&cfg, max_smps); <== default 2 > > > > ibnd_set_from_node(&cfg, from); <== default NULL > > > > > > I would prefer to not complicate API with ibnd_set_this() helpers. It > > > would be necessary to add new ones in the future which will lead to API > > > changes. > > > > See below. > > > > > > > > > if (ibnd_discover_fabric(&fabric, "foo", &cfg)) { <== anything not in > > > > cfg is > > > > defaulted here > > > > fprintf(stderr, "Wow it failed\n"); > > > > } > > > > > > > > This allows us to change ibnd_config structure any time we want without > > > > affecting the API. I don't think the "pad" you used is a good idea. > > > > > > Without padding we will break ABI each time when new field is added to > > > the config structure. > > > > No it does not iff you use the ibnd_set_this() helpers and make the config > > private. > > In you example 'ibnd_config_t cfg' is on the stack... :) yes, bad example. > > I would really suggest to keep API simple and useful - to fill up > a structure described in header files is much simpler than pass over > various helpers calls. I disagree but since you are looking to branch I think this bug needs to be fixed. A patch is following this email, Ira > > Sasha > > > > > > > > > > Also since we are breaking the API we might as well return the fabric > > > > as a > > > > parameter and have an error code. But I could go either way on this > > > > one. > > > > > > > > Ira > > > > > > > > > > > > [*] query_smp.c probably should have it's own timeout here but we can > > > > discuss > > > > later. > > > > > > > > [#] What sucks about this is that libibmad already has the > > > > functionality to > > > > open the umad port and configure it (50 line function). Now we will be > > > > duplicating this functionality. > > > > > > I think you can use mad_rpc_open_port(ca_name, port_number, ...) just > > > fine (and so the rest of libibmad stuff) - it will open separate fd. > > > > Yes for the libibmad functionality we can do that. I was speaking of the > > use of the umad layer. To use that layer we have to duplicate the > > functionality of mad_rpc_open_port in every tool which requires umad layer > > access, correct? Right now we are mixing the 2 layers (mad and umad) in > > saquery (get_bind_handle) as well as libibnetdisc. > > > > I think we need to be careful we don't do this again! > > > > Ira > > > > > > > > Sasha > > > -- > > > To unsubscribe from this list: send the line "unsubscribe linux-rdma" in > > > the body of a message to majord...@vger.kernel.org > > > More majordomo info at http://**vger.kernel.org/majordomo-info.html > > > > > > > > > -- > > Ira Weiny > > Math Programmer/Computer Scientist > > Lawrence Livermore National Lab > > 925-423-8008 > > wei...@llnl.gov > > > -- > To unsubscribe from this list: send the line "unsubscribe linux-rdma" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://*vger.kernel.org/majordomo-info.html > -- Ira Weiny Math Programmer/Computer Scientist Lawrence Livermore National Lab 925-423-8008 wei...@llnl.gov -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html