Re: [PATCH v4 1/7] kernel-shark-qt: Add plugin infrastructure to be used by the Qt-baset KS.

[Steven Rostedt <rostedt@goodmis.org>]

On Wed, 19 Sep 2018 17:36:51 +0300
"Yordan Karadzhov (VMware)" <y.karadz@gmail.com> wrote:

> diff --git a/kernel-shark-qt/src/libkshark-plugin.c b/kernel-shark-qt/src/libkshark-plugin.c
> new file mode 100644
> index 0000000..8feb00a
> --- /dev/null
> +++ b/kernel-shark-qt/src/libkshark-plugin.c
> @@ -0,0 +1,292 @@
> +// SPDX-License-Identifier: LGPL-2.1
> +
> +/*
> + * Copyright (C) 2017 VMware Inc, Yordan Karadzhov <y.karadz@gmail.com>
> + */
> +
> + /**
> +  *  @file    libkshark-plugin.c
> +  *  @brief   KernelShark plugins.
> +  */
> +
> +// C
> +#define _GNU_SOURCE
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <sys/stat.h>
> +#include <dlfcn.h>
> +#include <errno.h>
> +
> +// KernelShark
> +#include "libkshark-plugin.h"
> +#include "libkshark.h"
> +
> +static struct kshark_event_handler *
> +gui_event_handler_alloc(int event_id,
> +			kshark_plugin_event_handler_func evt_func,
> +			kshark_plugin_draw_handler_func dw_func)
> +{
> +	struct kshark_event_handler *handler = malloc(sizeof(*handler));
> +
> +	if (!handler) {
> +		fprintf(stderr,
> +			"failed to allocate memory for gui eventhandler");
> +		return NULL;
> +	}
> +
> +	handler->next = NULL;
> +	handler->id = event_id;
> +	handler->event_func = evt_func;
> +	handler->draw_func = dw_func;
> +
> +	return handler;
> +}
> +
> +/**
> + * @brief Search the list of event handlers for a handle associated with a
> + *	  given event type.
> + *
> + * @param handlers: Input location for the Event handler list.
> + * @param event_id: Event Id to search for.
> + */
> +struct kshark_event_handler *
> +find_event_handler(struct kshark_event_handler *handlers, int event_id)

This is not a static function. Shouldn't it have a "kshark_" prefix?

