Re: [RFC 3/4] tcg/plugins: Support for inter-plugin interactions

[Andrew S. Fasano]

> Andrew Fasano <fasano@mit.edu> writes:
> 
> > Expand tcg-plugin system to allow for plugins to export functions
> > and callbacks that can be used by other plugins. Exported functions
> > can be called at runtime by other loaded plugins. Loaded plugins
> > can register functions with exported callbacks and have these
> > functions run whenever the callback is triggered.
> 
> Could you please split the callback and function handling in future
> patches to aid review.

Will do, now that I see your response I understand why that would've been
better.

> 
> >
> > Signed-off-by: Andrew Fasano <fasano@mit.edu>
> > ---
> >  include/qemu/plugin-qpp.h    | 252 +++++++++++++++++++++++++++++++++++
> >  plugins/core.c               |  11 ++
> >  plugins/loader.c             |  24 ++++
> >  plugins/plugin.h             |   5 +
> >  plugins/qemu-plugins.symbols |   1 +
> >  5 files changed, 293 insertions(+)
> >  create mode 100644 include/qemu/plugin-qpp.h
> >
> > diff --git a/include/qemu/plugin-qpp.h b/include/qemu/plugin-qpp.h
> > new file mode 100644
> > index 0000000000..befa4f9b8b
> > --- /dev/null
> > +++ b/include/qemu/plugin-qpp.h
> > @@ -0,0 +1,252 @@
> > +#ifndef PLUGIN_QPP_H
> > +#define PLUGIN_QPP_H
> > +
> > +/*
> > + * Facilities for "QEMU plugin to plugin" (QPP) interactions between tcg
> > + * plugins.  These allow for an inter-plugin callback system as well
> > + * as direct function calls between loaded plugins. For more details see
> > + * docs/devel/plugin.rst.
> > + */
> > +
> > +#include <dlfcn.h>
> > +#include <gmodule.h>
> > +#include <assert.h>
> > +extern GModule * qemu_plugin_name_to_handle(const char *);
> 
> Plugin API functions should have /** */ kernel-doc annotations for the
> benefit of the autogenerated API docs. Moreover handles to plugins are
> usually anonymous pointer types to discourage them fishing about in the
> contents.
> 
> The fact we expose GModule to the plugin to do the linking makes me
> think that maybe the linking should be pushed into loader and be done on
> behalf of the plugin.
[snip]
> This house keeping definitely makes me think event handling would be
> better handled inside of the core plugin code. Instead of jumping
> through macro hoops to track state you could have something like:
> 
>   void * handle = qemu_plugin_register_event_trigger("plugin name", "event 
> name")
>   ...
>   qemu_plugin_trigger_event(handle, void *ev_data);
> 
> and then on the listener side:
> 
>   qemu_plugin_register_event_listener("plugin name", "event name",  my_fn, 
> user_data);
>   ...
>   my_fun(void *ev_data, void *udata) {
>     .. do stuff ..
>   }
>  
> it would also allow both plugins and the core code to introspect which
> events have been added.
> 
You have thoroughly convinced me that this logic would be better in the core
plugin code. I'll try to make that change such that plugins can be designed
like your example above.

> > +
> > +
> > +/*
> > + * QPP_RUN_CB(name, args...) should be run by the plugin that created
> > + * a callback whenever it wishes to trigger the callback functions that
> > + * have been registered with its previously-created callback of the 
> > provided
> > + * name. If this macro is run with a callback name that was not previously
> > + * created, a compile time error will be raised.
> > + */
> > +#define QPP_RUN_CB(cb_name, ...) do {                           \
> > +  int cb_idx;                                                   \
> > +  for (cb_idx = 0; cb_idx < qpp_##cb_name##_num_cb; cb_idx++) { \
> > +    if (qpp_##cb_name##_cb[cb_idx] != NULL) {                   \
> > +      qpp_##cb_name##_cb[cb_idx](__VA_ARGS__);                  \
> > +    }                                                           \
> > +  }                                                             \
> > +} while (false)
> > +
> > +/*
> > + * A header file that defines a callback function should use
> > + * the QPP_CB_PROTOTYPE macro to create the necessary types.
> > + */
> > +#define QPP_CB_PROTOTYPE(fn_ret, name, ...) \
> > +  typedef fn_ret(PLUGIN_CONCAT(name, _t))(__VA_ARGS__);
> 
> If we stick to a single event prototype with event data (which can be
> specified by the other plugin header) and user data that should be
> flexible enough and avoid additional complications?
> 

Sure, removing the magic to handle variable argument would simplify
the implementation while keeping the flexibility here.

