On Mon, 4 Nov 2019 at 13:18, Alex Bennée <alex.ben...@linaro.org> wrote:
>
> This is a very simple versioning API which allows the plugin
> infrastructure to check the API a plugin was built against. We also
> expose a min/cur API version to the plugin via the info block in case
> it wants to avoid using old deprecated APIs in the future.
I think the general idea here is fine, and I want to see
us get the version-check into 4.2, because I don't think
we should release a QEMU which doesn't do a basic version
sanity check. I have some minor comments below.
> Signed-off-by: Alex Bennée <alex.ben...@linaro.org>
> ---
> include/qemu/qemu-plugin.h | 19 +++++++++++++++++++
> plugins/loader.c | 15 +++++++++++++++
> plugins/plugin.h | 2 ++
> tests/plugin/bb.c | 2 ++
> tests/plugin/empty.c | 2 ++
> tests/plugin/hotpages.c | 2 ++
> tests/plugin/howvec.c | 2 ++
> tests/plugin/insn.c | 2 ++
> tests/plugin/mem.c | 2 ++
> 9 files changed, 48 insertions(+)
>
> diff --git a/include/qemu/qemu-plugin.h b/include/qemu/qemu-plugin.h
> index a00a7deb461..5502e112c81 100644
> --- a/include/qemu/qemu-plugin.h
> +++ b/include/qemu/qemu-plugin.h
As an aside, is this the header file used for building
the plugins? It seems a bit odd to have put it
in include/qemu, which is for headers used when building
QEMU itself.
> @@ -38,9 +38,28 @@
>
> typedef uint64_t qemu_plugin_id_t;
>
> +/*
> + * Versioning plugins:
> + *
> + * The plugin API will pass a minimum and current API version that
> + * QEMU currently supports. The minimum API will be incremented if an
> + * API needs to be deprecated.
> + *
> + * The plugins export the API they were built against by exposing the
> + * symbol qemu_plugin_version which can be checked.
> + */
> +
> +extern QEMU_PLUGIN_EXPORT int qemu_plugin_version;
> +
> +#define QEMU_PLUGIN_VERSION 0
> +
> typedef struct {
> /* string describing architecture */
> const char *target_name;
> + struct {
> + int min;
> + int cur;
> + } version;
> /* is this a full system emulation? */
> bool system_emulation;
> union {
> diff --git a/plugins/loader.c b/plugins/loader.c
> index ce724ed5839..1bcca909691 100644
> --- a/plugins/loader.c
> +++ b/plugins/loader.c
> @@ -178,6 +178,19 @@ static int plugin_load(struct qemu_plugin_desc *desc,
> const qemu_info_t *info)
> goto err_symbol;
> }
>
> + if (!g_module_symbol(ctx->handle, "qemu_plugin_version", &sym)) {
> + warn_report("%s: missing version %s", __func__, g_module_error());
Failure to declare the required version should be an error,
not just a warning.
> + } else {
> + int version = *(int *)sym;
> + if (version < QEMU_PLUGIN_MIN_VERSION ||
> + version > QEMU_PLUGIN_VERSION) {
> + error_report("%s: bad plugin version %d vs %d/%d",
> + __func__, version, QEMU_PLUGIN_MIN_VERSION,
> + QEMU_PLUGIN_VERSION);
I think this message is too cryptic, and would prefer
something more like:
"TCG plugin %s requires API version %d, but this QEMU supports only
versions %d to %d"
where the first %s is the name of the plugin binary, not
the __func__ of this function (which is not useful to
the end user).
Better still, special case the MIN_VERSION == VERSION
case to avoid saying "versions 1 to 1".
> + goto err_symbol;
> + }
> + }
> +
> qemu_rec_mutex_lock(&plugin.lock);
>
> /* find an unused random id with &ctx as the seed */
> @@ -248,6 +261,8 @@ int qemu_plugin_load_list(QemuPluginList *head)
> g_autofree qemu_info_t *info = g_new0(qemu_info_t, 1);
>
> info->target_name = TARGET_NAME;
> + info->version.min = QEMU_PLUGIN_MIN_VERSION;
> + info->version.cur = QEMU_PLUGIN_VERSION;
> #ifndef CONFIG_USER_ONLY
> MachineState *ms = MACHINE(qdev_get_machine());
> info->system_emulation = true;
thanks
-- PMM
