Re: [PATCH v2 2/6] ethdev: add iterator to match devargs input

[Thomas Monjalon <thomas@monjalon.net>]

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.