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 <mre...@redhat.com>
> > Reviewed-by: Fam Zheng <f...@redhat.com>
> > Reviewed-by: Eric Blake <ebl...@redhat.com>
> > Signed-off-by: Daniel P. Berrange <berra...@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
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
