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

[Anthony Liguori]

On 06/26/2012 03:42 AM, Markus Armbruster wrote:
> 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.

Okay, let me turn this around:

How do people normally limit this beyond just eye-balling?  My terminals are 
80-width as god intended them to be.  Trying to guess whether you cross 75 or 
not seems to be a bit silly.  git also doesn't do anything helpful like stick an 
indicator in the comments below the message where the 75 character mark is.

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

checkpatch.pl is dumb.  I don't see any long lines in the patch...

> > 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.

Hrm, I tried very hard to make sure it was significant.  Otherwise you can't do 
something like:

-object rng-urandom,filename=/dev/foo,opened=true

b/c filename needs to be set before opened gets set since filename is checked to 
be != NULL when opened is set to true.

> > 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?

Every path is a unique namespace.  I'm sticking everything in /objects right now 
because it's a unique namespace that won't conflict with other namespaces (like 
/block or /peripheral).  I wish we only used one unique namespace because then 
we can just refer to short names which is why I didn't introduce a /rng namespace.

Using a single namespace makes it easier to work with paths because then you can 
rely on partial path resolution.

At some point, we will truly want -device to be an alias for -object and then 
we'll probably want to add a qom-container argument to -object in order to 
specify which container the object should be created in.

But I didn't do it here because I don't want people placing things in random 
containers.

> >   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.

I assumed implied_opt_name also forced that option to be required.  I'll double 
check.  This is why I put the assert (to assert my assumption).

Regards,

Anthony Liguori

> > +
> > +    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.