On Tue, Jun 12, 2018 at 01:43:18PM +0000, Xueming(Steven) Li wrote:
<snip>
> > > > void *tmp;
> > > > unsigned int i;
> > > > unsigned int j = 0;
> > > > unsigned int n = 0;
> > > > int ret;
> > > >
> > > > + if (dpdk_dev->devargs) {
> > > > + ret = rte_eth_devargs_parse(dpdk_dev->devargs->args,
> > > > ð_da);
> > > > + if (ret)
> > > > + goto error;
> > > > + } else {
> > > > + memset(ð_da, 0, sizeof(eth_da));
> > > > + }
> > > > next:
> > > > + if (j) {
> > > > + unsigned int k;
> > > > +
> > > > + for (k = 0; k < eth_da.nb_representor_ports; ++k)
> > > > + if (eth_da.representor_ports[k] == j - 1)
> > > > + break;
> > > > + if (k == eth_da.nb_representor_ports)
> > > > + goto skip;
> > > > + }
> > > > errno = 0;
> > > > ctx = mlx5_glue->open_device(ibv_dev[j]);
> > >
> > > Need a range check for j here.
> >
> > I think it's properly checked. j == 0 stands for "master device", always
> > found at index 0 and probed.
> > Representors devices, if any, start at index 1 which triggers the previous
> > block. This block makes
> > sure that a given representor is indeed enabled before either spawning the
> > related device (pass
> > through with a valid "j") or skipping it altogether (goto skip).
>
> Yes, this code looks good. What I wanted to ask what if dev args specify an
> invalid rep id, e.g. 33.
> This code walk through silently w/o warning, it works, but it better to have
> a warning if input id out of range.
You're right. On the other hand this provides a means to spawn all
representors without necessarily knowing how many can be instantiated first,
e.g. by always providing a "representor=[0-31]" argument, since no special
keyword is defined to request them all.
Not saying it's a good or bad thing, but somewhat harmless. Just like
specifying "-w {DBDF}" arguments with invalid addresses, nonexistent
representors are silently ignored.
In any case, this can be improved later. We're already seeing a couple of
limitations with the representor argument, namely the lack of hot-plug
support, which will need to be addressed as well.
--
Adrien Mazarguil
6WIND
