| Message ID | 20170515140410.14172-5-berrange@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 2017-05-15 16:04, Daniel P. Berrange wrote: > The qemu-img dd/convert commands will create an image file and > then try to open it. Historically it has been possible to open > new files without passing any options. With encrypted files > though, the *key-secret options are mandatory, so we need to > provide those options when opening the newly created file. > > Reviewed-by: Max Reitz <mreitz@redhat.com> > Reviewed-by: Fam Zheng <famz@redhat.com> > Reviewed-by: Eric Blake <eblake@redhat.com> > Signed-off-by: Daniel P. Berrange <berrange@redhat.com> > --- > qemu-img.c | 42 +++++++++++++++++++++++++++++++++++++----- > 1 file changed, 37 insertions(+), 5 deletions(-) > > diff --git a/qemu-img.c b/qemu-img.c > index e0e3d31..dcddded 100644 > --- a/qemu-img.c > +++ b/qemu-img.c > @@ -314,15 +314,18 @@ static BlockBackend *img_open_opts(const char *optstr, > } > > static BlockBackend *img_open_file(const char *filename, > + QDict *options, > const char *fmt, int flags, > bool writethrough, bool quiet, > bool force_share) > { > BlockBackend *blk; > Error *local_err = NULL; > - QDict *options = qdict_new(); > > if (fmt) { > + if (!options) { > + options = qdict_new(); > + } This is the only place where my attempted rebase and your version differ. I think this has to be done unconditionally, because otherwise: $ ./qemu-img info -U null-co:// [1] 16327 segmentation fault (core dumped) ./qemu-img info -U null-co:// Also, I'm not sure the R-bs apply for this patch any longer. (They do for patch 1 because it's just a contextual difference. For patch 2, it's a borderline case (I would drop it, but I can understand keeping it). For patch 3 it's more than just borderline - I would definitely drop the R-b, but the differences are still rather mechanical, so it is acceptable to keep it. But I think there are too many changes here in this patch to keep the R-bs. In fact, I'm pretty sure none of Eric, Fam and me have given an R-b to this segfault...) Max > qdict_put_str(options, "driver", fmt); > } > > @@ -344,6 +347,35 @@ static BlockBackend *img_open_file(const char *filename, > } > > > +static int img_add_key_secrets(void *opaque, > + const char *name, const char *value, > + Error **errp) > +{ > + QDict *options = opaque; > + > + if (g_str_has_suffix(name, "key-secret")) { > + qdict_put(options, name, qstring_from_str(value)); > + } > + > + return 0; > +} > + > +static BlockBackend *img_open_new_file(const char *filename, > + QemuOpts *create_opts, > + const char *fmt, int flags, > + bool writethrough, bool quiet, > + bool force_share) > +{ > + QDict *options = NULL; > + > + options = qdict_new(); > + qemu_opt_foreach(create_opts, img_add_key_secrets, options, &error_abort); > + > + return img_open_file(filename, options, fmt, flags, writethrough, quiet, > + force_share); > +} > + > + > static BlockBackend *img_open(bool image_opts, > const char *filename, > const char *fmt, int flags, bool writethrough, > @@ -364,7 +396,7 @@ static BlockBackend *img_open(bool image_opts, > blk = img_open_opts(filename, opts, flags, writethrough, quiet, > force_share); > } else { > - blk = img_open_file(filename, fmt, flags, writethrough, quiet, > + blk = img_open_file(filename, NULL, fmt, flags, writethrough, quiet, > force_share); > } > return blk; > @@ -2286,8 +2318,8 @@ static int img_convert(int argc, char **argv) > * That has to wait for bdrv_create to be improved > * to allow filenames in option syntax > */ > - s.target = img_open_file(out_filename, out_fmt, flags, > - writethrough, quiet, false); > + s.target = img_open_new_file(out_filename, opts, out_fmt, > + flags, writethrough, quiet, false); > } > if (!s.target) { > ret = -1; > @@ -4351,7 +4383,7 @@ static int img_dd(int argc, char **argv) > * with the bdrv_create() call above which does not > * support image-opts style. > */ > - blk2 = img_open_file(out.filename, out_fmt, BDRV_O_RDWR, > + blk2 = img_open_file(out.filename, NULL, out_fmt, BDRV_O_RDWR, > false, false, false); > > if (!blk2) { >
On 2017-05-15 19:43, Max Reitz wrote: > On 2017-05-15 16:04, Daniel P. Berrange wrote: >> The qemu-img dd/convert commands will create an image file and >> then try to open it. Historically it has been possible to open >> new files without passing any options. With encrypted files >> though, the *key-secret options are mandatory, so we need to >> provide those options when opening the newly created file. >> >> Reviewed-by: Max Reitz <mreitz@redhat.com> >> Reviewed-by: Fam Zheng <famz@redhat.com> >> Reviewed-by: Eric Blake <eblake@redhat.com> >> Signed-off-by: Daniel P. Berrange <berrange@redhat.com> >> --- >> qemu-img.c | 42 +++++++++++++++++++++++++++++++++++++----- >> 1 file changed, 37 insertions(+), 5 deletions(-) >> >> diff --git a/qemu-img.c b/qemu-img.c >> index e0e3d31..dcddded 100644 >> --- a/qemu-img.c >> +++ b/qemu-img.c >> @@ -314,15 +314,18 @@ static BlockBackend *img_open_opts(const char *optstr, >> } >> >> static BlockBackend *img_open_file(const char *filename, >> + QDict *options, >> const char *fmt, int flags, >> bool writethrough, bool quiet, >> bool force_share) >> { >> BlockBackend *blk; >> Error *local_err = NULL; >> - QDict *options = qdict_new(); >> >> if (fmt) { >> + if (!options) { >> + options = qdict_new(); >> + } > > This is the only place where my attempted rebase and your version > differ. I think this has to be done unconditionally, because otherwise: > > $ ./qemu-img info -U null-co:// > [1] 16327 segmentation fault (core dumped) ./qemu-img info -U null-co:// > > Also, I'm not sure the R-bs apply for this patch any longer. > > (They do for patch 1 because it's just a contextual difference. For > patch 2, it's a borderline case (I would drop it, but I can understand > keeping it). For patch 3 it's more than just borderline - I would > definitely drop the R-b, but the differences are still rather > mechanical, so it is acceptable to keep it. > But I think there are too many changes here in this patch to keep the > R-bs. In fact, I'm pretty sure none of Eric, Fam and me have given an > R-b to this segfault...) And just saw v10... Maybe I should start working on my inbox back to front... Max
On Mon, May 15, 2017 at 07:43:15PM +0200, Max Reitz wrote: > On 2017-05-15 16:04, Daniel P. Berrange wrote: > > The qemu-img dd/convert commands will create an image file and > > then try to open it. Historically it has been possible to open > > new files without passing any options. With encrypted files > > though, the *key-secret options are mandatory, so we need to > > provide those options when opening the newly created file. > > > > Reviewed-by: Max Reitz <mreitz@redhat.com> > > Reviewed-by: Fam Zheng <famz@redhat.com> > > Reviewed-by: Eric Blake <eblake@redhat.com> > > Signed-off-by: Daniel P. Berrange <berrange@redhat.com> > > --- > > qemu-img.c | 42 +++++++++++++++++++++++++++++++++++++----- > > 1 file changed, 37 insertions(+), 5 deletions(-) > > > > diff --git a/qemu-img.c b/qemu-img.c > > index e0e3d31..dcddded 100644 > > --- a/qemu-img.c > > +++ b/qemu-img.c > > @@ -314,15 +314,18 @@ static BlockBackend *img_open_opts(const char *optstr, > > } > > > > static BlockBackend *img_open_file(const char *filename, > > + QDict *options, > > const char *fmt, int flags, > > bool writethrough, bool quiet, > > bool force_share) > > { > > BlockBackend *blk; > > Error *local_err = NULL; > > - QDict *options = qdict_new(); > > > > if (fmt) { > > + if (!options) { > > + options = qdict_new(); > > + } > > This is the only place where my attempted rebase and your version > differ. I think this has to be done unconditionally, because otherwise: > > $ ./qemu-img info -U null-co:// > [1] 16327 segmentation fault (core dumped) ./qemu-img info -U null-co:// Yep, sorry this was the mistake that made me send v10. > Also, I'm not sure the R-bs apply for this patch any longer. > > (They do for patch 1 because it's just a contextual difference. For > patch 2, it's a borderline case (I would drop it, but I can understand > keeping it). For patch 3 it's more than just borderline - I would > definitely drop the R-b, but the differences are still rather > mechanical, so it is acceptable to keep it. > But I think there are too many changes here in this patch to keep the > R-bs. In fact, I'm pretty sure none of Eric, Fam and me have given an > R-b to this segfault...) True, I'm never too sure what level of changes is large enough to require dropping the R-b. Normally I just do it if it is feature changes or non-trivial review feedback, where as this was just (supposedly easy) conflict resolution, but it did go wrong this time :-( Regards, Daniel
diff --git a/qemu-img.c b/qemu-img.c index e0e3d31..dcddded 100644 --- a/qemu-img.c +++ b/qemu-img.c @@ -314,15 +314,18 @@ static BlockBackend *img_open_opts(const char *optstr, } static BlockBackend *img_open_file(const char *filename, + QDict *options, const char *fmt, int flags, bool writethrough, bool quiet, bool force_share) { BlockBackend *blk; Error *local_err = NULL; - QDict *options = qdict_new(); if (fmt) { + if (!options) { + options = qdict_new(); + } qdict_put_str(options, "driver", fmt); } @@ -344,6 +347,35 @@ static BlockBackend *img_open_file(const char *filename, } +static int img_add_key_secrets(void *opaque, + const char *name, const char *value, + Error **errp) +{ + QDict *options = opaque; + + if (g_str_has_suffix(name, "key-secret")) { + qdict_put(options, name, qstring_from_str(value)); + } + + return 0; +} + +static BlockBackend *img_open_new_file(const char *filename, + QemuOpts *create_opts, + const char *fmt, int flags, + bool writethrough, bool quiet, + bool force_share) +{ + QDict *options = NULL; + + options = qdict_new(); + qemu_opt_foreach(create_opts, img_add_key_secrets, options, &error_abort); + + return img_open_file(filename, options, fmt, flags, writethrough, quiet, + force_share); +} + + static BlockBackend *img_open(bool image_opts, const char *filename, const char *fmt, int flags, bool writethrough, @@ -364,7 +396,7 @@ static BlockBackend *img_open(bool image_opts, blk = img_open_opts(filename, opts, flags, writethrough, quiet, force_share); } else { - blk = img_open_file(filename, fmt, flags, writethrough, quiet, + blk = img_open_file(filename, NULL, fmt, flags, writethrough, quiet, force_share); } return blk; @@ -2286,8 +2318,8 @@ static int img_convert(int argc, char **argv) * That has to wait for bdrv_create to be improved * to allow filenames in option syntax */ - s.target = img_open_file(out_filename, out_fmt, flags, - writethrough, quiet, false); + s.target = img_open_new_file(out_filename, opts, out_fmt, + flags, writethrough, quiet, false); } if (!s.target) { ret = -1; @@ -4351,7 +4383,7 @@ static int img_dd(int argc, char **argv) * with the bdrv_create() call above which does not * support image-opts style. */ - blk2 = img_open_file(out.filename, out_fmt, BDRV_O_RDWR, + blk2 = img_open_file(out.filename, NULL, out_fmt, BDRV_O_RDWR, false, false, false); if (!blk2) {