[v2,1/3] qemu-img/convert: Use @opts for one thing only
diff mbox

Message ID 20170426134649.9042-2-mreitz@redhat.com
State New
Headers show

Commit Message

Max Reitz April 26, 2017, 1:46 p.m. UTC
After storing the creation options for the new image into @opts, we
fetch some things for our own information, like the backing file name,
or whether to use encryption or preallocation.

With the -n parameter, there will not be any creation options; this is
not too bad because this just means that querying a NULL @opts will
always return the default value.

However, we also use @opts for the --object options. Therefore, @opts is
not necessarily NULL if -n was specified; instead, it may contain those
options. In practice, this probably does not cause any problems because
there most likely is no object that supports any of the parameters we
query here, but this is neither something we should rely on nor does
this variable reuse make the code very nice to read.

Therefore, just use an own variable for the --object options.

Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 qemu-img.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

Comments

Eric Blake April 26, 2017, 2:03 p.m. UTC | #1
On 04/26/2017 08:46 AM, Max Reitz wrote:
> After storing the creation options for the new image into @opts, we
> fetch some things for our own information, like the backing file name,
> or whether to use encryption or preallocation.
> 
> With the -n parameter, there will not be any creation options; this is
> not too bad because this just means that querying a NULL @opts will
> always return the default value.
> 
> However, we also use @opts for the --object options. Therefore, @opts is
> not necessarily NULL if -n was specified; instead, it may contain those
> options. In practice, this probably does not cause any problems because
> there most likely is no object that supports any of the parameters we
> query here, but this is neither something we should rely on nor does
> this variable reuse make the code very nice to read.
> 
> Therefore, just use an own variable for the --object options.

Reads better with s/an own/a separate/

> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
>  qemu-img.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 

R-b still stands.
Max Reitz April 26, 2017, 2:15 p.m. UTC | #2
On 26.04.2017 16:03, Eric Blake wrote:
> On 04/26/2017 08:46 AM, Max Reitz wrote:
>> After storing the creation options for the new image into @opts, we
>> fetch some things for our own information, like the backing file name,
>> or whether to use encryption or preallocation.
>>
>> With the -n parameter, there will not be any creation options; this is
>> not too bad because this just means that querying a NULL @opts will
>> always return the default value.
>>
>> However, we also use @opts for the --object options. Therefore, @opts is
>> not necessarily NULL if -n was specified; instead, it may contain those
>> options. In practice, this probably does not cause any problems because
>> there most likely is no object that supports any of the parameters we
>> query here, but this is neither something we should rely on nor does
>> this variable reuse make the code very nice to read.
>>
>> Therefore, just use an own variable for the --object options.
> 
> Reads better with s/an own/a separate/

I certainly wouldn't mind if someone (*cough*) were to update this while
applying. O:-)

Max

>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> Reviewed-by: Eric Blake <eblake@redhat.com>
>> ---
>>  qemu-img.c | 10 ++++++----
>>  1 file changed, 6 insertions(+), 4 deletions(-)
>>
> 
> R-b still stands.

Patch
diff mbox

diff --git a/qemu-img.c b/qemu-img.c
index b2c4dc9613..ae6f27f899 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -2049,13 +2049,15 @@  static int img_convert(int argc, char **argv)
         case 'W':
             s.wr_in_order = false;
             break;
-        case OPTION_OBJECT:
-            opts = qemu_opts_parse_noisily(&qemu_object_opts,
-                                           optarg, true);
-            if (!opts) {
+        case OPTION_OBJECT: {
+            QemuOpts *object_opts;
+            object_opts = qemu_opts_parse_noisily(&qemu_object_opts,
+                                                  optarg, true);
+            if (!object_opts) {
                 goto fail_getopt;
             }
             break;
+        }
         case OPTION_IMAGE_OPTS:
             image_opts = true;
             break;