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

["Yordan Karadzhov (VMware)" <y.karadz@gmail.com>]

On 30.10.20 г. 3:57 ч., Steven Rostedt wrote:
> On Thu, 29 Oct 2020 16:49:03 +0200
> "Yordan Karadzhov (VMware)" <y.karadz@gmail.com> wrote:
> 
>> 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).
> 
> Maybe I got confused ;-)
> 
> Is there going to be different interface structures? Why the "void *"
> and not just supply a "struct kshark_data_stream *"?

Hi Steven,

Yes, my idea is that in the future we may decide to change something in 
the interface, or to have a second completely different interface, while 
we may still need to keep the interface version that we have now for 
backward compatibility.

> 
> That way at least you have some kind of type checking when tasks move
> things around. I try to avoid using "void *" because it can easily be
> the source of unwanted bugs, due to the lack of type checking.

What if we define the interface to start with an integer identifier?

/** Data interface identifier. */
typedef enum kshark_data_interface_id {
	/** An interface with unknown type. */
	KS_INVALIDE_INTERFACE,

	/** Generic interface suitable Ftrace data. */
	KS_GENERIC_DATA_INTERFACE,
} kshark_data_interface_id;

/**
  * Structure representing the interface of methods used to operate over
  * the data from a given stream.
  */
struct kshark_generic_stream_interface {
	/** Interface version identifier. */
	int			type; /* MUST BE FIRST ENTRY */

	/** Method used to retrieve the Process Id of the entry. */
	stream_get_int_func	get_pid;

	/** Method used to retrieve the Event Id of the entry. */
	stream_get_int_func	get_event_id;

....

and it can be used like this:

char *kshark_get_aux_field(const struct kshark_entry *entry)
{
	struct kshark_generic_stream_interface *interface;
	struct kshark_data_stream *stream =
		kshark_get_stream_from_entry(entry);

	....

	interface = stream->interface;
	if (interface->type == KS_GENERIC_DATA_INTERFACE &&
	    interface->aux_field)
		return interface->aux_field(stream, entry);

	return NULL;
}

What do you think?


Thanks a lot!
Yordan


> 
> -- Steve
>