Re: [PATCH v26 10/15] trace-cmd: Add timestamp synchronization per vCPU

[Steven Rostedt <rostedt@goodmis.org>]

On Thu, 21 Jan 2021 09:44:51 +0200
"Tzvetomir Stoyanov (VMware)" <tz.stoyanov@gmail.com> wrote:

  
> -static void tsync_offset_load(struct tracecmd_input *handle, char *buf)
> +#define safe_read(R, C) \
> +	{ if ((C) > size) return -EFAULT; \
> +	 (R) = tep_read_number(tep, buf, (C)); \
> +	 buf += (C); \
> +	 size -= (C); }
> +static int tsync_offset_load(struct tep_handle	*tep,
> +			     struct timesync_offsets *ts_offsets, char *buf, int size)
>  {
> -	struct host_trace_info *host = &handle->host;
> -	long long *buf8 = (long long *)buf;
> +	int start_size = size;
>  	int i, j;
>  
> -	for (i = 0; i < host->ts_samples_count; i++) {
> -		host->ts_samples[i].time = tep_read_number(handle->pevent,
> -							   buf8 + i, 8);
> -		host->ts_samples[i].offset = tep_read_number(handle->pevent,
> -						buf8 + host->ts_samples_count + i, 8);
> -		host->ts_samples[i].scaling = tep_read_number(handle->pevent,
> -						buf8 + (2 * host->ts_samples_count) + i, 8);
> -	}
> -	qsort(host->ts_samples, host->ts_samples_count,
> +	for (i = 0; i < ts_offsets->ts_samples_count; i++)
> +		safe_read(ts_offsets->ts_samples[i].time, 8);
> +	for (i = 0; i < ts_offsets->ts_samples_count; i++)
> +		safe_read(ts_offsets->ts_samples[i].offset, 8);
> +	for (i = 0; i < ts_offsets->ts_samples_count; i++)
> +		safe_read(ts_offsets->ts_samples[i].scaling, 8);

I like to follow the Linux kernel requirements for macros. Which is any
complex macro like the "safe_read" above, needs to be done in a
do { } while (0) loop, as it is safer to use than just brackets. Also,
perhaps even making a macro out of the loop too.

#define safe_read(R, C) 					\
	do {							\
		if (size < (C))					\
			return -EFAULT;				\
		(R) = tep_read_number(tep, buf, (C));		\
		buf += (C);					\
		size -= (C);					\
	} while (0)

#define safe_read_loop(type) 						\
	do {								\
		int i;							\
		for (i = 0; i < ts_offsets->ts_samples_count; i++)	\
			safe_read(ts_offsets->ts_samples[i].type, 8);	\
	} while (0)

Then replace all the above with;

	safe_read_loop(time);
	safe_read_loop(offset);
	safe_read_loop(scaling);


I'll look at this more tomorrow.

-- Steve

> +	qsort(ts_offsets->ts_samples, ts_offsets->ts_samples_count,
>  	      sizeof(struct ts_offset_sample), tsync_offset_cmp);
>  	/* Filter possible samples with equal time */
> -	for (i = 0, j = 0; i < host->ts_samples_count; i++) {
> -		if (i == 0 || host->ts_samples[i].time != host->ts_samples[i-1].time)
> -			host->ts_samples[j++] = host->ts_samples[i];
> +	for (i = 0, j = 0; i < ts_offsets->ts_samples_count; i++) {
> +		if (i == 0 || ts_offsets->ts_samples[i].time != ts_offsets->ts_samples[i-1].time)
> +			ts_offsets->ts_samples[j++] = ts_offsets->ts_samples[i];
> +	}
> +	ts_offsets->ts_samples_count = j;
> +
> +	return start_size - size;
> +}
> +