09/10/2018 11:31, Andrew Rybchenko:
> On 10/9/18 3:16 AM, Thomas Monjalon wrote:
> > if (str != NULL) {
> > - kvargs = rte_kvargs_parse(str, eth_params_keys);
> > + if (str[0] == '+') /* no validation of keys */
> > + str ++;
>
> As I understand it should be no space before ++
Yes, I ran checkpatch after sending the patches :(
[...]
> > + /*
> > + * Assume parameters of old syntax can match only at ethdev level.
> > + * Extra parameters will be ignored, thanks to "+" prefix.
> > + */
> > + str_size = strlen(devargs.args) + 2;
> > + cls_str = malloc(str_size);
> > + if (cls_str == NULL) {
> > + ret = -ENOMEM;
> > + goto error;
> > + }
> > + ret = snprintf(cls_str, str_size, "+%s", devargs.args);
> > + if (ret < 0) {
>
> As I understand we expect ret == str_size - 1 here. May be it makes sent
> to harden the check.
No strong opinion. OK I'll change the check.
> > + ret = -EINVAL;
> > + goto error;
> > + }
> > + iter->cls_str = cls_str;
> > + free(devargs.args); /* allocated by rte_devargs_parse() */
> > + devargs.args = NULL;
> > +
> > + iter->bus = devargs.bus;
> > + if (iter->bus->dev_iterate == NULL) {
> > + ret = -ENOTSUP;
> > + goto error;
> > + }
> > +
> > + /* Convert bus args to new syntax for use with new API dev_iterate. */
> > + if (strcmp(iter->bus->name, "vdev") == 0)
> > + bus_param_key = "name";
> > + else if (strcmp(iter->bus->name, "pci") == 0)
> > + bus_param_key = "addr";
>
> I thought that if one branch has curly brackets other branches should
> have curly brackets as well.
Yes, I don't like this coding rule but I will change.
> > + else {
> > + ret = -ENOTSUP;
> > + goto error;
> > + }
> > + str_size = strlen(bus_param_key) + strlen(devargs.name) + 2;
> > + bus_str = malloc(str_size);
> > + if (bus_str == NULL) {
> > + ret = -ENOMEM;
> > + goto error;
> > + }
> > + ret = snprintf(bus_str, str_size, "%s=%s",
> > + bus_param_key, devargs.name);
> > + if (ret < 0) {
>
> May be it makes sense to make the check more strict: ret != str_size - 1
OK
> > + ret = -EINVAL;
> > + goto error;
> > + }
[...]
> > +void __rte_experimental
> > +rte_eth_iterator_free(struct rte_dev_iterator *iter)
>
> Yes, I know that the name is suggested by me, but maybe
> it should be rte_eth_iterator_fini() or _cleanup() as pair to _init.
Yes, you're right, it is not freeing the whole structure.
I will name it "cleanup", and will use memset to reset all fields.
> > +{
> > + free(RTE_CAST_FIELD(iter, bus_str, char *)); /* workaround const */
> > + iter->bus_str = NULL;
> > + free(RTE_CAST_FIELD(iter, cls_str, char *)); /* workaround const */
> > + iter->cls_str = NULL;
> > +}
[...]
> > +/**
> > + * @warning
> > + * @b EXPERIMENTAL: this API may change without prior notice.
> > + *
> > + * Free some allocated fields of the iterator.
> > + *
> > + * This function is automatically called by rte_eth_iterator_next()
> > + * on the last iteration (i.e. when no more matching port is found).
> > + *
>
> May be it makes sense to mention that it is safe to call it twice.
> It could be simpler to use it taking the possibility into account.
OK, good idea.
> > + * @param iter
> > + * Device iterator handle initialized by rte_eth_iterator_init().
> > + * The fields bus_str and cls_str are freed if needed.
> > + */
> > +__rte_experimental
> > +void rte_eth_iterator_free(struct rte_dev_iterator *iter);
[...]
> > --- a/lib/librte_ethdev/rte_ethdev_version.map
> > +++ b/lib/librte_ethdev/rte_ethdev_version.map
> > @@ -237,6 +237,8 @@ EXPERIMENTAL {
> > rte_eth_dev_owner_unset;
> > rte_eth_dev_rx_offload_name;
> > rte_eth_dev_tx_offload_name;
> > + rte_eth_iterator_init;
> > + rte_eth_iterator_next;
>
> rte_eth_iterator_free() or renamed
Yes, good catch!
As usual, thanks for the good review Andrew.