> +{
> +	for (; handlers; handlers = handlers->next)
> +		if (handlers->id == event_id)
> +			return handlers;
> +
> +	return NULL;
> +}
> +
> +/**
> + * @brief Add new event handler to an existing list of handlers.
> + *
> + * @param handlers: Input location for the Event handler list.
> + * @param event_id: Event Id.
> + * @param evt_func: Input location for an Event action provided by the plugin.
> + * @param dw_func: Input location for a Draw action provided by the plugin.
> + *
> + * @returns Zero on success, or a negative error code on failure.
> + */
> +int kshark_register_event_handler(struct kshark_event_handler **handlers,
> +				  int event_id,
> +				  kshark_plugin_event_handler_func evt_func,
> +				  kshark_plugin_draw_handler_func dw_func)
> +{
> +	struct kshark_event_handler *handler =
> +		gui_event_handler_alloc(event_id, evt_func, dw_func);
> +
> +	if(!handler)
> +		return -ENOMEM;
> +
> +	handler->next = *handlers;
> +	*handlers = handler;
> +	return 0;
> +}
> +
> +/**
> + * @brief Search the list for a specific plugin handle. If such a plugin handle
> + *	  exists, unregister (remove and free) this handle from the list.
> + *
> + * @param handlers: Input location for the Event handler list.
> + * @param event_id: Event Id of the plugin handler to be unregistered.
> + * @param evt_func: Event action function of the handler to be unregistered.
> + * @param dw_func: Draw action function of the handler to be unregistered.
> + */
> +void kshark_unregister_event_handler(struct kshark_event_handler **handlers,
> +				     int event_id,
> +				     kshark_plugin_event_handler_func evt_func,
> +				     kshark_plugin_draw_handler_func dw_func)
> +{
> +	struct kshark_event_handler **last;
> +
> +	for (last = handlers; *last; last = &(*last)->next) {
> +		if ((*last)->id == event_id &&
> +		    (*last)->event_func == evt_func &&
> +		    (*last)->draw_func == dw_func) {
> +			struct kshark_event_handler *this_handler;
> +			this_handler = *last;
> +			*last = this_handler->next;
> +			free(this_handler);
> +
> +			return;
> +		}
> +	}
> +}
> +
> +/**
> + * @brief Free all Event handlers in a given list.
> + *
> + * @param handlers: Input location for the Event handler list.
> + */
> +void kshark_free_event_handler_list(struct kshark_event_handler *handlers)
> +{
> +	struct kshark_event_handler *last;
> +
> +	while (handlers) {
> +		last = handlers;
> +		handlers = handlers->next;
> +		free(last);
> +	}
> +}
> +
> +/**
> + * @brief Allocate memory for a new plugin. Add this plugin to the list of
> + *	  plugins used by the session.
> + *
> + * @param kshark_ctx: Input location for the session context pointer.
> + * @param file: The plugin object file to load.
> + *
> + * @returns Zero on success, or a negative error code on failure.
> + */
> +int kshark_register_plugin(struct kshark_context *kshark_ctx,
> +			   const char *file)
> +{
> +	struct kshark_plugin_list *plugin = kshark_ctx->plugins;
> +	struct stat st;
> +	int ret;
> +
> +	while (plugin) {
> +		if (strcmp(plugin->file, file) == 0)
> +			return -EEXIST;
> +
> +		plugin = plugin->next;
> +	}
> +
> +	ret = stat(file, &st);
> +	if (ret < 0) {
> +		fprintf(stderr, "plugin %s not found\n", file);
> +		return -ENODEV;
> +	}
> +
> +	plugin = calloc(sizeof(struct kshark_plugin_list), 1);
> +	if (!plugin) {
> +		fprintf(stderr, "failed to allocate memory for plugin\n");
> +		return -ENOMEM;
> +	}
> +
> +	if (asprintf(&plugin->file, "%s", file) <= 0) {
> +		fprintf(stderr,
> +			"failed to allocate memory for plugin file name");
> +		return -ENOMEM;
> +	}
> +
> +	plugin->handle = dlopen(plugin->file, RTLD_NOW | RTLD_GLOBAL);
> +	if (!plugin->handle)
> +		goto fail;
> +
> +	plugin->init = dlsym(plugin->handle,
> +			     KSHARK_PLUGIN_INITIALIZER_NAME);
> +
> +	plugin->close = dlsym(plugin->handle,
> +			      KSHARK_PLUGIN_DEINITIALIZER_NAME);
> +
> +	if (!plugin->init || !plugin->close)
> +		goto fail;
> +
> +	plugin->next = kshark_ctx->plugins;
> +	kshark_ctx->plugins = plugin;
> +
> +	return 0;
> +
> + fail:
> +	fprintf(stderr, "cannot load plugin '%s'\n%s\n",
> +		plugin->file, dlerror());
> +
> +	if (plugin->handle) {
> +		dlclose(plugin->handle);
> +		plugin->handle = NULL;
> +	}
> +
> +	free(plugin);
> +
> +	return EFAULT;
> +}
> +
> +/**
> + * @brief Unrgister a plugin.
> + *
> + * @param kshark_ctx: Input location for context pointer.
> + * @param file: The plugin object file to unregister.
> + */
> +void kshark_unregister_plugin(struct kshark_context *kshark_ctx,
> +			      const char *file)
> +{
> +	struct kshark_plugin_list **last;
> +
> +	for (last = &kshark_ctx->plugins; *last; last = &(*last)->next) {
> +		if (strcmp((*last)->file, file) == 0) {
> +			struct kshark_plugin_list *this_plugin;
> +			this_plugin = *last;
> +			*last = this_plugin->next;
> +
> +			dlclose(this_plugin->handle);
> +			free(this_plugin);
> +
> +			return;
> +		}
> +	}
> +}
> +
> +/**
> + * @brief Free all plugins in a given list.
> + *
> + * @param plugins: Input location for the plugins list.
> + */
> +void kshark_free_plugin_list(struct kshark_plugin_list *plugins)
> +{
> +	struct kshark_plugin_list *last;
> +
> +	while (plugins) {
> +		last = plugins;
> +		plugins = plugins->next;
> +
> +		free(last->file);
> +		dlclose(last->handle);
> +
> +		free(last);
> +	}
> +}
> +
> +/**
> + * @brief Use this function to initialize/update/deinitialize all registered
> + *	  plugins.
> + *
> + * @param kshark_ctx: Input location for context pointer.
> + * @param task_id: Action identifier specifying the action to be executed.
> + *
> + * @returns The number of successful added/removed plugin handlers on success,
> + *	    or a negative error code on failure.
> + */
> +int kshark_handle_plugins(struct kshark_context *kshark_ctx,
> +			  int task_id)

Shouldn't task_id be of type: enum kshark_plugin_actions ?

> +{
> +	struct kshark_plugin_list *plugin;
> +	int handler_count = 0;
> +
> +	if (task_id != KSHARK_PLUGIN_INIT &&
> +	    task_id != KSHARK_PLUGIN_UPDATE &&
> +	    task_id != KSHARK_PLUGIN_CLOSE)
> +		return -EINVAL;

You can simplify the above with:

	switch (task_id) {
	case KSHARK_PLUGIN_INIT:
	case KSHARK_PLUGIN_UPDATE:
	case KSHARK_PLUGIN_CLOSE:
		break;
	default:
		return -EINVAL;
	}

Or, if you want to not even bother, you can add below:

> +
> +	for (plugin = kshark_ctx->plugins; plugin; plugin = plugin->next) {
> +		switch (task_id) {
> +		case KSHARK_PLUGIN_INIT:
> +			handler_count += plugin->init(kshark_ctx);
> +			break;
> +
> +		case KSHARK_PLUGIN_UPDATE:
> +			plugin->close(kshark_ctx);
> +			handler_count += plugin->init(kshark_ctx);
> +			break;
> +
> +		case KSHARK_PLUGIN_CLOSE:
> +			handler_count += plugin->close(kshark_ctx);
> +			break;
		default;
			return -EINVAL;

As a switch needs to have the jumps regardless. There's no need to do
the test first. The only difference is, it wont error if there's no
plugins. But is that a problem?

-- Steve

> +		}
> +	}
> +
> +	return handler_count;
> +}