Re: [Qemu-devel] [PATCH 4/7] qemu-io: add support for --object command line arg

["Daniel P. Berrange" <berrange@redhat.com>]

On Tue, Dec 22, 2015 at 09:55:17AM -0700, Eric Blake wrote:
> On 12/22/2015 04:06 AM, Daniel P. Berrange wrote:
> > Allow creation of user creatable object types with qemu-io
> > via a --object command line arg. This will be used to supply
> > passwords and/or encryption keys to the various block driver
> > backends via the recently added 'secret' object type.
> > 
> >  # echo -n letmein > mypasswd.txt
> >  # qemu-io --object secret,id=sec0,file=mypasswd.txt \
> >       ...other args...
> 
> Same comments as on 3/7.
> 
> > 
> > Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> > ---
> >  qemu-io.c | 87 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 87 insertions(+)
> 
> > @@ -205,6 +207,9 @@ static void usage(const char *name)
> >  "Usage: %s [-h] [-V] [-rsnm] [-f FMT] [-c STRING] ... [file]\n"
> >  "QEMU Disk exerciser\n"
> >  "\n"
> > +"  --object OBJECTDEF   define a object such as 'secret' for\n"
> 
> s/a object/an object/
> 
> > +"                       providing passwords and/or encryption\n"
> > +"                       keys\n"
> 
> 3 lines feels long; you got away with only 2 lines in 3/7 by using
> longer line wrapping, while still fitting in the user's 80 column output:
> 
> +"  --object type,id=ID,...   define an object such as 'secret' for
> providing\n"
> +"                            passwords and/or encryption keys\n"
> 
> 
> >  
> > +enum {
> > +    OPTION_OBJECT = 258,
> > +};
> 
> 256 would work. But 258 doesn't hurt.
> 
> 
> > +static int object_create(void *opaque, QemuOpts *opts, Error **errp)
> > +{
> > +    Error *err = NULL;
> > +    char *type = NULL;
> > +    char *id = NULL;
> > +    void *dummy = NULL;
> > +    OptsVisitor *ov;
> > +    QDict *pdict;
> > +
> > +    ov = opts_visitor_new(opts);
> > +    pdict = qemu_opts_to_qdict(opts, NULL);
> > +
> > +    visit_start_struct(opts_get_visitor(ov), &dummy, NULL, NULL, 0, &err);
> 
> Same comments as on 3/7.
> 
> We now have 5 very similar functions (hmp.c, vl.c, and your three
> additions); should this be factored into a common reusable function
> rather than open-coding it into each client?

Yeah, it is a bit questionable to have this duplication. I already saved
a  fair bit with the first patch introducing the shared user_createable_new.

I was a little reluctant to move this 'object_create' method into the
qom/ code though, since I hate the idea of the legacy 'QemuOpts' data
anywhere near those nice new APIs. I guess I could perhaps just keep the
qemu_opts_to_qdict() call in the caller.

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|