Re: [Qemu-devel] [PATCH 3/4] vl: add -object option to create QOM objects from the command line

[Markus Armbruster]

Anthony Liguori <address@hidden> writes:

> This will create a new QOM object in the '/objects' path.  Note that 
> properties

Long line, will look fugly in git-log.  Please wrap at column 70-75.

checkpatch.pl complains about long lines in the patch proper, too :)

> are set in order which allows for simple objects to be initialized entirely
> with this option and then realized.

Is there any way to avoid making the option order significant?  I find
that a rather poor user interface.

> This option is roughly equivalent to -device but for things that are not
> devices.
>
> Signed-off-by: Anthony Liguori <address@hidden>
> ---
>  qemu-config.c   |   10 ++++++++++
>  qemu-options.hx |    8 ++++++++
>  vl.c            |   55 
> +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 73 insertions(+), 0 deletions(-)
>
> diff --git a/qemu-config.c b/qemu-config.c
> index bb3bff4..8135430 100644
> --- a/qemu-config.c
> +++ b/qemu-config.c
> @@ -614,6 +614,15 @@ QemuOptsList qemu_boot_opts = {
>      },
>  };
>  
> +QemuOptsList qemu_object_opts = {
> +    .name = "object",
> +    .implied_opt_name = "qom-type",
> +    .head = QTAILQ_HEAD_INITIALIZER(qemu_object_opts.head),
> +    .desc = {
> +        { }
> +    },
> +};
> +
>  static QemuOptsList *vm_config_groups[32] = {
>      &qemu_drive_opts,
>      &qemu_chardev_opts,
> @@ -629,6 +638,7 @@ static QemuOptsList *vm_config_groups[32] = {
>      &qemu_machine_opts,
>      &qemu_boot_opts,
>      &qemu_iscsi_opts,
> +    &qemu_object_opts,
>      NULL,
>  };
>  
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 8b66264..20cfe1c 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -2743,6 +2743,14 @@ DEF("qtest-log", HAS_ARG, QEMU_OPTION_qtest_log,
>      "-qtest-log LOG  specify tracing options\n",
>      QEMU_ARCH_ALL)
>  
> +DEF("object", HAS_ARG, QEMU_OPTION_object,
> +    "-object TYPENAME[,PROP1=VALUE1,...]\n"
> +    "                create an new object of type TYPENAME setting 
> properties\n"
> +    "                in the order they are specified.  Note that the 'id'\n"
> +    "                property must be set.  These objects are placed in 
> the\n"
> +    "                '/objects' path.\n",
> +    QEMU_ARCH_ALL)
> +

Could you explain why putting these into /objects always is fine?

Doesn't this mean that -object is *not* more general than -device?

>  HXCOMM This is the last statement. Insert new options before this line!
>  STEXI
>  @end table
> diff --git a/vl.c b/vl.c
> index 1329c30..ac25153 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -161,6 +161,7 @@ int main(int argc, char **argv)
>  #include "arch_init.h"
>  
>  #include "ui/qemu-spice.h"
> +#include "qapi/string-input-visitor.h"
>  
>  //#define DEBUG_NET
>  //#define DEBUG_SLIRP
> @@ -2255,6 +2256,53 @@ static void free_and_trace(gpointer mem)
>      free(mem);
>  }
>  
> +static int object_set_property(const char *name, const char *value, void 
> *opaque)
> +{
> +    Object *obj = OBJECT(opaque);
> +    StringInputVisitor *siv;
> +    Error *local_err = NULL;
> +
> +    if (strcmp(name, "qom-type") == 0 || strcmp(name, "id") == 0) {
> +        return 0;
> +    }
> +
> +    siv = string_input_visitor_new(value);
> +    object_property_set(obj, string_input_get_visitor(siv), name, 
> &local_err);
> +    string_input_visitor_cleanup(siv);
> +
> +    if (local_err) {
> +        qerror_report_err(local_err);
> +        error_free(local_err);
> +        return -1;
> +    }
> +
> +    return 0;
> +}
> +
> +static int object_create(QemuOpts *opts, void *opaque)
> +{
> +    const char *type = qemu_opt_get(opts, "qom-type");
> +    const char *id = qemu_opts_id(opts);
> +    Object *obj;
> +
> +    g_assert(type != NULL);

I suspect "-object x=y" makes this fail.

> +
> +    if (id == NULL) {
> +        qerror_report(QERR_MISSING_PARAMETER, "id");
> +        return -1;
> +    }
> +
> +    obj = object_new(type);
> +    if (qemu_opt_foreach(opts, object_set_property, obj, 1) < 0) {
> +        return -1;
> +    }
> +
> +    object_property_add_child(container_get(object_get_root(), "/objects"),
> +                              id, obj, NULL);
> +
> +    return 0;
> +}
> +
>  int qemu_init_main_loop(void)
>  {
>      return main_loop_init();
> @@ -3199,6 +3247,9 @@ int main(int argc, char **argv, char **envp)
>              case QEMU_OPTION_qtest_log:
>                  qtest_log = optarg;
>                  break;
> +            case QEMU_OPTION_object:
> +                opts = qemu_opts_parse(qemu_find_opts("object"), optarg, 1);
> +                break;
>              default:
>                  os_parse_cmd_args(popt->index, optarg);
>              }
> @@ -3206,6 +3257,10 @@ int main(int argc, char **argv, char **envp)
>      }
>      loc_set_none();
>  
> +    if (qemu_opts_foreach(qemu_find_opts("object"), object_create, NULL, 0) 
> != 0) {
> +        exit(1);
> +    }
> +
>      if (machine->hw_version) {
>          qemu_set_version(machine->hw_version);
>      }

Don't let yourself be discouraged by my griping.  I actually like the
feature.