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

[Max Reitz]

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

signature.asc
Description: OpenPGP digital signature