Re: [PATCH v25 01/16] trace-cmd: Replace time sync protocol ID with string

From: Steven Rostedt <rostedt@goodmis.org>
To: "Tzvetomir Stoyanov (VMware)" <tz.stoyanov@gmail.com>
Cc: linux-trace-devel@vger.kernel.org
Subject: Re: [PATCH v25 01/16] trace-cmd: Replace time sync protocol ID with string
Date: Thu, 5 Nov 2020 09:52:39 -0500	[thread overview]
Message-ID: <20201105095239.500efdf2@gandalf.local.home> (raw)
In-Reply-To: <20201029111816.247241-2-tz.stoyanov@gmail.com>

On Thu, 29 Oct 2020 13:18:01 +0200
"Tzvetomir Stoyanov (VMware)" <tz.stoyanov@gmail.com> wrote:

> Different timestamp synchronization algorithms will be implemented as
> trace-cmd plugins. Using IDs for identifying these algorithms is
> a problem, as these IDs must be defined in a single place. In order
> to be more flexible, protocol IDs are replaced by protocol names -
> strings that are part of the plugin code and are registered with
> plugin initialisation.
> The plugin names are limited up to 15 symbols, in order to simplify
> the structure of sync messages.
> 
> Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com>
> ---
>  include/trace-cmd/trace-cmd.h             |  32 +++---
>  lib/trace-cmd/include/trace-tsync-local.h |  12 +-
>  lib/trace-cmd/trace-msg.c                 | 102 ++++++++++------
>  lib/trace-cmd/trace-timesync.c            | 134 ++++++++++------------
>  tracecmd/include/trace-local.h            |   5 +-
>  tracecmd/trace-agent.c                    |   8 +-
>  tracecmd/trace-record.c                   |  19 +--
>  tracecmd/trace-tsync.c                    |  19 ++-
>  8 files changed, 170 insertions(+), 161 deletions(-)
> 
> diff --git a/include/trace-cmd/trace-cmd.h b/include/trace-cmd/trace-cmd.h
> index f3c95f30..8d3bea73 100644
> --- a/include/trace-cmd/trace-cmd.h
> +++ b/include/trace-cmd/trace-cmd.h
> @@ -385,50 +385,44 @@ void tracecmd_msg_set_done(struct tracecmd_msg_handle *msg_handle);
>  int tracecmd_msg_send_trace_req(struct tracecmd_msg_handle *msg_handle,
>  				int argc, char **argv, bool use_fifos,
>  				unsigned long long trace_id,
> -				char *tsync_protos,
> -				int tsync_protos_size);
> +				char **tsync_protos);

Probably should make these const char * const *
(I'll have to check if I place those correctly ;-)

As I believe this is passed in and not modified.

>  int tracecmd_msg_recv_trace_req(struct tracecmd_msg_handle *msg_handle,
>  				int *argc, char ***argv, bool *use_fifos,
>  				unsigned long long *trace_id,
> -				char **tsync_protos,
> -				unsigned int *tsync_protos_size);
> +				char ***tsync_protos);

Hmm, maybe we need to make this into its own structure that handles the
array, because char *** is starting to get too complex. Prehaps we should
just abstract it out!

struct tracecmd_tsync_protos;

int tracecmd_msg_send_trace_req(struct tracecmd_msg_handle *msg_handle,
				int argc, char **argv, bool use_fifos,
				unsigned long long trace_id,
				struct tracecmd_tsync_protos *tsync_protos);

int tracecmd_msg_recv_trace_req(struct tracecmd_msg_handle *msg_handle,
				int *argc, char ***argv, bool *use_fifos,
				unsigned long long *trace_id,
				struct tracecmd_tsync_protos **tsync_protos);

Then we have internally:

struct tracecmd_tsync_protos {
	char **protos;
}

That we play with, and have helper functions to extract the list for cases
that want to see all the protos. This would also give us flexibility to add
more complex operations to this later.

