Re: [PATCH v2 07/20] kernel-shark: Add basic methods for Data streams

[Steven Rostedt <rostedt@goodmis.org>]

On Thu, 29 Oct 2020 12:10:36 +0200
"Yordan Karadzhov (VMware)" <y.karadz@gmail.com> wrote:

> >> +/**
> >> + * @brief Get the name of the command/task from its Process Id.
> >> + *
> >> + * @param sd: Data stream identifier.
> >> + * @param pid: Process Id of the command/task.
> >> + */
> >> +char *kshark_comm_from_pid(int sd, int pid)  
> > 
> > I wonder if we should abstract this further, and call it
> > "kshark_name_from_id()", as comm and pid are specific to processes, and
> > we may have a stream that will represent something other than processes.
> >   
> 
> This is just a helper function that wraps the corresponding method of 
> the interface. In the future we can add a similar helper/wrapper 
> function with different name if we have streams that contain no processes.
> 
> However, you are actually making a very good point. It may be even 
> better if we abstract the interface itself, instead of trying to have 
> more abstract method names. We can keep the existing interface of 
> methods unchanged, but in the definition of "struct kshark_data_stream" 
> make the interface void*
> 
> struct kshark_data_stream {
> 	/** Data stream identifier. */
> 	uint8_t			stream_id;
> 
> ....
> 
> 	/** List of Plugin's Draw handlers. */
> 	struct kshark_draw_handler		*draw_handlers;
> 
> 	/**
> 	 * Abstract interface of methods used to operate over the data
> 	 * from a given stream. An implementation must be provided.
> 	 */
> 	void	*interface;
> };
> 
> and then the wrapping functions may look like this
> 
> char *kshark_comm_from_pid(int sd, int pid)
> {
> 	struct kshark_data_stream_interface *interface;
> 	struct kshark_context *kshark_ctx = NULL;
> 	struct kshark_data_stream *stream;
> 	struct kshark_entry e;
> 
> 	if (!kshark_instance(&kshark_ctx))
> 		return NULL;
> 
> 	stream = kshark_get_data_stream(kshark_ctx, sd);
> 	if (!stream)
> 		return NULL;
> 
> 	interface = stream->interface;
> 	if (!interface || !interface->get_task)
> 		return NULL;
> 
> 	e.visible = KS_PLUGIN_UNTOUCHED_MASK;
> 	e.pid = pid;
> 
> 	return interface->get_task(stream, &e);
> }
> 
> And in the future we can add more interface implementations and more 
> helper functions.
> 
> What do you think?


Do we need to make the interface "void *" and not just a non defined type
"struct kshark_data_stream_interface *"?

Just have:

struct kshark_data_stream_interface;

And then reference it without defining it, and have the streams define it.
If you need this to have inheritance and a bit of polymorphism you can do
that too :-) That is, if you want this interface to have something common
among all streams.

struct kshark_data_stream_interface {
	int		type;
	int		common_data;
};

then local to the file that implements it:

struct data_stream_interface {
	struct kshark_data_stream_interface	kinterface;
	int					unique_data;
};


{
	struct data_stream_interface	*interface;


	interface = (struct data_stream_interface *)stream->interface;

	if (interface->kinterface.type != my_type)
		return (or error);

	unique_data = interface->unique_data;

}


This is even how the trace events work in the kernel. For example:

struct trace_entry {
	unsigned short		type;
	unsigned char		flags;
	unsigned char		preempt_count;
	int			pid;
};

struct kprobe_trace_entry_head {
	struct trace_entry	ent;
	unsigned long		ip;
};

struct kretprobe_trace_entry_head {
	struct trace_entry	ent;
	unsigned long		func;
	unsigned long		ret_ip;
};

-- Steve