Re: [Qemu-devel] [PATCH v9 4/4] qemu-img: copy *key-secret opts when opening newly created files

[Daniel P. Berrange]

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 <address@hidden>
> > Reviewed-by: Fam Zheng <address@hidden>
> > Reviewed-by: Eric Blake <address@hidden>
> > Signed-off-by: Daniel P. Berrange <address@hidden>
> > ---
> >  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 :|