From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:49923) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aBPjV-0002au-Dy for qemu-devel@nongnu.org; Tue, 22 Dec 2015 11:24:21 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aBPjT-0008Hp-PF for qemu-devel@nongnu.org; Tue, 22 Dec 2015 11:24:17 -0500 References: <1450782389-17326-1-git-send-email-berrange@redhat.com> <1450782389-17326-3-git-send-email-berrange@redhat.com> From: Eric Blake Message-ID: <56797920.9030306@redhat.com> Date: Tue, 22 Dec 2015 09:24:00 -0700 MIME-Version: 1.0 In-Reply-To: <1450782389-17326-3-git-send-email-berrange@redhat.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="SIsRUmepXcAAEJo1lUhJioSfFn94siUF0" Subject: Re: [Qemu-devel] [PATCH 2/7] qemu-img: add support for --object command line arg List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Daniel P. Berrange" , qemu-devel@nongnu.org Cc: Kevin Wolf , Paolo Bonzini , Markus Armbruster , qemu-block@nongnu.org, =?UTF-8?Q?Andreas_F=c3=a4rber?= This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --SIsRUmepXcAAEJo1lUhJioSfFn94siUF0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable 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" :) > passwords and/or encryption keys to the various block driver > backends via the recently added 'secret' object type. >=20 > # 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. > # qemu-img info --object secret,id=3Dsec0,file=3Dmypasswd.txt \ > ...other info args... >=20 > Signed-off-by: Daniel P. Berrange > --- > 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. > @@ -94,6 +98,11 @@ static void QEMU_NORETURN help(void) > "\n" > "Command parameters:\n" > " 'filename' is a disk image filename\n" > + " 'objectdef' is a QEMU user creatable object definition. = See \n" Trailing whitespace on the user's terminal. > + " the @code{qemu(1)} manual page for a description of th= e object\n" > + " properties. The only object type that it makes sense t= o define\n" > + " is the @code{secret} object, which is used to supply p= asswords\n" > + " and/or encryption keys.\n" > " 'fmt' is the disk image format. It is guessed automatica= lly 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. [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 =3D NULL; > + char *type =3D NULL; > + char *id =3D NULL; > + void *dummy =3D NULL; Drop this. > + OptsVisitor *ov; > + QDict *pdict; Add a Visitor *v; helper variable. > + > + ov =3D opts_visitor_new(opts); > + pdict =3D qemu_opts_to_qdict(opts, NULL); > + > + visit_start_struct(opts_get_visitor(ov), &dummy, NULL, NULL, 0, &e= rr); 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 :) > + > + 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= =2E > @@ -319,6 +397,13 @@ static int img_create(int argc, char **argv) > case 'q': > quiet =3D true; > break; > + case OPTION_OBJECT: > + opts =3D 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. --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --SIsRUmepXcAAEJo1lUhJioSfFn94siUF0 Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 Comment: Public key at http://people.redhat.com/eblake/eblake.gpg Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQEcBAEBCAAGBQJWeXkgAAoJEKeha0olJ0NqUJUIAKIuappaZnJ+ZnR6UPVGD+Fk P9M1+vWnKfuQsYJdd3V9NgpeyTaQLODPx2IApOlJim5JBp6dm+FrgDHwfxPcd+hT T+eyjYXUkGNUoo4/DgbvhJg1z5Qjcoqcf2gPw3l7FXBMABuwn23kNj3Cx+bv8jeI H4qGQPMmC/NyHRHv3MCbthMJIAPm39fmJaykTALP8J0BJ7F9bv92rd8NRs2EXWqb V+pFjE/REvlMlTkwvLU8zEHwi1xnPfdH4XdYuo8nD81QcXv9J7Ox2MUcIlzhOz+/ L/XJCX8tZTPVbjVWqK+7q9oiyPAsPoCFD/eQ1aSGipxyyxraBGTmHNqowUjIOyg= =W5BE -----END PGP SIGNATURE----- --SIsRUmepXcAAEJo1lUhJioSfFn94siUF0--