Re: [PATCH 02/14] iotests.py: qemu_img*("create"): support IMGOPTS='compression_type=zstd'

2021-07-19 Thread Vladimir Sementsov-Ogievskiy 

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

Re: [PATCH 02/14] iotests.py: qemu_img*("create"): support IMGOPTS='compression_type=zstd'

2021-07-19 Thread Vladimir Sementsov-Ogievskiy 

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
---
  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.
  #
+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




+
+    resul

Re: [PATCH 02/14] iotests.py: qemu_img*("create"): support IMGOPTS='compression_type=zstd'

2021-07-16 Thread Vladimir Sementsov-Ogievskiy 

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
---
  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.
  #
+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 want

Re: [PATCH 02/14] iotests.py: qemu_img*("create"): support IMGOPTS='compression_type=zstd'

2021-07-16 Thread Max Reitz 

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
---
  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.
  #
  
+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)
+