On 29.10.20 г. 16:04 ч., Steven Rostedt wrote:
On Thu, 29 Oct 2020 12:10:36 +0200 "Yordan Karadzhov (VMware)" <y.karadz@xxxxxxxxx> 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.
Maybe I don't understand your idea very well, but I think what you suggest has very different behavior. What I want is the implementation of the interface to stay in the same header file (libkshark.h). In the future we can add more interfaces but this will be again in the same header (libkshark.h).
The readout plugins will include libkshark.h and will have to chose one of the available interfaces and implement its methods like this:
static int kshark_tep_stream_init(struct kshark_data_stream *stream,
struct tracecmd_input *input)
{
struct kshark_data_stream_interface *interface;
stream->interface = interface = calloc(1, sizeof(*interface));
if (!interface)
return -ENOMEM;
....
interface->get_pid = tepdata_get_pid;
interface->get_task = tepdata_get_task;
interface->get_event_id = tepdata_get_event_id;
....
Note that the plugins will include libkshark.h but libkshark will never
include headers from plugins.
Does it make any sense, or maybe I just don't understand your suggestion? Thanks a lot! Yordan
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
|