> From: Gaëtan Rivet [mailto:gaetan.ri...@6wind.com]
> Sent: Tuesday, August 28, 2018 5:40 PM
> To: Power, Ciara <ciara.po...@intel.com>
> Cc: Van Haaren, Harry <harry.van.haa...@intel.com>; Archbold, Brian
> <brian.archb...@intel.com>; Kenny, Emma <emma.ke...@intel.com>; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH 02/11] telemetry: add initial connection
> socket
<snip>
> > +int32_t
> > +rte_telemetry_check_port_activity(int port_id)
> > +{
> > + int pid;
> > +
> > + RTE_ETH_FOREACH_DEV(pid) {
> > + if (pid == port_id)
> > + return 1;
> > + }
> > + TELEMETRY_LOG_ERR("Error - port_id: %d is invalid, not active\n",
> > + port_id);
> > + return 0;
> > +}
> > +
>
> This function seems more about ethdev than telemetry.
> Maybe add it as part of librte_ethdev.
Yep that might be a better place, making it a generic ethdev function.
> The "active" semantic is blurry however. With this implementation, this
> is checking that the device is currently attached and owned by the
> default user. Should telemetry be limited to devices owned by default
> user?
Good question - perhaps one that we can get input on from others.
Would you (app X) want App Y to read telemetry from your ethdev/cryptodev?
Perhaps keeping telemetry to an "owned" port is a feature, perhaps
a limitation.
I'd like input on this one :D
> Also, this function does not seem used in this patch, can it be added
> when used?
Sure.
> > +static int32_t
> > +rte_telemetry_reg_ethdev_to_metrics(uint16_t port_id)
>
> "_to" might not be necessary.
>
> > +{
> > + int ret, num_xstats, start_index, i;
> > + struct rte_eth_xstat *eth_xstats;
> > +
> > + if (!rte_eth_dev_is_valid_port(port_id)) {
> > + TELEMETRY_LOG_ERR("Error - port_id: %d is invalid\n", port_id);
> > + return -EINVAL;
> > + }
> > +
> > + num_xstats = rte_eth_xstats_get(port_id, NULL, 0);
> > + if (num_xstats < 0) {
> > + TELEMETRY_LOG_ERR("Error - rte_eth_xstats_get(%u) failed:"
> > + " %d\n", port_id, num_xstats);
>
> I guess there isn't really a consensus yet, as the checkpatch.sh tool
> might be misconfigured, but the cesura is very awkward here.
>
> Same for other logs.
Agreed, improvements possible here.
> > + return -EPERM;
> > + }
> > +
> > + eth_xstats = malloc(sizeof(struct rte_eth_xstat) * num_xstats);
> > + if (eth_xstats == NULL) {
> > + TELEMETRY_LOG_ERR("Error - Failed to malloc memory for"
> > + " xstats\n");
> > + return -ENOMEM;
> > + }
> > + ret = rte_eth_xstats_get(port_id, eth_xstats, num_xstats);
> > + if (ret < 0 || ret > num_xstats) {
> > + free(eth_xstats);
> > + TELEMETRY_LOG_ERR("Error - rte_eth_xstats_get(%u) len%i"
> > + " failed: %d\n", port_id, num_xstats, ret);
> > + return -EPERM;
> > + }
> > + struct rte_eth_xstat_name *eth_xstats_names;
> > + eth_xstats_names = malloc(sizeof(struct rte_eth_xstat_name) *
> > + num_xstats);
> > + if (eth_xstats_names == NULL) {
>
> You are sometimes checking pointers against NULL, sometimes using "!".
> You can choose either in your library, but it would be better to be
> consistent and use a unified coding style.
Heh, I guess that's down to dev-style, and you'll note multiple sign-offs ;)
Can review for v2.
> > + free(eth_xstats);
> > + TELEMETRY_LOG_ERR("Error - Failed to malloc memory for"
> > + " xstats_names\n");
> > + return -ENOMEM;
> > + }
> > + ret = rte_eth_xstats_get_names(port_id, eth_xstats_names,
> > + num_xstats);
> > + if (ret < 0 || ret > num_xstats) {
> > + free(eth_xstats);
> > + free(eth_xstats_names);
> > + TELEMETRY_LOG_ERR("Error - rte_eth_xstats_get_names(%u)"
> > + " len%i failed: %d\n", port_id, num_xstats,
> > + ret);
> > + return -EPERM;
> > + }
> > + const char *xstats_names[num_xstats];
> > +
> > + for (i = 0; i < num_xstats; i++)
> > + xstats_names[i] = eth_xstats_names[eth_xstats[i].id].name;
> > +
> > + start_index = rte_metrics_reg_names(xstats_names, num_xstats);
> > +
> > + if (start_index < 0) {
> > + TELEMETRY_LOG_ERR("Error - rte_metrics_reg_names failed -"
> > + " metrics may already be registered\n");
> > + free(eth_xstats);
> > + free(eth_xstats_names);
> > + return -1;
> > + }
> > + free(eth_xstats_names);
> > + free(eth_xstats);
>
> At this point, you have repeated 3 times those frees().
> It would be cleaner to define proper labels and use goto instead.
Agreed.
Thanks for review!
