[RFC PATCH] Subject: [RFC PATCH] plugins: Passed the parsed arguments directly to plugins

[RFC PATCH] Subject: [RFC PATCH] plugins: Passed the parsed arguments directly to plugins

[Mahmoud Mandour]

Arguments were passed to plugins in the following form:
    -plugin path/to/plugin,arg="positional_arg=value",arg="second_arg"

This patch removes the need for "arg" so that the argument name itself
is now expected and passed directly to the plugin.

Now options can be passed in the following manner:
    -plugin path/to/plugin,positional_arg=value,second_arg

Since short boolean arguments are deprecated, passing an argument that
takes no value will trigger a warning saying that the user should use a
full "arg_name=on" instead of just "arg_name". In either case, the
argument is passed to the plugin as only the name, omitting the "=on"
part.

Signed-off-by: Mahmoud Mandour <ma.mandourr@gmail.com>
---
 plugins/loader.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/plugins/loader.c b/plugins/loader.c
index 05df40398d..7f32b8c8bd 100644
--- a/plugins/loader.c
+++ b/plugins/loader.c
@@ -94,6 +94,7 @@ static int plugin_add(void *opaque, const char *name, const char *value,
 {
     struct qemu_plugin_parse_arg *arg = opaque;
     struct qemu_plugin_desc *p;
+    char *full_arg;
 
     if (strcmp(name, "file") == 0) {
         if (strcmp(value, "") == 0) {
@@ -107,7 +108,7 @@ static int plugin_add(void *opaque, const char *name, const char *value,
             QTAILQ_INSERT_TAIL(arg->head, p, entry);
         }
         arg->curr = p;
-    } else if (strcmp(name, "arg") == 0) {
+    } else {
         if (arg->curr == NULL) {
             error_setg(errp, "missing earlier '-plugin file=' option");
             return 1;
@@ -115,9 +116,12 @@ static int plugin_add(void *opaque, const char *name, const char *value,
         p = arg->curr;
         p->argc++;
         p->argv = g_realloc_n(p->argv, p->argc, sizeof(char *));
-        p->argv[p->argc - 1] = g_strdup(value);
-    } else {
-        error_setg(errp, "-plugin: unexpected parameter '%s'; ignored", name);
+        if (strcmp(value, "on")) {
+            full_arg = g_strdup_printf("%s=%s", name, value);
+        } else {
+            full_arg = g_strdup(name);
+        }
+        p->argv[p->argc - 1] = full_arg;
     }
     return 0;
 }
-- 
2.25.1

Re: [RFC PATCH] Subject: [RFC PATCH] plugins: Passed the parsed arguments directly to plugins

[Alex Bennée]

Mahmoud Mandour <ma.mandourr@gmail.com> writes:

The subject got a bit mangled. I usually send single one off patches
directly from the command line like this:

  git send-email --subject-prefix "RFC PATCH" HEAD^.. --to qemu HEAD^..

> Arguments were passed to plugins in the following form:
>     -plugin path/to/plugin,arg="positional_arg=value",arg="second_arg"
>
> This patch removes the need for "arg" so that the argument name itself
> is now expected and passed directly to the plugin.
>
> Now options can be passed in the following manner:
>     -plugin path/to/plugin,positional_arg=value,second_arg
>
> Since short boolean arguments are deprecated, passing an argument that
> takes no value will trigger a warning saying that the user should use a
> full "arg_name=on" instead of just "arg_name". In either case, the
> argument is passed to the plugin as only the name, omitting the "=on"
> part.
>
> Signed-off-by: Mahmoud Mandour <ma.mandourr@gmail.com>
> ---
>  plugins/loader.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/plugins/loader.c b/plugins/loader.c
> index 05df40398d..7f32b8c8bd 100644
> --- a/plugins/loader.c
> +++ b/plugins/loader.c
> @@ -94,6 +94,7 @@ static int plugin_add(void *opaque, const char *name, const char *value,
>  {
>      struct qemu_plugin_parse_arg *arg = opaque;
>      struct qemu_plugin_desc *p;
> +    char *full_arg;
>  
>      if (strcmp(name, "file") == 0) {
>          if (strcmp(value, "") == 0) {
> @@ -107,7 +108,7 @@ static int plugin_add(void *opaque, const char *name, const char *value,
>              QTAILQ_INSERT_TAIL(arg->head, p, entry);
>          }
>          arg->curr = p;
> -    } else if (strcmp(name, "arg") == 0) {
> +    } else {

Unfortunately I think we also want to support the arg= form for now and
also update the deprecated.rst and plugin.rst documentation to point to
the new format.

>          if (arg->curr == NULL) {
>              error_setg(errp, "missing earlier '-plugin file=' option");
>              return 1;
> @@ -115,9 +116,12 @@ static int plugin_add(void *opaque, const char *name, const char *value,
>          p = arg->curr;
>          p->argc++;
>          p->argv = g_realloc_n(p->argv, p->argc, sizeof(char *));
> -        p->argv[p->argc - 1] = g_strdup(value);
> -    } else {
> -        error_setg(errp, "-plugin: unexpected parameter '%s'; ignored", name);
> +        if (strcmp(value, "on")) {
> +            full_arg = g_strdup_printf("%s=%s", name, value);

We should probably prefer g_strcmp0 to strcmp and an explicit value test
(!= 0) because strcmp is weird with truth/falsity. However as we are
handling the syntactic sugar of adding "on" in the command line I wonder
if we can query QemuOpt directly. Maybe something like:

        /* pass true bools as single arg */
        if (qapi_bool_parse(name, value, &is_true, errp) && is_true) {
            full_arg = g_strdup(name);
        } else {
            full_arg = g_strdup_printf("%s=%s", name, value);
        }


> +        } else {
> +            full_arg = g_strdup(name);
> +        }
> +        p->argv[p->argc - 1] = full_arg;
>      }
>      return 0;
>  }


-- 
Alex Bennée