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