>  
>  int tracecmd_msg_send_trace_resp(struct tracecmd_msg_handle *msg_handle,
>  				 int nr_cpus, int page_size,
>  				 unsigned int *ports, bool use_fifos,
>  				 unsigned long long trace_id,
> -				 unsigned int tsync_proto,
> -				 unsigned int tsync_port);
> +				 char *tsync_proto, unsigned int tsync_port);
>  int tracecmd_msg_recv_trace_resp(struct tracecmd_msg_handle *msg_handle,
>  				 int *nr_cpus, int *page_size,
>  				 unsigned int **ports, bool *use_fifos,
>  				 unsigned long long *trace_id,
> -				 unsigned int *tsync_proto,
> +				 char **tsync_proto,
>  				 unsigned int *tsync_port);
>  
>  int tracecmd_msg_send_time_sync(struct tracecmd_msg_handle *msg_handle,
> -				unsigned int sync_protocol,
> -				unsigned int sync_msg_id,
> +				char *sync_protocol, unsigned int sync_msg_id,
>  				unsigned int payload_size, char *payload);
>  int tracecmd_msg_recv_time_sync(struct tracecmd_msg_handle *msg_handle,
> -				unsigned int *sync_protocol,
> +				char *sync_protocol,
>  				unsigned int *sync_msg_id,
>  				unsigned int *payload_size, char **payload);
>  
>  /* --- Timestamp synchronization --- */
>  
> -enum{
> -	TRACECMD_TIME_SYNC_PROTO_NONE	= 0,
> -};
> +#define TRACECMD_TSYNC_PNAME_LENGTH	16
> +#define TRACECMD_TSYNC_PROTO_NONE	"none"
> +
>  enum{
>  	TRACECMD_TIME_SYNC_CMD_PROBE	= 1,
>  	TRACECMD_TIME_SYNC_CMD_STOP	= 2,
>  };
>  
> -#define TRACECMD_TIME_SYNC_PROTO_PTP_WEIGHT	10
> -
>  struct tracecmd_time_sync {
> -	unsigned int			sync_proto;
> +	char				*proto_name;

	cost char			*proto_name;

>  	int				loop_interval;
>  	pthread_mutex_t			lock;
>  	pthread_cond_t			cond;
> @@ -438,9 +432,9 @@ struct tracecmd_time_sync {
>  };
>  
>  void tracecmd_tsync_init(void);
> -int tracecmd_tsync_proto_getall(char **proto_mask, int *words);
> -unsigned int tracecmd_tsync_proto_select(char *proto_mask, int words);
> -bool tsync_proto_is_supported(unsigned int proto_id);
> +int tracecmd_tsync_proto_getall(char ***protos);
> +char *tracecmd_tsync_proto_select(char **protos);

const char *

> +bool tsync_proto_is_supported(char *proto_name);
>  void tracecmd_tsync_with_host(struct tracecmd_time_sync *tsync);
>  void tracecmd_tsync_with_guest(struct tracecmd_time_sync *tsync);
>  int tracecmd_tsync_get_offsets(struct tracecmd_time_sync *tsync,
> diff --git a/lib/trace-cmd/include/trace-tsync-local.h b/lib/trace-cmd/include/trace-tsync-local.h
> index 1de9d5e5..1f3bc443 100644
> --- a/lib/trace-cmd/include/trace-tsync-local.h
> +++ b/lib/trace-cmd/include/trace-tsync-local.h
> @@ -26,12 +26,12 @@ struct clock_sync_context {
>  	unsigned int			remote_port;
>  };
>  
> -static int get_trace_req_protos(char *buf, int length,
> -				char **tsync_protos,
> -				unsigned int *tsync_protos_size)
> +static int get_trace_req_protos(char *buf, int length, char ***tsync_protos)
>  {
> -	*tsync_protos = malloc(length);
> -	if (!*tsync_protos)
> +	int count = 0;
> +	char *p;
> +	int i, j;
> +
> +	i = length;
> +	p = buf;
> +	while (i > 0) {
> +		i -= strlen(p) + 1;
> +		count++;
> +		p -= strlen(p) + 1;

Do you really want p to go backwards here?

> +	}
> +
> +	*tsync_protos = calloc(count + 1, sizeof(char *));
> +	if (!(*tsync_protos))
>  		return -1;
> -	memcpy(*tsync_protos, buf, length);
> -	*tsync_protos_size = length;
> +
> +	i = length;
> +	p = buf;
> +	j = 0;
> +	while (i > 0 && j < (count - 1)) {
> +		i -= strlen(p) + 1;
> +		(*tsync_protos)[j++] = strdup(p);
> +		p -= strlen(p) + 1;

And here too?

-- Steve

> +	}
>  
>  	return 0;
>  }
> @@ -1026,8 +1054,7 @@ out:
>  int tracecmd_msg_recv_trace_req(struct tracecmd_msg_handle *msg_handle,
>  				int *argc, char ***argv, bool *use_fifos,
>  				unsigned long long *trace_id,
> -				char **tsync_protos,
> -				unsigned int *tsync_protos_size)
> +				char ***tsync_protos)
>  {
>  	struct tracecmd_msg msg;
>  	unsigned int param_id;