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

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

On 12.11.20 г. 23:17 ч., Steven Rostedt wrote:
> On Thu, 12 Nov 2020 16:23:44 +0200
> "Yordan Karadzhov (VMware)" <y.karadz@gmail.com> wrote:
> 
>> +/**
>> + * @brief Add new Data stream.
>> + *
>> + * @param kshark_ctx: Input location for context pointer.
>> + *
>> + * @returns Zero on success or a negative errno code on failure.
>> + */


Yes, you are right. It must look like this


#define KS_MAX_STREAM_ID	INT16_MAX

>> +int kshark_add_stream(struct kshark_context *kshark_ctx)
>> +{
>> +	struct kshark_data_stream *stream;
>> +	int new_stream;
>> +

	if(kshark_ctx->stream_info.next_free_stream_id > KS_MAX_STREAM_ID)
		return ENODEV;

>> +	if (kshark_ctx->stream_info.next_free_stream_id ==
>> +	    kshark_ctx->stream_info.array_size) {
>> +		printf("realloc %i\n", 2 * kshark_ctx->stream_info.array_size);
> 
>   Some left over debugging? ^^^
> 
> Also, you probably want a check to make sure that you don't get bigger than
> KS_DEFAULT_NUM_STRINGS, otherwise the stream id wont fit into event id.
> 
> Oh, and KS_DEFAULT_NUM_STRINGS probably needs to be increased to the new
> max stream id.
> 
> 
>> +		struct kshark_data_stream **streams_tmp;
>> +
>> +		streams_tmp = realloc(kshark_ctx->stream,
>> +				      2 * kshark_ctx->stream_info.array_size * sizeof(*kshark_ctx->stream));
>> +		if (!streams_tmp)
>> +			return -ENOMEM;
>> +
>> +		kshark_ctx->stream = streams_tmp;
>> +		kshark_ctx->stream_info.array_size *= 2;
>> +	}
>> +
>> +	stream = kshark_stream_alloc();
>> +	if (!stream)
>> +		return -ENOMEM;
>> +
>> +	if (pthread_mutex_init(&stream->input_mutex, NULL) != 0) {
>> +		kshark_stream_free(stream);
>> +		return -EAGAIN;
>> +	}
>> +
>> +	if (kshark_ctx->stream_info.next_free_stream_id >
>> +	    kshark_ctx->stream_info.max_stream_id) {
>> +		new_stream = ++kshark_ctx->stream_info.max_stream_id;
>> +
>> +		kshark_ctx->stream_info.next_free_stream_id = new_stream + 1;
>> +
>> +		kshark_ctx->stream[new_stream] = stream;
>> +		stream->stream_id = new_stream;
>> +	} else {
>> +		new_stream = kshark_ctx->stream_info.next_free_stream_id;
>> +
>> +		kshark_ctx->stream_info.next_free_stream_id =
>> +			index_from_ptr(kshark_ctx->stream[new_stream]);
>> +
>> +		kshark_ctx->stream[new_stream] = stream;
>> +		stream->stream_id = new_stream;
>> +	}
>> +
>> +	kshark_ctx->n_streams++;
>> +
>> +	return stream->stream_id;
>> +}
>> +
>> +/**
>> + * @brief Remove Data stream.
>> + *
>> + * @param kshark_ctx: Input location for context pointer.
>> + * @param sd: Data stream identifier.
>> + *
>> + * @returns Zero on success or a negative errno code on failure.
>> + */
>> +int kshark_remove_stream(struct kshark_context *kshark_ctx, int sd)
>> +{
>> +	struct kshark_data_stream *stream;
>> +
>> +	stream = kshark_get_data_stream(kshark_ctx, sd);
>> +	if (!stream)
>> +		return -EFAULT;
>> +
>> +	kshark_stream_free(stream);
>> +	kshark_ctx->stream[sd] =
>> +		index_to_ptr(kshark_ctx->stream_info.next_free_stream_id);
>> +	kshark_ctx->stream_info.next_free_stream_id = sd;
>> +	kshark_ctx->n_streams--;
>> +
>> +	return 0;
>> +}
>> +
>> +static bool kshark_is_valid_stream(void *ptr)
>> +{
>> +	unsigned long p = (unsigned long) ptr;
>> +	bool v = !((p & ~INDEX_MASK) == INVALID_STREAM_MASK);
>> +
>> +	return p && v;
>> +}
>> +
>> +/**
>> + * @brief Get the Data stream object having given Id.
>> + *
>> + * @param kshark_ctx: Input location for context pointer.
>> + * @param sd: Data stream identifier.
>> + *
>> + * @returns Pointer to a Data stream object if the sream exists. Otherwise
>> + *	    NULL.
>> + */
>> +struct kshark_data_stream *
>> +kshark_get_data_stream(struct kshark_context *kshark_ctx, int sd)
>> +{
>> +	if (sd >= 0 && sd < KS_DEFAULT_NUM_STREAMS)
> 
> The above should probably be:
> 	if (sd >= 0 && sd <= kshark_ctx->stream_info.max_stream_id)
> 

Correct. In fact KS_DEFAULT_NUM_STREAMS sould not be defined in this 
patch but in [10/20].

> Because the array is not of KS_DEFAULT_NUM_STREAMS in size, if sd is
> greater than max_stream_id and less then KS_DEFAULT_NUM_STREAMS this code
> will access non allocated memory.
> 
>> +		if (kshark_ctx->stream[sd] &&
>> +		    kshark_is_valid_stream(kshark_ctx->stream[sd]))
>> +			return kshark_ctx->stream[sd];
>> +
>> +	return NULL;
>> +}
>> +
>> +/**
>> + * @brief Get the Data stream object corresponding to a given entry
>> + *
>> + * @param entry: Input location for the KernelShark entry.
>> + *
>> + * @returns Pointer to a Data stream object on success. Otherwise NULL.
>> + */
>> +struct kshark_data_stream *
>> +kshark_get_stream_from_entry(const struct kshark_entry *entry)
>> +{
>> +	struct kshark_context *kshark_ctx = NULL;
>> +
>> +	if (!kshark_instance(&kshark_ctx))
>> +		return NULL;
>> +
>> +	return kshark_get_data_stream(kshark_ctx, entry->stream_id);
>> +}
>> +
>> +/**
>> + * @brief Get an array containing the Ids of all opened Trace data streams.
>> + * 	  The User is responsible for freeing the array.
>> + *
>> + * @param kshark_ctx: Input location for context pointer.
>> + */
>> +int *kshark_all_streams(struct kshark_context *kshark_ctx)
>> +{
>> +	int *ids, i, count = 0;
>> +
>> +	ids = calloc(kshark_ctx->n_streams, (sizeof(*ids)));
>> +	if (!ids)
>> +		return NULL;
>> +
>> +	for (i = 0; i <= kshark_ctx->stream_info.max_stream_id; ++i)
>> +		if (kshark_ctx->stream[i] &&
>> +		    kshark_is_valid_stream(kshark_ctx->stream[i]))
>> +			ids[count++] = i;
>> +
>> +	return ids;
>> +}
>>   /**
>>    * @brief Close the trace data file and free the trace data handle.
>>    *
>> @@ -252,6 +484,474 @@ void kshark_free(struct kshark_context *kshark_ctx)
>>   	free(kshark_ctx);
>>   }
>>   
>> +/* Quiet warnings over documenting simple defs. */
>> +//! @cond Doxygen_Suppress
>> +
>> +#define BROKEN_STREAM(stream) (!stream || !stream->interface)
>> +
>> +//! @endcond
>> +
>> +/**
>> + * @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)
>> +{
>> +	struct kshark_generic_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 (BROKEN_STREAM(stream))
>> +		return NULL;
> 
> Is kshark_get_data_stream() ever used without stream->interface? At least
> in this patch, all callers of kshark_get_data_stream() or
> kshark_get_stream_from_entry() (which the last thing it does is call
> kshark_get_data_stream()), is followed by this BROKEN_STREAM() check.
> 
> Would it make more sense to put in kshark_get_data_stream() the check for
> stream->interface, and return NULL if that's not set? Then we can remove
> all of the BROKEN_STREAM() callers and just have:
> 
> 	if (!stream)
> 		return NULL;
> 
Yes, it is used a lot in the GUI, but you are right. There is no point 
of having kshark_get_data_stream() return broken stream objects.
Will be changed in v4.

> 
>> +
>> +	interface = stream->interface;
>> +	if (interface->type == KS_GENERIC_DATA_INTERFACE &&
>> +	    interface->get_task) {
>> +		e.visible = KS_PLUGIN_UNTOUCHED_MASK;
>> +		e.pid = pid;
>> +
>> +		return interface->get_task(stream, &e);
>> +	}
>> +
>> +	return NULL;
>> +}
>> +
>> +/**
>> + * @brief Get the name of the event from its Id.
>> + *
>> + * @param sd: Data stream identifier.
>> + * @param event_id: The unique Id of the event type.
>> + */
>> +char *kshark_event_from_id(int sd, int event_id)
>> +{
>> +	struct kshark_generic_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 (BROKEN_STREAM(stream))
>> +		return NULL;
>> +
>> +	interface = stream->interface;
>> +	if (interface->type == KS_GENERIC_DATA_INTERFACE &&
>> +	    interface->get_event_name) {
>> +		e.visible = KS_PLUGIN_UNTOUCHED_MASK;
>> +		e.event_id = event_id;
>> +
>> +		return interface->get_event_name(stream, &e);
>> +	}
>> +
>> +	return NULL;
>> +}
>> +
>> +/**
>> + * @brief Get the original process Id of the entry. Using this function make
>> + *	  sense only in cases when the original value can be overwritten by
>> + *	  plugins. If you know that no plugins are loaded use "entry->pid"
>> + *	  directly.
>> + *
>> + * @param entry: Input location for an entry.
>> + */
>> +int kshark_get_pid(const struct kshark_entry *entry)
>> +{
>> +	struct kshark_generic_stream_interface *interface;
>> +	struct kshark_data_stream *stream =
>> +		kshark_get_stream_from_entry(entry);
>> +
>> +	if (BROKEN_STREAM(stream))
>> +		return -EFAULT;
>> +
>> +	interface = stream->interface;
>> +	if (interface->type == KS_GENERIC_DATA_INTERFACE &&
>> +	    interface->get_pid)
>> +		return -EFAULT;
>> +
>> +	return interface->get_pid(stream, entry);
>> +}
>> +
>> +/**
>> + * @brief Get the original event Id of the entry. Using this function make
>> + *	  sense only in cases when the original value can be overwritten by
>> + *	  plugins. If you know that no plugins are loaded use "entry->event_id"
>> + *	  directly.
>> + *
>> + * @param entry: Input location for an entry.
>> + */
>> +int kshark_get_event_id(const struct kshark_entry *entry)
>> +{
>> +	struct kshark_generic_stream_interface *interface;
>> +	struct kshark_data_stream *stream =
>> +		kshark_get_stream_from_entry(entry);
>> +
>> +	if (BROKEN_STREAM(stream))
>> +		return -EFAULT;
>> +
>> +	interface = stream->interface;
>> +	if (interface->type == KS_GENERIC_DATA_INTERFACE &&
>> +	    interface->get_event_id)
>> +		return interface->get_event_id(stream, entry);
>> +
>> +	return -EFAULT;
>> +}
>> +
>> +/**
>> + * @brief Get an array of all event Ids for a given data stream.
>> + *
>> + * @param stream: Input location for a Trace data stream pointer.
>> + *
>> + * @returns An array of event Ids. The user is responsible for freeing the
>> + *	    outputted array.
>> + */
>> +int *kshark_get_all_event_ids(struct kshark_data_stream *stream)
>> +{
>> +	struct kshark_generic_stream_interface *interface;
>> +
>> +	if (BROKEN_STREAM(stream))
>> +		return NULL;
>> +
>> +	interface = stream->interface;
>> +	if (interface->type == KS_GENERIC_DATA_INTERFACE &&
>> +	    interface->get_all_event_ids)
>> +		return interface->get_all_event_ids(stream);
>> +
>> +	return NULL;
>> +}
>> +
>> +/**
>> + * @brief Find the event Ids corresponding to a given event name.
>> + *
>> + * @param stream: Input location for a Trace data stream pointer.
>> + * @param event_name: The name of the event.
>> + *
>> + * @returns Event Ids number.
>> + */
>> +int kshark_find_event_id(struct kshark_data_stream *stream,
>> +			 const char *event_name)
>> +{
>> +	struct kshark_generic_stream_interface *interface;
>> +
>> +	if (BROKEN_STREAM(stream))
>> +		return -EFAULT;
>> +
>> +	interface = stream->interface;
>> +	if (interface->type == KS_GENERIC_DATA_INTERFACE &&
>> +	    interface->find_event_id)
>> +		return interface->find_event_id(stream, event_name);
>> +
>> +	return -EFAULT;
>> +}
>> +
>> +/**
>> + * @brief Find the event name corresponding to a given entry.
>> + *
>> + * @param entry: Input location for an entry.
>> + *
>> + * @returns The mane of the event on success, or NULL in case of failure.
>> + *	    The use is responsible for freeing the output string.
>> + */
>> +char *kshark_get_event_name(const struct kshark_entry *entry)
>> +{
>> +	struct kshark_generic_stream_interface *interface;
>> +	struct kshark_data_stream *stream =
>> +		kshark_get_stream_from_entry(entry);
>> +
>> +	if (BROKEN_STREAM(stream))
>> +		return NULL;
>> +
>> +	interface = stream->interface;
>> +	if (interface->type == KS_GENERIC_DATA_INTERFACE &&
>> +	    interface->get_event_name)
>> +		return interface->get_event_name(stream, entry);
>> +
>> +	return NULL;
>> +}
>> +
>> +/**
>> + * @brief Find the task name corresponding to a given entry.
>> + *
>> + * @param entry: Input location for an entry.
>> + *
>> + * @returns The mane of the task on success, or NULL in case of failure.
>> + *	    The use is responsible for freeing the output string.
>> + */
>> +char *kshark_get_task(const struct kshark_entry *entry)
>> +{
>> +	struct kshark_generic_stream_interface *interface;
>> +	struct kshark_data_stream *stream =
>> +		kshark_get_stream_from_entry(entry);
>> +
>> +	if (BROKEN_STREAM(stream))
>> +		return NULL;
>> +
>> +	interface = stream->interface;
>> +	if (interface->type == KS_GENERIC_DATA_INTERFACE &&
>> +	    interface->get_task)
>> +		return interface->get_task(stream, entry);
>> +
>> +	return NULL;
>> +}
>> +
>> +/**
>> + * @brief Get the basic information (text) about the entry.
>> + *
>> + * @param entry: Input location for an entry.
>> + *
>> + * @returns A the info text. The user is responsible for freeing the
>> + *	    outputted string.
>> + */
>> +char *kshark_get_info(const struct kshark_entry *entry)
>> +{
>> +	struct kshark_generic_stream_interface *interface;
>> +	struct kshark_data_stream *stream =
>> +		kshark_get_stream_from_entry(entry);
>> +
>> +	if (BROKEN_STREAM(stream))
>> +		return NULL;
>> +
>> +	interface = stream->interface;
>> +	if (interface->type == KS_GENERIC_DATA_INTERFACE &&
>> +	    interface->get_info)
>> +		return interface->get_info(stream, entry);
>> +
>> +	return NULL;
>> +}
>> +
>> +/**
>> + * @brief Get the auxiliary information about the entry. In the case of
>> + * 	  TEP (Ftrace) data, this function provides the latency info.
>> + *
>> + * @param entry: Input location for an entry.
>> + *
>> + * @returns A the auxiliary text info. The user is responsible for freeing the
>> + *	    outputted string.
>> + */
>> +char *kshark_get_aux_info(const struct kshark_entry *entry)
>> +{
>> +	struct kshark_generic_stream_interface *interface;
>> +	struct kshark_data_stream *stream =
>> +		kshark_get_stream_from_entry(entry);
>> +
>> +	if (BROKEN_STREAM(stream))
>> +		return NULL;
>> +
>> +	interface = stream->interface;
>> +	if (interface->type == KS_GENERIC_DATA_INTERFACE &&
>> +	    interface->aux_info)
>> +		return interface->aux_info(stream, entry);
>> +
>> +	return NULL;
>> +}
>> +
>> +/**
>> + * @brief Get an array of all data field names associated with a given entry.
>> + *
>> + * @param entry: Input location for an entry.
>> + * @param fields: Output location of the array of field names. The user is
>> + *		  responsible for freeing the elements of the outputted array.
>> + *
>> + * @returns Total number of event fields on success, or a negative errno in
>> + *	    the case of a failure.
>> + */
>> +int kshark_get_all_event_field_names(const struct kshark_entry *entry,
>> +				     char ***fields)
>> +{
>> +	struct kshark_generic_stream_interface *interface;
>> +	struct kshark_data_stream *stream =
>> +		kshark_get_stream_from_entry(entry);
>> +
>> +	if (BROKEN_STREAM(stream))
>> +		return -EFAULT;
>> +
>> +	interface = stream->interface;
>> +	if (interface->type == KS_GENERIC_DATA_INTERFACE &&
>> +	    interface->get_all_event_field_names)
>> +		return interface->get_all_event_field_names(stream,
>> +							    entry, fields);
>> +
>> +	return -EFAULT;
>> +}
>> +
>> +/**
>> + * @brief Get the value type of an event field corresponding to a given entry.
>> + *
>> + * @param entry: Input location for an entry.
>> + * @param field: The name of the data field.
>> + *
>> + * @returns The type of the data field on success, or KS_INVALIDE_FIELD in
>> + *	    the case of a failure.
>> + */
>> +kshark_event_field_format
>> +kshark_get_event_field_type(const struct kshark_entry *entry,
>> +			    const char *field)
>> +{
>> +	struct kshark_generic_stream_interface *interface;
>> +	struct kshark_data_stream *stream =
>> +		kshark_get_stream_from_entry(entry);
>> +
>> +	if (BROKEN_STREAM(stream))
>> +		return KS_INVALIDE_FIELD;
>> +
>> +	interface = stream->interface;
>> +	if (interface->type == KS_GENERIC_DATA_INTERFACE &&
>> +	    interface->get_event_field_type)
>> +		return interface->get_event_field_type(stream, entry, field);
>> +
>> +	return KS_INVALIDE_FIELD;
>> +}
>> +
>> +/**
>> + * @brief Get the value of an event field in a given trace record.
>> + *
>> + * @param stream: Input location for a Trace data stream pointer.
>> + * @param rec: Input location for a record.
>> + * @param field: The name of the data field.
>> + * @param val: Output location for the value of the field.
>> + *
>> + * @returns Zero on success or a negative errno in the case of a failure.
>> + */
>> +int kshark_read_record_field_int(struct kshark_data_stream *stream, void *rec,
>> +				 const char *field, int64_t *val)
>> +{
>> +	struct kshark_generic_stream_interface *interface;
>> +
>> +	if (BROKEN_STREAM(stream))
>> +		return -EFAULT;
>> +
>> +	interface = stream->interface;
>> +	if (interface->type == KS_GENERIC_DATA_INTERFACE &&
>> +	    interface->read_record_field_int64)
>> +		return interface->read_record_field_int64(stream, rec,
>> +							  field, val);
>> +
>> +	return -EFAULT;
>> +}
>> +
>> +/**
>> + * @brief Get the value of an event field corresponding to a given entry.
>> + *	  The value is retrieved via the offset in the file of the original
>> + *	  record.
>> + *
>> + * @param entry: Input location for an entry.
>> + * @param field: The name of the data field.
>> + * @param val: Output location for the value of the field.
>> + *
>> + * @returns Zero on success or a negative errno in the case of a failure.
>> + */
>> +int kshark_read_event_field_int(const struct kshark_entry *entry,
>> +				const char* field, int64_t *val)
>> +{
>> +	struct kshark_generic_stream_interface *interface;
>> +	struct kshark_data_stream *stream =
>> +		kshark_get_stream_from_entry(entry);
>> +
>> +	if (BROKEN_STREAM(stream))
>> +		return -EFAULT;
>> +
>> +	interface = stream->interface;
>> +	if (interface->type == KS_GENERIC_DATA_INTERFACE &&
>> +	    interface->read_event_field_int64)
>> +		return interface->read_event_field_int64(stream, entry, field, val);
>> +
>> +	return -EFAULT;
>> +}
>> +
>> +/** @brief Print the entry. */
>> +void kshark_print_entry(const struct kshark_entry *entry)
>> +{
>> +	char *entry_str = kshark_dump_entry(entry);
> 
> Should there be a "if (!entry_str) return;" check here?
> 

Thanks a lot!
Y.

> -- Steve
> 
>> +
>> +	puts(entry_str);
>> +	free(entry_str);
>> +}
>> +
>> +/**
>> + * @brief Load the content of the trace data file asociated with a given
>> + *	  Data stream into an array of kshark_entries.
>> + *	  If one or more filters are set, the "visible" fields of each entry
>> + *	  is updated according to the criteria provided by the filters. The
>> + *	  field "filter_mask" of the session's context is used to control the
>> + *	  level of visibility/invisibility of the filtered entries.
>> + *
>> + * @param kshark_ctx: Input location for context pointer.
>> + * @param sd: Data stream identifier.
>> + * @param data_rows: Output location for the trace data. The user is
>> + *		     responsible for freeing the elements of the outputted
>> + *		     array.
>> + *
>> + * @returns The size of the outputted data in the case of success, or a
>> + *	    negative error code on failure.
>> + */