Re: [PATCH 02/11] telemetry: add initial connection socket

["Gaëtan Rivet" <gaetan.rivet@6wind.com>]

Hi Ciara,

On Thu, Aug 23, 2018 at 01:08:04PM +0100, Ciara Power wrote:
> This patch adds the telemetry UNIX socket. It is used to
> allow connections from external clients.
> 
> On the initial connection from a client, ethdev stats are
> registered in the metrics library, to allow for their retrieval
> at a later stage.
> 
> Signed-off-by: Ciara Power <ciara.power@intel.com>
> Signed-off-by: Brian Archbold <brian.archbold@intel.com>
> ---
>  lib/librte_telemetry/rte_telemetry.c          | 205 ++++++++++++++++++++++++++
>  lib/librte_telemetry/rte_telemetry_internal.h |   4 +
>  2 files changed, 209 insertions(+)
> 
> diff --git a/lib/librte_telemetry/rte_telemetry.c b/lib/librte_telemetry/rte_telemetry.c
> index 8d7b0e3..f984929 100644
> --- a/lib/librte_telemetry/rte_telemetry.c
> +++ b/lib/librte_telemetry/rte_telemetry.c
> @@ -3,21 +3,159 @@
>   */
>  
>  #include <unistd.h>
> +#include <fcntl.h>
> +#include <sys/socket.h>
> +#include <sys/un.h>
>  
>  #include <rte_eal.h>
>  #include <rte_ethdev.h>
>  #include <rte_metrics.h>
> +#include <rte_string_fns.h>
>  
>  #include "rte_telemetry.h"
>  #include "rte_telemetry_internal.h"
>  
>  #define SLEEP_TIME 10
>  
> +#define DEFAULT_DPDK_PATH "/var/run/.rte_telemetry"
> +
> +const char *socket_path = DEFAULT_DPDK_PATH;
>  static telemetry_impl *static_telemetry;
>  
> +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.

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?

Also, this function does not seem used in this patch, can it be added
when used?

> +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.

> +		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.

> +		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.

[snip]

-- 
Gaëtan Rivet
6WIND