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.
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
|