On Tue, 11 May 2010 09:42:34 -0700
Sasha Khapyorsky <[email protected]> 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 [email protected]
> > > More majordomo info at http://**vger.kernel.org/majordomo-info.html
> > >
> >
> >
> > --
> > Ira Weiny
> > Math Programmer/Computer Scientist
> > Lawrence Livermore National Lab
> > 925-423-8008
> > [email protected]
> >
> --
> 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
>
--
Ira Weiny
Math Programmer/Computer Scientist
Lawrence Livermore National Lab
925-423-8008
[email protected]
--
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