> > +
> > +/*
> > + 
> > *****************************************************************************
> > + * The QPP_REG_CB and QPP_REMOVE_CB macros are to be used by plugins that
> > + * wish to run internal logic when another plugin determines that some 
> > event
> > + * has occured. The plugin name, target callback name, and a local function
> > + * are provided to these macros.
> > + 
> > *****************************************************************************
> > + */
> > +
> > +/*
> > + * When a plugin wishes to register a function `cb_func` with a callback
> > + * `cb_name` provided `other_plugin`, it must use the QPP_REG_CB.
> > + */
> > +#define QPP_REG_CB(other_plugin, cb_name, cb_func)      \
> > +{                                                       \
> > +  dlerror();                                            \
> > +  void *h = qemu_plugin_name_to_handle(other_plugin);   \
> > +  if (!h) {                                             \
> > +    fprintf(stderr, "In trying to add plugin callback, "\
> > +                    "couldn't load %s plugin\n",        \
> > +                    other_plugin);                      \
> > +    assert(h);                                          \
> > +  }                                                     \
> > +  void (*add_cb)(cb_name##_t fptr);                     \
> > +  if (g_module_symbol(h, "qpp_add_cb_" #cb_name,        \
> > +                      (gpointer *) &add_cb)) {          \
> > +    add_cb(cb_func);                                    \
> > +  } else {                                              \
> > +    fprintf(stderr, "Could not find symbol " #cb_name   \
> > +            " in " #other_plugin "\n");                 \
> > +  }                                                     \
> 
> Another benefit of doing the linking in loader would be more graceful
> handling of the error cases and reporting.
Agreed.
> 
> > +}
> > +
> > +/*
> > + * If a plugin wishes to disable a previously-registered `cb_func` it 
> > should
> > + * use the QPP_REMOVE_CB macro.
> > + */
> > +#define QPP_REMOVE_CB(other_plugin, cb_name, cb_func)            \
> > +{                                                                \
> > +  dlerror();                                                     \
> > +  void *op = panda_get_plugin_by_name(other_plugin);             \
> > +  if (!op) {                                                     \
> > +    fprintf(stderr, "In trying to remove plugin callback, "      \
> > +                    "couldn't load %s plugin\n", other_plugin);  \
> > +    assert(op);                                                  \
> > +  }                                                              \
> > +  void (*rm_cb)(cb_name##_t fptr) = (void (*)(cb_name##_t))      \
> > +                                    dlsym(op, "qpp_remove_cb_"   \
> > +                                              #cb_name);         \
> > +  assert(rm_cb != 0);                                            \
> > +  rm_cb(cb_func);                                                \
> > +}
> > +
> > +/*
> > + 
> > *****************************************************************************
> > + * The QPP_FUN_PROTOTYPE enables a plugin to expose a local function to 
> > other
> > + * plugins. The macro should be used in a header file that is included
> > + * by both the plugin that defines the function as well as other plugins
> > + * that wish to call the function.
> > + 
> > *****************************************************************************
> > + */
> > +
> > +/*
> > + * A header file that defines an exported function should use the
> > + * QPP_FUN_PROTOTYPE to create the necessary types. When other plugins
> > + * include this header, a function pointer named `[plugin_name]_[fn]` will
> > + * be created and available for the plugin to call.
> > + *
> > + * Note that these functions are not callbacks, but instead regular
> > + * functions that are exported by one plugin such that they can be called 
> > by
> > + * others.
> > + *
> > + * In particular, this macro will create a function pointer by combining 
> > the
> > + * `plugin_name` with an underscore and the name provided in `fn`. It will
> > + * create a function to run on plugin load, that initializes this function
> > + * pointer to the function named `fn` inside the plugin named 
> > `plugin_name`.
> > + * If this fails, an error will be printed and the plugin will abort.
> 
> This replicates a bunch of what the QEMU_PLUGIN_EXPORT does but also
> adds some concatenation rules. Would it not be enough just to export the
> name and then restrict the linking in loader.c to searching plugins in a
> declared list from the source plugin?
> 

I don't think so, but I could certainly be wrong. From what I understand,
QEMU_PLUGIN_EXPORT just makes the labeled function globally visible so its
address can be resolved with dlsym after it's loaded with dlopen.

The initial version of the code I shared here handles this by using macros
that create a constructor that runs on plugin load. Inside these constructors,
the macro-generated code uses the new qemu_plugin_name_to_handle function
to get a handle to the target plugin, then g_module_symbol to map that to
a function pointer. Finally, with this function pointer in hand, it sets up
the function name such that it can be called.

This could be redesigned to move more of this logic into the plugin core and
keep the plugins from directly working with the opaque handles, but I believe
some macros would still be necessary to make the target function name
into an object that could be called. This change would shorten the
QPP_FUN_PROTOTYPE macro to something more like this:

#define QPP_FUN_PROTOTYPE(plugin_name, fn_ret, fn, args)                      \
  fn_ret fn(args);                                                            \
  typedef fn_ret(*PLUGIN_CONCAT(fn, _t))(args);                               \
  fn##_t QPP_NAME(plugin_name, fn);                                           \
  void _QPP_SETUP_NAME(plugin_name, fn) (void);                               \
                                                                              \
  void __attribute__ ((constructor)) _QPP_SETUP_NAME(plugin_name, fn) (void) {\
    qemu_plugin_initialize_qpp_fn_or_error(CURRENT_PLUGIN, plugin_name,       \
                                           #fn, &QPP_NAME(plugin_name, fn))    \
  }

Then some of the logic could move to `qemu_plugin_initialize_qpp_fn_or_error`
which would be in the plugin core core and responsible for handling the
dynamic linking or raising an error. It could also support logging when
plugins load references to one another and (if desired) handle automatically
loading plugins as necessary.

Does that seem like an improvement? Or if there's something I'm missing that
would allow plugins to more easily call functions in another with various
type signatures and without needing to manually cast function pointers, I'm
all ears!

> > + *
> > + * There are three distinct cases this macro must handle:
> > + * 1) When the header is loaded by the plugin that defines the function.
> > + *    In this case, we do not need to find the symbol externally.
> > + *    qemu_plugin_name_to_handle will return NULL, we see that the
> > + *    target plugin matches CURRENT_PLUGIN and do nothing.
> > + * 2) When the header is loaded by another plugin but the function
> > + *    is not available (i.e., the target plugin isn't loaded or the
> > + *    target plugin does not export the requested function). In this case 
> > we
> > + *    raise a fatal error.
> > + * 3) When the header is loaded by another plugin. In this case
> > + *    we need to get a handle to the target plugin and then use
> > + *    g_module_symbol to resolve the symbol and store it as the fn_name.
> > + *    If we get the handle, but can't resolve the symbol, raise a fatal 
> > error.
> > + *
> > + * This plugin creates the following local variables and functions:
> > + *
> > + * 1) `fn`: A prototype for the provided function, with the specified 
> > arguments.
> > + *    This is necessary for case 1 above and irrelevant for the others.
> > + * 2) A function pointer typedef for `[fn]_t` set to a pointer of the type 
> > of
> > + *    `fn`.
> > + * 3) A function pointer (of type `[fn]_t`) named `[plugin_name]_[fn]`
> > + * 4) A constructor function named "_qpp_setup_[plugin_name]_[fn]" that 
> > will
> > + *    run when the plugin is loaded. In case 1 above, it will do nothing. 
> > In
> > + *    case 2 it will print an error to stderr and abort. In case 3 it will
> > + *    update the function pointer "[plugin_name]_[fn]" to point to the 
> > target
> > + *    function.
> > + *
> > + * After this constructor runs, the plugin can directly call the target 
> > function
> > + * using `[plugin_name]_[fn]()`.
> > + *
> > + * For example, consider a plugin named "my_plugin" that wishes to export a
> > + * function  named "my_func" that returns void and takes a single integer 
> > arg.
> > + * This would work as follows:
> > + * 1) The header file for the plugin, my_plugin.h, should include
> > + *    QPP_FUN_PROTOTYPE(my_plugin, void, my_func, int) which will define 
> > the
> > + *    function prototype and necessary internal state.
> > + * 2) This header should be included by my_plugin.c which should also 
> > include
> > + *    QEMU_PLUGIN_EXPORT void my_func(int) {...} with the function 
> > definition.
> > + * 3) Other plugins can get access to this function by including 
> > my_plugin.h
> > + *    which will set up the function pointer `my_plugin_my_func` 
> > automatically
> > + *    using the internal state here.
> > + *
> > + */
> > +#define QPP_FUN_PROTOTYPE(plugin_name, fn_ret, fn, args)                   
> >    \
> > +  fn_ret fn(args);                                                         
> >    \
> > +  typedef fn_ret(*PLUGIN_CONCAT(fn, _t))(args);                            
> >    \
> > +  fn##_t QPP_NAME(plugin_name, fn);                                        
> >    \
> > +  void _QPP_SETUP_NAME(plugin_name, fn) (void);                            
> >    \
> > +                                                                           
> >    \
> > +  void __attribute__ ((constructor)) _QPP_SETUP_NAME(plugin_name, fn) 
> > (void) {\
> > +    GModule *h = qemu_plugin_name_to_handle(#plugin_name);                 
> >    \
> > +    if (!h && strcmp(CURRENT_PLUGIN, #plugin_name) != 0) {        \
> > +      fprintf(stderr, "Error plugin %s cannot access %s. Is it loaded?\n", 
> >    \
> > +                       CURRENT_PLUGIN, #plugin_name);             \
> > +      abort();                                                             
> >    \
> > +    } else if (h && !g_module_symbol(h, #fn,                               
> >    \
> > +                           (gpointer *)&QPP_NAME(plugin_name, fn))) {      
> >    \
> > +      fprintf(stderr, "Error loading symbol " # fn " from plugin "         
> >    \
> > +                      # plugin_name "\n");                                 
> >    \
> > +      abort();                                                             
> >    \
> > +    }                                                                      
> >    \
> > +  }
> 
> While nothing stops a plugin calling abort() itself I'd rather avoid
> having it inserted by macro magic. Another argument for doing this
> inside the core code.

Sounds good to me.

> 
> > +
> > +#endif /* PLUGIN_QPP_H */
> > diff --git a/plugins/core.c b/plugins/core.c
> > index 792262da08..81c714bbf4 100644
> > --- a/plugins/core.c
> > +++ b/plugins/core.c
> > @@ -236,6 +236,17 @@ void qemu_plugin_vcpu_exit_hook(CPUState *cpu)
> >      qemu_rec_mutex_unlock(&plugin.lock);
> >  }
> > 
> > +GModule *qemu_plugin_name_to_handle(const char* name)
> > +{
> > +    struct qemu_plugin_ctx *ctx;
> > +    QTAILQ_FOREACH(ctx, &plugin.ctxs, entry) {
> > +        if (is_plugin_named(*ctx, name)) {
> > +            return plugin_id_to_ctx_locked(ctx->id)->handle;
> 
> The _locked prefix in QEMU indicated the function expects a lock to be
> held. In this case you could use QEMU_LOCK_GUARD(&plugin.lock); at the
> head of the function.

Thanks, I didn't know that.

> 
> > +        }
> > +    }
> > +    return NULL;
> > +}
> > +
> >  struct plugin_for_each_args {
> >      struct qemu_plugin_ctx *ctx;
> >      qemu_plugin_vcpu_simple_cb_t cb;
> > diff --git a/plugins/loader.c b/plugins/loader.c
> > index 88c30bde2d..26d3c14661 100644
> > --- a/plugins/loader.c
> > +++ b/plugins/loader.c
> > @@ -265,6 +265,30 @@ static int plugin_load(struct qemu_plugin_desc *desc, 
> > const qemu_info_t *info, E
> >      return 1;
> >  }
> > 
> > +/**
> > + * is_plugin_named - compare a plugins's name to a provided name
> > + * @ctx: the ctx for the plugin in question
> > + * @name: the name that should be used in the comparison
> > + *
> > + * Returns true if the names match.
> > + */
> > +
> > +bool is_plugin_named(struct qemu_plugin_ctx ctx, const char *name)
> > +{
> > +  char *plugin_basename = basename(ctx.desc->path);
> > +  /*
> > +   * First resolve the name of the shared object for the current plugin,
> > +   * then check if it could possibly be of the form libNAME.so
> > +   * where NAME is the provided name. If so, compare the strings.
> > +   */
> > +  if (plugin_basename == NULL || strlen(plugin_basename) != strlen(name) + 
> > 6) {
> > +    return false;
> > +  }
> > +
> > +  return strncmp(plugin_basename + 3, name,
> > +                 strlen(plugin_basename) - 6) == 0;
> 
> I'd rather favour just making plugin names mandatory with the API boost.

I agree.

> 
> > +}
> > +
> >  /* call after having removed @desc from the list */
> >  static void plugin_desc_free(struct qemu_plugin_desc *desc)
> >  {
> > diff --git a/plugins/plugin.h b/plugins/plugin.h
> > index 5eb2fdbc85..69d9b09d4c 100644
> > --- a/plugins/plugin.h
> > +++ b/plugins/plugin.h
> > @@ -97,4 +97,9 @@ void plugin_register_vcpu_mem_cb(GArray **arr,
> > 
> >  void exec_inline_op(struct qemu_plugin_dyn_cb *cb);
> > 
> > +GModule *qemu_plugin_name_to_handle(const char* name);
> 
> exported functions shouldn't need to be declared twice, but as above we
> can drop this I think.
> 
> > +
> > +/* loader.c */
> > +bool is_plugin_named(struct qemu_plugin_ctx ctx, const char *name);
> > +
> >  #endif /* PLUGIN_H */
> > diff --git a/plugins/qemu-plugins.symbols b/plugins/qemu-plugins.symbols
> > index 71f6c90549..d98c0bc38a 100644
> > --- a/plugins/qemu-plugins.symbols
> > +++ b/plugins/qemu-plugins.symbols
> > @@ -18,6 +18,7 @@
> >    qemu_plugin_mem_size_shift;
> >    qemu_plugin_n_max_vcpus;
> >    qemu_plugin_n_vcpus;
> > +  qemu_plugin_name_to_handle;
> >    qemu_plugin_outs;
> >    qemu_plugin_path_to_binary;
> >    qemu_plugin_register_atexit_cb;
> 
> 
> --
> Alex Bennée