Re: [RFC PATCH v6 05/11] trace-cmd: Add VM kernel tracing over vsockets transport

[Slavomir Kaslev <kaslevs@vmware.com>]

On Thu, Feb 14, 2019 at 03:03:48PM -0500, Steven Rostedt wrote:
> On Thu, 14 Feb 2019 16:13:29 +0200
> Slavomir Kaslev <kaslevs@vmware.com> wrote:
> 
> >  void start_threads(enum trace_type type, struct common_record_context *ctx)
> >  {
> >  	struct buffer_instance *instance;
> > -	int *brass = NULL;
> >  	int total_cpu_count = 0;
> >  	int i = 0;
> >  	int ret;
> >  
> > -	for_all_instances(instance)
> > +	for_all_instances(instance) {
> > +		/* Start the connection now to find out how many CPUs we need */
> > +		if (instance->flags & BUFFER_FL_GUEST)
> > +			connect_to_agent(instance);
> >  		total_cpu_count += instance->cpu_count;
> > +	}
> >  
> >  	/* make a thread for every CPU we have */
> > -	pids = malloc(sizeof(*pids) * total_cpu_count * (buffers + 1));
> > +	pids = calloc(total_cpu_count * (buffers + 1), sizeof(*pids));
> >  	if (!pids)
> > -		die("Failed to allocat pids for %d cpus", total_cpu_count);
> > -
> > -	memset(pids, 0, sizeof(*pids) * total_cpu_count * (buffers + 1));
> > +		die("Failed to allocate pids for %d cpus", total_cpu_count);
> >  
> >  	for_all_instances(instance) {
> > +		int *brass = NULL;
> >  		int x, pid;
> >  
> > -		if (host) {
> > +		if (instance->flags & BUFFER_FL_AGENT) {
> > +			setup_agent(instance, ctx);
> > +		} else if (instance->flags & BUFFER_FL_GUEST) {
> > +			setup_guest(instance);
> > +		} else if (host) {
> >  			instance->msg_handle = setup_connection(instance, ctx);
> >  			if (!instance->msg_handle)
> >  				die("Failed to make connection");
> > @@ -3139,13 +3444,14 @@ static void print_stat(struct buffer_instance *instance)
> >  {
> >  	int cpu;
> >  
> > +	if (quiet)
> > +		return;
> 
> Hmm, this looks unrelated to this patch, and looks like it should have
> been in a patch by itself. As a clean up.
> 
> > +
> >  	if (!is_top_instance(instance))
> > -		if (!quiet)
> > -			printf("\nBuffer: %s\n\n", instance->name);
> > +		printf("\nBuffer: %s\n\n", instance->name);
> >  
> >  	for (cpu = 0; cpu < instance->cpu_count; cpu++)
> > -		if (!quiet)
> > -			trace_seq_do_printf(&instance->s_print[cpu]);
> > +		trace_seq_do_printf(&instance->s_print[cpu]);
> >  }
> >  
> >  enum {
> > @@ -3171,7 +3477,44 @@ static void add_options(struct tracecmd_output *handle, struct common_record_con
> >  	tracecmd_add_option(handle, TRACECMD_OPTION_TRACECLOCK, 0, NULL);
> >  	add_option_hooks(handle);
> >  	add_uname(handle);
> > +}
> > +
> > +static void write_guest_file(struct buffer_instance *instance)
> > +{
> > +	struct tracecmd_output *handle;
> > +	int cpu_count = instance->cpu_count;
> > +	char *file;
> > +	char **temp_files;
> > +	int i, fd;
> > +
> > +	file = get_guest_file(output_file, instance->name);
> > +	fd = open(file, O_RDWR);
> > +	if (fd < 0)
> > +		die("error opening %s", file);
> > +	put_temp_file(file);
> > +
> > +	handle = tracecmd_get_output_handle_fd(fd);
> > +	if (!handle)
> > +		die("error writing to %s", file);
> >  
> > +	temp_files = malloc(sizeof(*temp_files) * cpu_count);
> > +	if (!temp_files)
> > +		die("failed to allocate temp_files for %d cpus",
> > +		    cpu_count);
> > +
> > +	for (i = 0; i < cpu_count; i++) {
> > +		temp_files[i] = get_temp_file(instance, i);
> > +		if (!temp_files[i])
> > +			die("failed to allocate memory");
> > +	}
> > +
> > +	if (tracecmd_write_cpu_data(handle, cpu_count, temp_files) < 0)
> > +		die("failed to write CPU data");
> > +	tracecmd_output_close(handle);
> > +
> > +	for (i = 0; i < cpu_count; i++)
> > +		put_temp_file(temp_files[i]);
> > +	free(temp_files);
> >  }
> >  
> >  static void record_data(struct common_record_context *ctx)
> > @@ -3185,7 +3528,9 @@ static void record_data(struct common_record_context *ctx)
> >  	int i;
> >  
> >  	for_all_instances(instance) {
> > -		if (instance->msg_handle)
> > +		if (instance->flags & BUFFER_FL_GUEST)
> > +			write_guest_file(instance);
> > +		else if (host && instance->msg_handle)
> >  			finish_network(instance->msg_handle);
> >  		else
> >  			local = true;
> > @@ -4404,6 +4749,7 @@ void trace_stop(int argc, char **argv)
> >  		c = getopt(argc-1, argv+1, "hatB:");
> >  		if (c == -1)
> >  			break;
> > +
> >  		switch (c) {
> >  		case 'h':
> >  			usage(argv);
> > @@ -4566,6 +4912,63 @@ static void init_common_record_context(struct common_record_context *ctx,
> >  #define IS_STREAM(ctx) ((ctx)->curr_cmd == CMD_stream)
> >  #define IS_PROFILE(ctx) ((ctx)->curr_cmd == CMD_profile)
> >  #define IS_RECORD(ctx) ((ctx)->curr_cmd == CMD_record)
> > +#define IS_AGENT(ctx) ((ctx)->curr_cmd == CMD_record_agent)
> > +
> > +static void add_argv(struct buffer_instance *instance, char *arg, bool prepend)
> > +{
> > +	instance->argv = realloc(instance->argv,
> > +				 (instance->argc + 1) * sizeof(char *));
> > +	if (!instance->argv)
> > +		die("Can not allocate instance args");
> > +	if (prepend) {
> > +		memmove(instance->argv + 1, instance->argv,
> > +			instance->argc * sizeof(*instance->argv));
> > +		instance->argv[0] = arg;
> > +	} else {
> > +		instance->argv[instance->argc] = arg;
> > +	}
> > +	instance->argc++;
> > +}
> > +
> > +static void add_arg(struct buffer_instance *instance,
> > +		    int c, const char *opts,
> > +		    struct option *long_options, char *optarg)
> > +{
> > +	char *ptr;
> > +	char *arg;
> > +	int ret;
> > +	int i;
> > +
> > +	/* Short or long arg */
> > +	if (!(c & 0x80)) {
> > +		ret = asprintf(&arg, "-%c", c);
> > +		if (ret < 0)
> > +			die("Can not allocate argument");
> > +		ptr = strstr(opts, arg+1);
> > +		if (!ptr)
> > +			return; /* Not found? */
> 
> This leaks arg, as arg was allocated with asprintf(). Need a
> "free(arg)" here.

Bah! Another left over from the VIRT-SERVER branch. Reorganized this function
for the next iteration.