Adding support of IMGOPTS (like in bash tests) allows user to pass a
lot of different options. Still, some may require additional logic.

Now we want compression_type option, so add some smart logic around it:
ignore compression_type=zstd in IMGOPTS, if test want qcow2 in
compatibility mode. As well, ignore compression_type for non-qcow2
formats.

Note that we may instead add support only to qemu_img_create(), but
that works bad:

1. We'll have to update a lot of tests to use qemu_img_create instead
   of qemu_img('create'). (still, we may want do it anyway, but no
   reason to create a dependancy between task of supporting IMGOPTS and
   updating a lot of tests)

2. Some tests use qemu_img_pipe('create', ..) - even more work on
   updating

3. Even if we update all tests to go through qemu_img_create, we'll
   need a way to avoid creating new tests using qemu_img*('create') -
   add assertions.. That doesn't seem good.

So, let's add support of IMGOPTS to most generic
qemu_img_pipe_and_status().

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 tests/qemu-iotests/iotests.py | 48 ++++++++++++++++++++++++++++++++++-
 1 file changed, 47 insertions(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 0d99dd841f..80f0cb4f42 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -16,6 +16,7 @@
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 #
 
+import argparse
 import atexit
 import bz2
 from collections import OrderedDict
@@ -41,6 +42,19 @@
 from qemu.machine import qtest
 from qemu.qmp import QMPMessage
 
+
+def optstr2dict(opts: str) -> Dict[str, str]:
+    if not opts:
+        return {}
+
+    return {arr[0]: arr[1] for arr in
+            (opt.split('=', 1) for opt in opts.strip().split(','))}
+
+
+def dict2optstr(opts: Dict[str, str]) -> str:
+    return ','.join(f'{k}={v}' for k, v in opts.items())
+
+
 # Use this logger for logging messages directly from the iotests module
 logger = logging.getLogger('qemu.iotests')
 logger.addHandler(logging.NullHandler())
@@ -57,6 +71,8 @@
 if os.environ.get('QEMU_IMG_OPTIONS'):
     qemu_img_args += os.environ['QEMU_IMG_OPTIONS'].strip().split(' ')
 
+imgopts = optstr2dict(os.environ.get('IMGOPTS', ''))
+
 qemu_io_args = [os.environ.get('QEMU_IO_PROG', 'qemu-io')]
 if os.environ.get('QEMU_IO_OPTIONS'):
     qemu_io_args += os.environ['QEMU_IO_OPTIONS'].strip().split(' ')
@@ -121,11 +137,41 @@ def qemu_tool_pipe_and_status(tool: str, args: Sequence[str],
                                {-subp.returncode}: {cmd}\n')
         return (output, subp.returncode)
 
+def qemu_img_create_prepare_args(args: List[str]) -> List[str]:
+    if not args or args[0] != 'create':
+        return list(args)
+    args = args[1:]
+
+    p = argparse.ArgumentParser(allow_abbrev=False)
+    # -o option may be specified several times
+    p.add_argument('-o', action='append', default=[])
+    p.add_argument('-f')
+    parsed, remaining = p.parse_known_args(args)
+
+    opts = optstr2dict(','.join(parsed.o))
+
+    compat = 'compat' in opts and opts['compat'][0] == '0'
+    for k, v in imgopts.items():
+        if k in opts:
+            continue
+        if k == 'compression_type' and (compat or parsed.f != 'qcow2'):
+            continue
+        opts[k] = v
+
+    result = ['create']
+    if parsed.f is not None:
+        result += ['-f', parsed.f]
+    if opts:
+        result += ['-o', dict2optstr(opts)]
+    result += remaining
+
+    return result
+
 def qemu_img_pipe_and_status(*args: str) -> Tuple[str, int]:
     """
     Run qemu-img and return both its output and its exit code
     """
-    full_args = qemu_img_args + list(args)
+    full_args = qemu_img_args + qemu_img_create_prepare_args(list(args))
     return qemu_tool_pipe_and_status('qemu-img', full_args)
 
 def qemu_img(*args: str) -> int:
-- 
2.29.2

On 05.07.21 11:15, Vladimir Sementsov-Ogievskiy wrote:
> Adding support of IMGOPTS (like in bash tests) allows user to pass a
> lot of different options. Still, some may require additional logic.
>
> Now we want compression_type option, so add some smart logic around it:
> ignore compression_type=zstd in IMGOPTS, if test want qcow2 in
> compatibility mode. As well, ignore compression_type for non-qcow2
> formats.
>
> Note that we may instead add support only to qemu_img_create(), but
> that works bad:
>
> 1. We'll have to update a lot of tests to use qemu_img_create instead
>     of qemu_img('create'). (still, we may want do it anyway, but no
>     reason to create a dependancy between task of supporting IMGOPTS and
>     updating a lot of tests)
>
> 2. Some tests use qemu_img_pipe('create', ..) - even more work on
>     updating

I feel compelled to again say that we had a series that did exactly 
that.  But of course, now that so much time has passed, overhauling it 
would require quite a bit of work.

> 3. Even if we update all tests to go through qemu_img_create, we'll
>     need a way to avoid creating new tests using qemu_img*('create') -
>     add assertions.. That doesn't seem good.

That almost sounds like you remember my series, because:

https://lists.nongnu.org/archive/html/qemu-block/2019-10/msg00135.html

;)

> So, let's add support of IMGOPTS to most generic
> qemu_img_pipe_and_status().
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy<vsementsov@virtuozzo.com>
> ---
>   tests/qemu-iotests/iotests.py | 48 ++++++++++++++++++++++++++++++++++-
>   1 file changed, 47 insertions(+), 1 deletion(-)
>
> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> index 0d99dd841f..80f0cb4f42 100644
> --- a/tests/qemu-iotests/iotests.py
> +++ b/tests/qemu-iotests/iotests.py
> @@ -16,6 +16,7 @@
>   # along with this program.  If not, see<http://www.gnu.org/licenses/>.
>   #
>   
> +import argparse
>   import atexit
>   import bz2
>   from collections import OrderedDict
> @@ -41,6 +42,19 @@
>   from qemu.machine import qtest
>   from qemu.qmp import QMPMessage
>   
> +
> +def optstr2dict(opts: str) -> Dict[str, str]:
> +    if not opts:
> +        return {}
> +
> +    return {arr[0]: arr[1] for arr in
> +            (opt.split('=', 1) for opt in opts.strip().split(','))}
> +
> +
> +def dict2optstr(opts: Dict[str, str]) -> str:
> +    return ','.join(f'{k}={v}' for k, v in opts.items())
> +
> +
>   # Use this logger for logging messages directly from the iotests module
>   logger = logging.getLogger('qemu.iotests')
>   logger.addHandler(logging.NullHandler())
> @@ -57,6 +71,8 @@
>   if os.environ.get('QEMU_IMG_OPTIONS'):
>       qemu_img_args += os.environ['QEMU_IMG_OPTIONS'].strip().split(' ')
>   
> +imgopts = optstr2dict(os.environ.get('IMGOPTS', ''))
> +
>   qemu_io_args = [os.environ.get('QEMU_IO_PROG', 'qemu-io')]
>   if os.environ.get('QEMU_IO_OPTIONS'):
>       qemu_io_args += os.environ['QEMU_IO_OPTIONS'].strip().split(' ')
> @@ -121,11 +137,41 @@ def qemu_tool_pipe_and_status(tool: str, args: Sequence[str],
>                                  {-subp.returncode}: {cmd}\n')
>           return (output, subp.returncode)
>   
> +def qemu_img_create_prepare_args(args: List[str]) -> List[str]:
> +    if not args or args[0] != 'create':
> +        return list(args)
> +    args = args[1:]
> +
> +    p = argparse.ArgumentParser(allow_abbrev=False)
> +    # -o option may be specified several times
> +    p.add_argument('-o', action='append', default=[])
> +    p.add_argument('-f')
> +    parsed, remaining = p.parse_known_args(args)
> +
> +    opts = optstr2dict(','.join(parsed.o))
> +
> +    compat = 'compat' in opts and opts['compat'][0] == '0'

I suppose `opts['compat'][0] == '0'` is supposed to check for compat=0.10?

If so, then why not just check `opts['compat'] == '0.10'` to be 
clearer?  I don’t think we allow any other compat=0* values, and I have 
no reason to believe we ever will.

Also, I think compat=v2 is valid, too.  (And I think calling this 
variable “v2” would also make more sense than “compat”.)

> +    for k, v in imgopts.items():
> +        if k in opts:
> +            continue
> +        if k == 'compression_type' and (compat or parsed.f != 'qcow2'):
> +            continue
> +        opts[k] = v

Could also be done with something like

imgopts = os.environ.get('IMGOPTS')
opts = optstr2dict(','.join(([imgopts] if imgopts else []) + parsed.o))

if parsed.f != 'qcow2' or (opts.get('compat') in ['v2', '0.10']):
     opts.pop('compression_type', None)

(Never tested, of course)

Because optstr2dict() prioritizes later options over earlier ones. 
(Which is good, because that’s also qemu-img’s behavior.)

*shrug*

> +
> +    result = ['create']
> +    if parsed.f is not None:
> +        result += ['-f', parsed.f]

Can this even be None?  I hope none of our tests do this.

> +    if opts:
> +        result += ['-o', dict2optstr(opts)]
> +    result += remaining
> +
> +    return result
> +
>   def qemu_img_pipe_and_status(*args: str) -> Tuple[str, int]:
>       """
>       Run qemu-img and return both its output and its exit code
>       """
> -    full_args = qemu_img_args + list(args)
> +    full_args = qemu_img_args + qemu_img_create_prepare_args(list(args))
>       return qemu_tool_pipe_and_status('qemu-img', full_args)
>   
>   def qemu_img(*args: str) -> int:

There’s also qemu_img_verbose() which I think should be amended the same 
way (even if it’s currently never used for 'create').

Max

16.07.2021 13:07, Max Reitz wrote:
> On 05.07.21 11:15, Vladimir Sementsov-Ogievskiy wrote:
>> Adding support of IMGOPTS (like in bash tests) allows user to pass a
>> lot of different options. Still, some may require additional logic.
>>
>> Now we want compression_type option, so add some smart logic around it:
>> ignore compression_type=zstd in IMGOPTS, if test want qcow2 in
>> compatibility mode. As well, ignore compression_type for non-qcow2
>> formats.
>>
>> Note that we may instead add support only to qemu_img_create(), but
>> that works bad:
>>
>> 1. We'll have to update a lot of tests to use qemu_img_create instead
>>     of qemu_img('create'). (still, we may want do it anyway, but no
>>     reason to create a dependancy between task of supporting IMGOPTS and
>>     updating a lot of tests)
>>
>> 2. Some tests use qemu_img_pipe('create', ..) - even more work on
>>     updating
> 
> I feel compelled to again say that we had a series that did exactly that.  But of course, now that so much time has passed, overhauling it would require quite a bit of work.
> 
>> 3. Even if we update all tests to go through qemu_img_create, we'll
>>     need a way to avoid creating new tests using qemu_img*('create') -
>>     add assertions.. That doesn't seem good.
> 
> That almost sounds like you remember my series, because:
> 
> https://lists.nongnu.org/archive/html/qemu-block/2019-10/msg00135.html
> 
> ;)

:) No, I don't remember it.

