Re: [Qemu-block] [Qemu-devel] [PATCH 2/7] qemu-img: add support for --object command line arg

[Daniel P. Berrange]

On Tue, Dec 22, 2015 at 09:24:00AM -0700, Eric Blake wrote:
> On 12/22/2015 04:06 AM, Daniel P. Berrange wrote:
> > Allow creation of user creatable object types with qemu-img
> > via a --object command line arg. This will be used to supply
> 
> Does this read better as "a dash-dash-object", or "an object", in which
> case you may have an article mismatch?  You can skirt the issue by
> adding an adjective: "a new --object" works with either pronunciation of
> "--object" :)

Heh, ok

> > passwords and/or encryption keys to the various block driver
> > backends via the recently added 'secret' object type.
> > 
> >  # echo -n letmein > mypasswd.txt
> 
> 'echo -n' is not portable; although it doesn't matter here, I tend to
> favor 'printf letmein' for both its portability and for less typing.

Yep, I fixed my other patches to use printf previously too.

> >  qemu-img-cmds.hx |  44 ++++----
> >  qemu-img.c       | 300 
> > +++++++++++++++++++++++++++++++++++++++++++++++++++++--
> >  qemu-img.texi    |   8 ++
> >  3 files changed, 322 insertions(+), 30 deletions(-)
> 
> How will libvirt discover whether qemu-img is new enough to support this
> syntax?  Then again, qemu-img isn't used quite as heavily as qemu, and
> the speedups we gain by using QMP instead of -help scraping on qemu
> don't matter quite as much as what we can attempt with -help scraping on
> qemu-img.

Yeah, qemu-img feature detection is an outstanding unsolved problem
that I don't really have an answer for. In general though, I think
libvirt will just take the approach of blindly try to use it, if and
only if, we actually need the new feature. That should not create
us any back-compat problems, and get us moderately acceptable error
reporting if we try to use new feature with old qemu-img.

> > +           "    the @code{qemu(1)} manual page for a description of the 
> > object\n"
> > +           "    properties. The only object type that it makes sense to 
> > define\n"
> > +           "    is the @code{secret} object, which is used to supply 
> > passwords\n"
> > +           "    and/or encryption keys.\n"
> >             "  'fmt' is the disk image format. It is guessed automatically 
> > in most cases\n"
> >             "  'cache' is the cache mode used to write the output disk 
> > image, the valid\n"
> >             "    options are: 'none', 'writeback' (default, except for 
> > convert), 'writethrough',\n"
> 
> You wrapped the text to fit in 80 source columns, but the lines below
> wrapped to keep it at 80 user display columns (at the expense of longer
> source text).  I'd actually lean towards the longer lines in this case,
> even if we have to manually ignore checkpatch.pl.

Yep, good point, I missed that distinction.

> [GNU coreutils does it like:
> 
>     printf("\
> long line starting in column 0\n\
> etc.");
> 
> so that you can fit much closer to 80 output characters while still
> staying within 80 source columns; but I don't think we need the churn of
> taking on that style]
> 
> 
> > +static int object_create(void *opaque, QemuOpts *opts, Error **errp)
> > +{
> > +    Error *err = NULL;
> > +    char *type = NULL;
> > +    char *id = NULL;
> > +    void *dummy = NULL;
> 
> Drop this.
> 
> > +    OptsVisitor *ov;
> > +    QDict *pdict;
> 
> Add a Visitor *v; helper variable.
> 
> > +
> > +    ov = opts_visitor_new(opts);
> > +    pdict = qemu_opts_to_qdict(opts, NULL);
> > +
> > +    visit_start_struct(opts_get_visitor(ov), &dummy, NULL, NULL, 0, &err);
> 
> This conflicts with my pending patches:
> https://lists.gnu.org/archive/html/qemu-devel/2015-12/msg03863.html
> 
> If mine go in first, you'll want this to be:
> 
> visit_start_struct(v, NULL, NULL, 0, &err);
> 
> And even if yours goes in first, you should make it look more like this,
> so I don't have to fix it up after you:
> https://lists.gnu.org/archive/html/qemu-devel/2015-12/msg03862.html
> 
> (since it looks like you copied from there anyways :)

Yep, ok.

> 
> 
> > +
> > +    user_creatable_add(type, id, pdict, opts_get_visitor(ov), &err);
> > +    if (err) {
> > +        goto out;
> > +    }
> > +    visit_end_struct(opts_get_visitor(ov), &err);
> 
> visit_end_struct() needs to be called unconditionally if
> visit_start_struct() succeeded.  Again, if my series goes in first,
> rebase it to look like my changes to vl.c; if yours goes in first, I'll
> have to touch up your additions to match what I do elsewhere in my series.
> 
> > @@ -319,6 +397,13 @@ static int img_create(int argc, char **argv)
> >          case 'q':
> >              quiet = true;
> >              break;
> > +        case OPTION_OBJECT:
> > +            opts = qemu_opts_parse_noisily(qemu_find_opts("object"),
> > +                                           optarg, true);
> > +            if (!opts) {
> > +                exit(1);
> 
> Not for this patch, but maybe someday we should switch to
> exit(EXIT_FAILURE) throughout the file.

Yeah, that came to mind when I was working on these patches. A cleanup
for another day though, lest my number of pending patches exceed 100 !


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