> 
>> So, let's add support of IMGOPTS to most generic
>> qemu_img_pipe_and_status().
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy<vsementsov@virtuozzo.com>
>> ---
>>   tests/qemu-iotests/iotests.py | 48 ++++++++++++++++++++++++++++++++++-
>>   1 file changed, 47 insertions(+), 1 deletion(-)
>>
>> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
>> index 0d99dd841f..80f0cb4f42 100644
>> --- a/tests/qemu-iotests/iotests.py
>> +++ b/tests/qemu-iotests/iotests.py
>> @@ -16,6 +16,7 @@
>>   # along with this program.  If not, see<http://www.gnu.org/licenses/>.
>>   #
>> +import argparse
>>   import atexit
>>   import bz2
>>   from collections import OrderedDict
>> @@ -41,6 +42,19 @@
>>   from qemu.machine import qtest
>>   from qemu.qmp import QMPMessage
>> +
>> +def optstr2dict(opts: str) -> Dict[str, str]:
>> +    if not opts:
>> +        return {}
>> +
>> +    return {arr[0]: arr[1] for arr in
>> +            (opt.split('=', 1) for opt in opts.strip().split(','))}
>> +
>> +
>> +def dict2optstr(opts: Dict[str, str]) -> str:
>> +    return ','.join(f'{k}={v}' for k, v in opts.items())
>> +
>> +
>>   # Use this logger for logging messages directly from the iotests module
>>   logger = logging.getLogger('qemu.iotests')
>>   logger.addHandler(logging.NullHandler())
>> @@ -57,6 +71,8 @@
>>   if os.environ.get('QEMU_IMG_OPTIONS'):
>>       qemu_img_args += os.environ['QEMU_IMG_OPTIONS'].strip().split(' ')
>> +imgopts = optstr2dict(os.environ.get('IMGOPTS', ''))
>> +
>>   qemu_io_args = [os.environ.get('QEMU_IO_PROG', 'qemu-io')]
>>   if os.environ.get('QEMU_IO_OPTIONS'):
>>       qemu_io_args += os.environ['QEMU_IO_OPTIONS'].strip().split(' ')
>> @@ -121,11 +137,41 @@ def qemu_tool_pipe_and_status(tool: str, args: Sequence[str],
>>                                  {-subp.returncode}: {cmd}\n')
>>           return (output, subp.returncode)
>> +def qemu_img_create_prepare_args(args: List[str]) -> List[str]:
>> +    if not args or args[0] != 'create':
>> +        return list(args)
>> +    args = args[1:]
>> +
>> +    p = argparse.ArgumentParser(allow_abbrev=False)
>> +    # -o option may be specified several times
>> +    p.add_argument('-o', action='append', default=[])
>> +    p.add_argument('-f')
>> +    parsed, remaining = p.parse_known_args(args)
>> +
>> +    opts = optstr2dict(','.join(parsed.o))
>> +
>> +    compat = 'compat' in opts and opts['compat'][0] == '0'
> 
> I suppose `opts['compat'][0] == '0'` is supposed to check for compat=0.10?
> 
> If so, then why not just check `opts['compat'] == '0.10'` to be clearer?  I don’t think we allow any other compat=0* values, and I have no reason to believe we ever will.
> 
> Also, I think compat=v2 is valid, too.  (And I think calling this variable “v2” would also make more sense than “compat”.)

Good for me, will do.

> 
>> +    for k, v in imgopts.items():
>> +        if k in opts:
>> +            continue
>> +        if k == 'compression_type' and (compat or parsed.f != 'qcow2'):
>> +            continue
>> +        opts[k] = v
> 
> Could also be done with something like
> 
> imgopts = os.environ.get('IMGOPTS')

imgopts is a string after it. So you don't need to join it?

> opts = optstr2dict(','.join(([imgopts] if imgopts else []) + parsed.o))

Build a string to be than parsed looks strange IMHO..

> 
> if parsed.f != 'qcow2' or (opts.get('compat') in ['v2', '0.10']):
>      opts.pop('compression_type', None)
> 
> (Never tested, of course)
> 
> Because optstr2dict() prioritizes later options over earlier ones. (Which is good, because that’s also qemu-img’s behavior.)
> 

Ok, I'll think about this all when prepare v2, and we'll see how it goes

> 
>> +
>> +    result = ['create']
>> +    if parsed.f is not None:
>> +        result += ['-f', parsed.f]
> 
> Can this even be None?  I hope none of our tests do this.

I'm afraid they do, as I first wanted to make a default to be imgfmt, but faced tests that rely on default being raw.. May be I'm wrong. Will check it.

> 
>> +    if opts:
>> +        result += ['-o', dict2optstr(opts)]
>> +    result += remaining
>> +
>> +    return result
>> +
>>   def qemu_img_pipe_and_status(*args: str) -> Tuple[str, int]:
>>       """
>>       Run qemu-img and return both its output and its exit code
>>       """
>> -    full_args = qemu_img_args + list(args)
>> +    full_args = qemu_img_args + qemu_img_create_prepare_args(list(args))
>>       return qemu_tool_pipe_and_status('qemu-img', full_args)
>>   def qemu_img(*args: str) -> int:
> 
> There’s also qemu_img_verbose() which I think should be amended the same way (even if it’s currently never used for 'create').
> 

Right, thanks. I think better is rewrite qemu_img_verbose to to call qemu_img_pipe_and_status.

Another thing to do is moving handling luks from qemu_img_create to qemu_img_create_prepare_args, so all these things be in one place.


-- 
Best regards,
Vladimir

16.07.2021 13:34, Vladimir Sementsov-Ogievskiy wrote:
> 16.07.2021 13:07, Max Reitz wrote:
>> On 05.07.21 11:15, Vladimir Sementsov-Ogievskiy wrote:
>>> Adding support of IMGOPTS (like in bash tests) allows user to pass a
>>> lot of different options. Still, some may require additional logic.
>>>
>>> Now we want compression_type option, so add some smart logic around it:
>>> ignore compression_type=zstd in IMGOPTS, if test want qcow2 in
>>> compatibility mode. As well, ignore compression_type for non-qcow2
>>> formats.
>>>
>>> Note that we may instead add support only to qemu_img_create(), but
>>> that works bad:
>>>
>>> 1. We'll have to update a lot of tests to use qemu_img_create instead
>>>     of qemu_img('create'). (still, we may want do it anyway, but no
>>>     reason to create a dependancy between task of supporting IMGOPTS and
>>>     updating a lot of tests)
>>>
>>> 2. Some tests use qemu_img_pipe('create', ..) - even more work on
>>>     updating
>>
>> I feel compelled to again say that we had a series that did exactly that.  But of course, now that so much time has passed, overhauling it would require quite a bit of work.
>>
>>> 3. Even if we update all tests to go through qemu_img_create, we'll
>>>     need a way to avoid creating new tests using qemu_img*('create') -
>>>     add assertions.. That doesn't seem good.
>>
>> That almost sounds like you remember my series, because:
>>
>> https://lists.nongnu.org/archive/html/qemu-block/2019-10/msg00135.html
>>
>> ;)
> 
> :) No, I don't remember it.
> 
>>
>>> So, let's add support of IMGOPTS to most generic
>>> qemu_img_pipe_and_status().
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy<vsementsov@virtuozzo.com>
>>> ---
>>>   tests/qemu-iotests/iotests.py | 48 ++++++++++++++++++++++++++++++++++-
>>>   1 file changed, 47 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
>>> index 0d99dd841f..80f0cb4f42 100644
>>> --- a/tests/qemu-iotests/iotests.py
>>> +++ b/tests/qemu-iotests/iotests.py
>>> @@ -16,6 +16,7 @@
>>>   # along with this program.  If not, see<http://www.gnu.org/licenses/>.
>>>   #
>>> +import argparse
>>>   import atexit
>>>   import bz2
>>>   from collections import OrderedDict
>>> @@ -41,6 +42,19 @@
>>>   from qemu.machine import qtest
>>>   from qemu.qmp import QMPMessage
>>> +
>>> +def optstr2dict(opts: str) -> Dict[str, str]:
>>> +    if not opts:
>>> +        return {}
>>> +
>>> +    return {arr[0]: arr[1] for arr in
>>> +            (opt.split('=', 1) for opt in opts.strip().split(','))}
>>> +
>>> +
>>> +def dict2optstr(opts: Dict[str, str]) -> str:
>>> +    return ','.join(f'{k}={v}' for k, v in opts.items())
>>> +
>>> +
>>>   # Use this logger for logging messages directly from the iotests module
>>>   logger = logging.getLogger('qemu.iotests')
>>>   logger.addHandler(logging.NullHandler())
>>> @@ -57,6 +71,8 @@
>>>   if os.environ.get('QEMU_IMG_OPTIONS'):
>>>       qemu_img_args += os.environ['QEMU_IMG_OPTIONS'].strip().split(' ')
>>> +imgopts = optstr2dict(os.environ.get('IMGOPTS', ''))
>>> +
>>>   qemu_io_args = [os.environ.get('QEMU_IO_PROG', 'qemu-io')]
>>>   if os.environ.get('QEMU_IO_OPTIONS'):
>>>       qemu_io_args += os.environ['QEMU_IO_OPTIONS'].strip().split(' ')
>>> @@ -121,11 +137,41 @@ def qemu_tool_pipe_and_status(tool: str, args: Sequence[str],
>>>                                  {-subp.returncode}: {cmd}\n')
>>>           return (output, subp.returncode)
>>> +def qemu_img_create_prepare_args(args: List[str]) -> List[str]:
>>> +    if not args or args[0] != 'create':
>>> +        return list(args)
>>> +    args = args[1:]
>>> +
>>> +    p = argparse.ArgumentParser(allow_abbrev=False)
>>> +    # -o option may be specified several times
>>> +    p.add_argument('-o', action='append', default=[])
>>> +    p.add_argument('-f')
>>> +    parsed, remaining = p.parse_known_args(args)
>>> +
>>> +    opts = optstr2dict(','.join(parsed.o))
>>> +
>>> +    compat = 'compat' in opts and opts['compat'][0] == '0'
>>
>> I suppose `opts['compat'][0] == '0'` is supposed to check for compat=0.10?
>>
>> If so, then why not just check `opts['compat'] == '0.10'` to be clearer?  I don’t think we allow any other compat=0* values, and I have no reason to believe we ever will.
>>
>> Also, I think compat=v2 is valid, too.  (And I think calling this variable “v2” would also make more sense than “compat”.)
> 
> Good for me, will do.
> 
>>
>>> +    for k, v in imgopts.items():
>>> +        if k in opts:
>>> +            continue
>>> +        if k == 'compression_type' and (compat or parsed.f != 'qcow2'):
>>> +            continue
>>> +        opts[k] = v
>>
>> Could also be done with something like
>>
>> imgopts = os.environ.get('IMGOPTS')
> 
> imgopts is a string after it. So you don't need to join it?
> 
>> opts = optstr2dict(','.join(([imgopts] if imgopts else []) + parsed.o))
> 
> Build a string to be than parsed looks strange IMHO..

Oh, but that's exactly what I should do anyway to cover several -o options. Now I see that what you write is correct.

> 
>>
>> if parsed.f != 'qcow2' or (opts.get('compat') in ['v2', '0.10']):
>>      opts.pop('compression_type', None)
>>
>> (Never tested, of course)
>>
>> Because optstr2dict() prioritizes later options over earlier ones. (Which is good, because that’s also qemu-img’s behavior.)
>>
> 
> Ok, I'll think about this all when prepare v2, and we'll see how it goes
> 
>>
>>> +
>>> +    result = ['create']
>>> +    if parsed.f is not None:
>>> +        result += ['-f', parsed.f]
>>
>> Can this even be None?  I hope none of our tests do this.
> 
> I'm afraid they do, as I first wanted to make a default to be imgfmt, but faced tests that rely on default being raw.. May be I'm wrong. Will check it.
> 
>>
>>> +    if opts:
>>> +        result += ['-o', dict2optstr(opts)]
>>> +    result += remaining
>>> +
>>> +    return result
>>> +
>>>   def qemu_img_pipe_and_status(*args: str) -> Tuple[str, int]:
>>>       """
>>>       Run qemu-img and return both its output and its exit code
>>>       """
>>> -    full_args = qemu_img_args + list(args)
>>> +    full_args = qemu_img_args + qemu_img_create_prepare_args(list(args))
>>>       return qemu_tool_pipe_and_status('qemu-img', full_args)
>>>   def qemu_img(*args: str) -> int:
>>
>> There’s also qemu_img_verbose() which I think should be amended the same way (even if it’s currently never used for 'create').
>>
> 
> Right, thanks. I think better is rewrite qemu_img_verbose to to call qemu_img_pipe_and_status.
> 
> Another thing to do is moving handling luks from qemu_img_create to qemu_img_create_prepare_args, so all these things be in one place.
> 
> 


-- 
Best regards,
Vladimir

19.07.2021 15:58, Vladimir Sementsov-Ogievskiy wrote:
>>>>
>>>
>>> Could also be done with something like
>>>
>>> imgopts = os.environ.get('IMGOPTS')
>>
>> imgopts is a string after it. So you don't need to join it?
>>
>>> opts = optstr2dict(','.join(([imgopts] if imgopts else []) + parsed.o))
>>
>> Build a string to be than parsed looks strange IMHO..
> 
> Oh, but that's exactly what I should do anyway to cover several -o options. Now I see that what you write is correct.
> 
>>
>>>
>>> if parsed.f != 'qcow2' or (opts.get('compat') in ['v2', '0.10']):
>>>      opts.pop('compression_type', None)
>>>
>>> (Never tested, of course)
>>>
>>> Because optstr2dict() prioritizes later options over earlier ones. (Which is good, because that’s also qemu-img’s behavior.)
>>>
>>
>> Ok, I'll think about this all when prepare v2, and we'll see how it goes 

This way you also drop compression_type if test specify it. I think we shouldn't touch test specified options. Let it clearly fail instead.

We only want to ignore compression_type in IMGOPTS when create non-qcow2 image. I think I'll drop checking for compat: the only user for this check ic 065 and it's simpler to explicitly set compression_type in it even for compat=0.10 cases.

-- 
Best regards,
Vladimir