[PATCH v5 14/16] python/qemu: make 'args' style arguments immutable

[John Snow]

These arguments don't need to be mutable and aren't really used as
such. Clarify their types as immutable and adjust code to match where
necessary.

In general, It's probably best not to accept a user-defined mutable
object and store it as internal object state unless there's a strong
justification for doing so. Instead, try to use generic types as input
with empty tuples as the default, and coerce to list where necessary.

Signed-off-by: John Snow 
Reviewed-by: Kevin Wolf 
---
 python/qemu/machine.py | 30 +-
 python/qemu/qtest.py   | 16 
 2 files changed, 29 insertions(+), 17 deletions(-)

diff --git a/python/qemu/machine.py b/python/qemu/machine.py
index 2ba5e3134e..f7239a249b 100644
--- a/python/qemu/machine.py
+++ b/python/qemu/machine.py
@@ -18,6 +18,7 @@
 #
 
 import errno
+from itertools import chain
 import logging
 import os
 import subprocess
@@ -30,6 +31,8 @@
 Dict,
 List,
 Optional,
+Sequence,
+Tuple,
 Type,
 )
 from types import TracebackType
@@ -74,8 +77,12 @@ class QEMUMachine:
 # vm is guaranteed to be shut down here
 """
 
-def __init__(self, binary, args=None, wrapper=None, name=None,
- test_dir="/var/tmp",
+def __init__(self,
+ binary: str,
+ args: Sequence[str] = (),
+ wrapper: Sequence[str] = (),
+ name: Optional[str] = None,
+ test_dir: str = "/var/tmp",
  monitor_address: Optional[SocketAddrT] = None,
  socket_scm_helper=None, sock_dir=None):
 '''
@@ -93,14 +100,7 @@ def __init__(self, binary, args=None, wrapper=None, 
name=None,
 # Direct user configuration
 
 self._binary = binary
-
-if args is None:
-args = []
-# Copy mutable input: we will be modifying our copy
 self._args = list(args)
-
-if wrapper is None:
-wrapper = []
 self._wrapper = wrapper
 
 self._name = name or "qemu-%d" % os.getpid()
@@ -125,7 +125,7 @@ def __init__(self, binary, args=None, wrapper=None, 
name=None,
 self._iolog = None
 self._qmp_set = True   # Enable QMP monitor by default.
 self._qmp_connection: Optional[qmp.QEMUMonitorProtocol] = None
-self._qemu_full_args = None
+self._qemu_full_args: Tuple[str, ...] = ()
 self._temp_dir = None
 self._launched = False
 self._machine = None
@@ -357,7 +357,7 @@ def launch(self):
 raise QEMUMachineError('VM already launched')
 
 self._iolog = None
-self._qemu_full_args = None
+self._qemu_full_args = ()
 try:
 self._launch()
 self._launched = True
@@ -377,8 +377,12 @@ def _launch(self):
 """
 devnull = open(os.path.devnull, 'rb')
 self._pre_launch()
-self._qemu_full_args = (self._wrapper + [self._binary] +
-self._base_args + self._args)
+self._qemu_full_args = tuple(
+chain(self._wrapper,
+  [self._binary],
+  self._base_args,
+  self._args)
+)
 LOG.debug('VM launch command: %r', ' '.join(self._qemu_full_args))
 self._popen = subprocess.Popen(self._qemu_full_args,
stdin=devnull,
diff --git a/python/qemu/qtest.py b/python/qemu/qtest.py
index 05c63a1d58..ae4661d4d3 100644
--- a/python/qemu/qtest.py
+++ b/python/qemu/qtest.py
@@ -22,6 +22,7 @@
 from typing import (
 List,
 Optional,
+Sequence,
 TextIO,
 )
 
@@ -103,8 +104,13 @@ class QEMUQtestMachine(QEMUMachine):
 A QEMU VM, with a qtest socket available.
 """
 
-def __init__(self, binary, args=None, name=None, test_dir="/var/tmp",
- socket_scm_helper=None, sock_dir=None):
+def __init__(self,
+ binary: str,
+ args: Sequence[str] = (),
+ name: Optional[str] = None,
+ test_dir: str = "/var/tmp",
+ socket_scm_helper: Optional[str] = None,
+ sock_dir: Optional[str] = None):
 if name is None:
 name = "qemu-%d" % os.getpid()
 if sock_dir is None:
@@ -118,8 +124,10 @@ def __init__(self, binary, args=None, name=None, 
test_dir="/var/tmp",
 @property
 def _base_args(self) -> List[str]:
 args = super()._base_args
-args.extend(['-qtest', 'unix:path=' + self._qtest_path,
- '-accel', 'qtest'])
+args.extend([
+'-qtest', f"unix:path={self._qtest_path}",
+'-accel', 'qtest'
+])
 return args
 
 def _pre_launch(self):
-- 
2.21.3

[PATCH v5 15/16] iotests.py: Adjust HMP kwargs typing

[John Snow]

mypy wants to ensure there's consistency between the kwargs arguments
types and any unspecified keyword arguments. In this case, conv_keys is
a bool, but the remaining keys are Any type. Mypy (correctly) infers the
**kwargs type to be **Dict[str, str], which is not compatible with
conv_keys: bool.

Because QMP typing is a little fraught right now anyway, re-type kwargs
to Dict[str, Any] which has the benefit of silencing this check right
now.

A future re-design might type these more aggressively, but this will
give us a baseline to work from with minimal disruption.

(Thanks Kevin Wolf for the debugging assist here)

Signed-off-by: John Snow 
Reviewed-by: Kevin Wolf 
---
 tests/qemu-iotests/iotests.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 4457786f09..d5cde7c0da 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -553,7 +553,7 @@ def add_incoming(self, addr):
 
 def hmp(self, command_line: str, use_log: bool = False) -> QMPMessage:
 cmd = 'human-monitor-command'
-kwargs = {'command-line': command_line}
+kwargs: Dict[str, Any] = {'command-line': command_line}
 if use_log:
 return self.qmp_log(cmd, **kwargs)
 else:
-- 
2.21.3

[PATCH v5 16/16] python/qemu: Add mypy type annotations

[John Snow]

These should all be purely annotations with no changes in behavior at
all. You need to be in the python folder, but you should be able to
confirm that these annotations are correct (or at least self-consistent)
by running `mypy --strict qemu`.

Signed-off-by: John Snow 
---
 python/qemu/accel.py   |  8 ++--
 python/qemu/machine.py | 94 --
 python/qemu/qmp.py | 44 +++-
 python/qemu/qtest.py   | 26 +++-
 4 files changed, 98 insertions(+), 74 deletions(-)

diff --git a/python/qemu/accel.py b/python/qemu/accel.py
index 7fabe62920..4325114e51 100644
--- a/python/qemu/accel.py
+++ b/python/qemu/accel.py
@@ -17,6 +17,7 @@
 import logging
 import os
 import subprocess
+from typing import List, Optional
 
 LOG = logging.getLogger(__name__)
 
@@ -29,7 +30,7 @@
 }
 
 
-def list_accel(qemu_bin):
+def list_accel(qemu_bin: str) -> List[str]:
 """
 List accelerators enabled in the QEMU binary.
 
@@ -49,7 +50,8 @@ def list_accel(qemu_bin):
 return [acc.strip() for acc in out.splitlines()[1:]]
 
 
-def kvm_available(target_arch=None, qemu_bin=None):
+def kvm_available(target_arch: Optional[str] = None,
+  qemu_bin: Optional[str] = None) -> bool:
 """
 Check if KVM is available using the following heuristic:
   - Kernel module is present in the host;
@@ -72,7 +74,7 @@ def kvm_available(target_arch=None, qemu_bin=None):
 return True
 
 
-def tcg_available(qemu_bin):
+def tcg_available(qemu_bin: str) -> bool:
 """
 Check if TCG is available.
 
diff --git a/python/qemu/machine.py b/python/qemu/machine.py
index f7239a249b..3f0c98987f 100644
--- a/python/qemu/machine.py
+++ b/python/qemu/machine.py
@@ -28,6 +28,7 @@
 import tempfile
 from typing import (
 Any,
+BinaryIO,
 Dict,
 List,
 Optional,
@@ -38,7 +39,7 @@
 from types import TracebackType
 
 from . import qmp
-from .qmp import SocketAddrT, QMPMessage
+from .qmp import QMPMessage, QMPReturnValue, SocketAddrT
 
 LOG = logging.getLogger(__name__)
 
@@ -67,7 +68,7 @@ class AbnormalShutdown(QEMUMachineError):
 
 class QEMUMachine:
 """
-A QEMU VM
+A QEMU VM.
 
 Use this object as a context manager to ensure
 the QEMU process terminates::
@@ -84,7 +85,8 @@ def __init__(self,
  name: Optional[str] = None,
  test_dir: str = "/var/tmp",
  monitor_address: Optional[SocketAddrT] = None,
- socket_scm_helper=None, sock_dir=None):
+ socket_scm_helper: Optional[str] = None,
+ sock_dir: Optional[str] = None):
 '''
 Initialize a QEMUMachine
 
@@ -118,28 +120,28 @@ def __init__(self,
 self._remove_monitor_sockfile = True
 
 # Runstate
-self._qemu_log_path = None
-self._qemu_log_file = None
+self._qemu_log_path: Optional[str] = None
+self._qemu_log_file: Optional[BinaryIO] = None
 self._popen: Optional['subprocess.Popen[bytes]'] = None
-self._events = []
-self._iolog = None
+self._events: List[QMPMessage] = []
+self._iolog: Optional[str] = None
 self._qmp_set = True   # Enable QMP monitor by default.
 self._qmp_connection: Optional[qmp.QEMUMonitorProtocol] = None
 self._qemu_full_args: Tuple[str, ...] = ()
-self._temp_dir = None
+self._temp_dir: Optional[str] = None
 self._launched = False
-self._machine = None
+self._machine: Optional[str] = None
 self._console_index = 0
 self._console_set = False
-self._console_device_type = None
+self._console_device_type: Optional[str] = None
 self._console_address = os.path.join(
 self._sock_dir, f"{self._name}-console.sock"
 )
-self._console_socket = None
-self._remove_files = []
+self._console_socket: Optional[socket.socket] = None
+self._remove_files: List[str] = []
 self._user_killed = False
 
-def __enter__(self):
+def __enter__(self) -> 'QEMUMachine':
 return self
 
 def __exit__(self,
@@ -148,14 +150,15 @@ def __exit__(self,
  exc_tb: Optional[TracebackType]) -> None:
 self.shutdown()
 
-def add_monitor_null(self):
+def add_monitor_null(self) -> None:
 """
 This can be used to add an unused monitor instance.
 """
 self._args.append('-monitor')
 self._args.append('null')
 
-def add_fd(self, fd, fdset, opaque, opts=''):
+def add_fd(self, fd: int, fdset: int,
+   opaque: str, opts: str = '') -> 'QEMUMachine':
 """
 Pass a file descriptor to the VM
 """
@@ -174,7 +177,8 @@ def add_fd(self, fd, fdset, opaque, opts=''):
 self._args.append(','.join(options))
 return self
 
-def send_fd_scm(self, fd=None, file_path=None):
+def send_fd_scm(self, fd: Optional[int] = None,
+   

[PATCH v5 12/16] python/machine.py: Add _qmp access shim

[John Snow]

Like many other Optional[] types, it's not always a given that this
object will be set. Wrap it in a type-shim that raises a meaningful
error and will always return a concrete type.

Signed-off-by: John Snow 
---
 python/qemu/machine.py | 24 +---
 1 file changed, 13 insertions(+), 11 deletions(-)

diff --git a/python/qemu/machine.py b/python/qemu/machine.py
index 0df94c3211..9760a2badb 100644
--- a/python/qemu/machine.py
+++ b/python/qemu/machine.py
@@ -124,7 +124,7 @@ def __init__(self, binary, args=None, wrapper=None, 
name=None,
 self._events = []
 self._iolog = None
 self._qmp_set = True   # Enable QMP monitor by default.
-self._qmp = None
+self._qmp_connection: Optional[qmp.QEMUMonitorProtocol] = None
 self._qemu_full_args = None
 self._temp_dir = None
 self._launched = False
@@ -291,14 +291,14 @@ def _pre_launch(self):
 if self._remove_monitor_sockfile:
 assert isinstance(self._monitor_address, str)
 self._remove_files.append(self._monitor_address)
-self._qmp = qmp.QEMUMonitorProtocol(
+self._qmp_connection = qmp.QEMUMonitorProtocol(
 self._monitor_address,
 server=True,
 nickname=self._name
 )
 
 def _post_launch(self):
-if self._qmp:
+if self._qmp_connection:
 self._qmp.accept()
 
 def _post_shutdown(self):
@@ -309,9 +309,9 @@ def _post_shutdown(self):
 # Comprehensive reset for the failed launch case:
 self._early_cleanup()
 
-if self._qmp:
+if self._qmp_connection:
 self._qmp.close()
-self._qmp = None
+self._qmp_connection = None
 
 self._load_io_log()
 
@@ -423,7 +423,7 @@ def _soft_shutdown(self, has_quit: bool = False,
 """
 self._early_cleanup()
 
-if self._qmp is not None:
+if self._qmp_connection:
 if not has_quit:
 # Might raise ConnectionReset
 self._qmp.cmd('quit')
@@ -501,11 +501,13 @@ def set_qmp_monitor(self, enabled=True):
 line. Default is True.
 @note: call this function before launch().
 """
-if enabled:
-self._qmp_set = True
-else:
-self._qmp_set = False
-self._qmp = None
+self._qmp_set = enabled
+
+@property
+def _qmp(self) -> qmp.QEMUMonitorProtocol:
+if self._qmp_connection is None:
+raise QEMUMachineError("Attempt to access QMP with no connection")
+return self._qmp_connection
 
 @classmethod
 def _qmp_args(cls, _conv_keys: bool = True, **args: Any) -> Dict[str, Any]:
-- 
2.21.3

[PATCH v5 09/16] python/machine.py: Don't modify state in _base_args()

[John Snow]

Don't append to the _remove_files list during _base_args; instead do so
during _launch. Rework _base_args as a @property to help facilitate
this impression.

This has the additional benefit of making the type of _console_address
easier to analyze statically.

Signed-off-by: John Snow 
Reviewed-by: Kevin Wolf 
---
 python/qemu/machine.py | 16 ++--
 python/qemu/qtest.py   | 11 ---
 2 files changed, 18 insertions(+), 9 deletions(-)

diff --git a/python/qemu/machine.py b/python/qemu/machine.py
index 0f7ec095cb..37e859f6d2 100644
--- a/python/qemu/machine.py
+++ b/python/qemu/machine.py
@@ -26,6 +26,7 @@
 import socket
 import tempfile
 from typing import (
+List,
 Optional,
 Type,
 )
@@ -129,7 +130,9 @@ def __init__(self, binary, args=None, wrapper=None, 
name=None,
 self._console_index = 0
 self._console_set = False
 self._console_device_type = None
-self._console_address = None
+self._console_address = os.path.join(
+self._sock_dir, f"{self._name}-console.sock"
+)
 self._console_socket = None
 self._remove_files = []
 self._user_killed = False
@@ -245,7 +248,8 @@ def _load_io_log(self):
 with open(self._qemu_log_path, "r") as iolog:
 self._iolog = iolog.read()
 
-def _base_args(self):
+@property
+def _base_args(self) -> List[str]:
 args = ['-display', 'none', '-vga', 'none']
 
 if self._qmp_set:
@@ -263,9 +267,6 @@ def _base_args(self):
 for _ in range(self._console_index):
 args.extend(['-serial', 'null'])
 if self._console_set:
-self._console_address = os.path.join(self._sock_dir,
- self._name + "-console.sock")
-self._remove_files.append(self._console_address)
 chardev = ('socket,id=console,path=%s,server,nowait' %
self._console_address)
 args.extend(['-chardev', chardev])
@@ -281,6 +282,9 @@ def _pre_launch(self):
 self._qemu_log_path = os.path.join(self._temp_dir, self._name + ".log")
 self._qemu_log_file = open(self._qemu_log_path, 'wb')
 
+if self._console_set:
+self._remove_files.append(self._console_address)
+
 if self._qmp_set:
 if self._remove_monitor_sockfile:
 assert isinstance(self._monitor_address, str)
@@ -366,7 +370,7 @@ def _launch(self):
 devnull = open(os.path.devnull, 'rb')
 self._pre_launch()
 self._qemu_full_args = (self._wrapper + [self._binary] +
-self._base_args() + self._args)
+self._base_args + self._args)
 LOG.debug('VM launch command: %r', ' '.join(self._qemu_full_args))
 self._popen = subprocess.Popen(self._qemu_full_args,
stdin=devnull,
diff --git a/python/qemu/qtest.py b/python/qemu/qtest.py
index 888c8bd2f6..05c63a1d58 100644
--- a/python/qemu/qtest.py
+++ b/python/qemu/qtest.py
@@ -19,7 +19,11 @@
 
 import socket
 import os
-from typing import Optional, TextIO
+from typing import (
+List,
+Optional,
+TextIO,
+)
 
 from .machine import QEMUMachine
 
@@ -111,8 +115,9 @@ def __init__(self, binary, args=None, name=None, 
test_dir="/var/tmp",
 self._qtest = None
 self._qtest_path = os.path.join(sock_dir, name + "-qtest.sock")
 
-def _base_args(self):
-args = super()._base_args()
+@property
+def _base_args(self) -> List[str]:
+args = super()._base_args
 args.extend(['-qtest', 'unix:path=' + self._qtest_path,
  '-accel', 'qtest'])
 return args
-- 
2.21.3

[PATCH v5 08/16] python/machine.py: reorder __init__

[John Snow]

Put the init arg handling all at the top, and mostly in order (deviating
when one is dependent on another), and put what is effectively runtime
state declaration at the bottom.

Signed-off-by: John Snow 
Reviewed-by: Philippe Mathieu-Daudé 
Reviewed-by: Kevin Wolf 
---
 python/qemu/machine.py | 29 +
 1 file changed, 17 insertions(+), 12 deletions(-)

diff --git a/python/qemu/machine.py b/python/qemu/machine.py
index bab1b1921f..0f7ec095cb 100644
--- a/python/qemu/machine.py
+++ b/python/qemu/machine.py
@@ -87,38 +87,43 @@ def __init__(self, binary, args=None, wrapper=None, 
name=None,
 @param socket_scm_helper: helper program, required for send_fd_scm()
 @note: Qemu process is not started until launch() is used.
 '''
+# Direct user configuration
+
+self._binary = binary
+
 if args is None:
 args = []
+# Copy mutable input: we will be modifying our copy
+self._args = list(args)
+
 if wrapper is None:
 wrapper = []
-if name is None:
-name = "qemu-%d" % os.getpid()
-if sock_dir is None:
-sock_dir = test_dir
-self._name = name
+self._wrapper = wrapper
+
+self._name = name or "qemu-%d" % os.getpid()
+self._test_dir = test_dir
+self._sock_dir = sock_dir or self._test_dir
+self._socket_scm_helper = socket_scm_helper
+
 if monitor_address is not None:
 self._monitor_address = monitor_address
 self._remove_monitor_sockfile = False
 else:
 self._monitor_address = os.path.join(
-sock_dir, f"{name}-monitor.sock"
+self._sock_dir, f"{self._name}-monitor.sock"
 )
 self._remove_monitor_sockfile = True
+
+# Runstate
 self._qemu_log_path = None
 self._qemu_log_file = None
 self._popen = None
-self._binary = binary
-self._args = list(args) # Force copy args in case we modify them
-self._wrapper = wrapper
 self._events = []
 self._iolog = None
-self._socket_scm_helper = socket_scm_helper
 self._qmp_set = True   # Enable QMP monitor by default.
 self._qmp = None
 self._qemu_full_args = None
-self._test_dir = test_dir
 self._temp_dir = None
-self._sock_dir = sock_dir
 self._launched = False
 self._machine = None
 self._console_index = 0
-- 
2.21.3

[PATCH v5 11/16] python/machine.py: use qmp.command

[John Snow]

machine.py and qmp.py both do the same thing here; refactor machine.py
to use qmp.py's functionality more directly.

Signed-off-by: John Snow 
Reviewed-by: Kevin Wolf 
---
 python/qemu/machine.py | 26 +++---
 1 file changed, 15 insertions(+), 11 deletions(-)

diff --git a/python/qemu/machine.py b/python/qemu/machine.py
index 48e70253fa..0df94c3211 100644
--- a/python/qemu/machine.py
+++ b/python/qemu/machine.py
@@ -26,6 +26,8 @@
 import socket
 import tempfile
 from typing import (
+Any,
+Dict,
 List,
 Optional,
 Type,
@@ -505,17 +507,23 @@ def set_qmp_monitor(self, enabled=True):
 self._qmp_set = False
 self._qmp = None
 
-def qmp(self, cmd, conv_keys=True, **args):
-"""
-Invoke a QMP command and return the response dict
-"""
+@classmethod
+def _qmp_args(cls, _conv_keys: bool = True, **args: Any) -> Dict[str, Any]:
 qmp_args = dict()
 for key, value in args.items():
-if conv_keys:
+if _conv_keys:
 qmp_args[key.replace('_', '-')] = value
 else:
 qmp_args[key] = value
+return qmp_args
 
+def qmp(self, cmd: str,
+conv_keys: bool = True,
+**args: Any) -> QMPMessage:
+"""
+Invoke a QMP command and return the response dict
+"""
+qmp_args = self._qmp_args(conv_keys, **args)
 return self._qmp.cmd(cmd, args=qmp_args)
 
 def command(self, cmd, conv_keys=True, **args):
@@ -524,12 +532,8 @@ def command(self, cmd, conv_keys=True, **args):
 On success return the response dict.
 On failure raise an exception.
 """
-reply = self.qmp(cmd, conv_keys, **args)
-if reply is None:
-raise qmp.QMPError("Monitor is closed")
-if "error" in reply:
-raise qmp.QMPResponseError(reply)
-return reply["return"]
+qmp_args = self._qmp_args(conv_keys, **args)
+return self._qmp.command(cmd, **qmp_args)
 
 def get_qmp_event(self, wait=False):
 """
-- 
2.21.3

[PATCH v5 10/16] python/machine.py: Handle None events in events_wait

[John Snow]

If the timeout is 0, we can get None back. Handle this explicitly.

Signed-off-by: John Snow 
Reviewed-by: Kevin Wolf 
---
 python/qemu/machine.py | 27 ---
 1 file changed, 20 insertions(+), 7 deletions(-)

diff --git a/python/qemu/machine.py b/python/qemu/machine.py
index 37e859f6d2..48e70253fa 100644
--- a/python/qemu/machine.py
+++ b/python/qemu/machine.py
@@ -33,7 +33,7 @@
 from types import TracebackType
 
 from . import qmp
-from .qmp import SocketAddrT
+from .qmp import SocketAddrT, QMPMessage
 
 LOG = logging.getLogger(__name__)
 
@@ -594,13 +594,20 @@ def event_wait(self, name, timeout=60.0, match=None):
 
 def events_wait(self, events, timeout=60.0):
 """
-events_wait waits for and returns a named event
-from QMP with a timeout.
+events_wait waits for and returns a single named event from QMP.
+In the case of multiple qualifying events, this function returns the
+first one.
 
-events: a sequence of (name, match_criteria) tuples.
-The match criteria are optional and may be None.
-See event_match for details.
-timeout: QEMUMonitorProtocol.pull_event timeout parameter.
+:param events: A sequence of (name, match_criteria) tuples.
+   The match criteria are optional and may be None.
+   See event_match for details.
+:param timeout: Optional timeout, in seconds.
+See QEMUMonitorProtocol.pull_event.
+
+:raise QMPTimeoutError: If timeout was non-zero and no matching events
+were found.
+:return: A QMP event matching the filter criteria.
+ If timeout was 0 and no event matched, None.
 """
 def _match(event):
 for name, match in events:
@@ -608,6 +615,8 @@ def _match(event):
 return True
 return False
 
+event: Optional[QMPMessage]
+
 # Search cached events
 for event in self._events:
 if _match(event):
@@ -617,6 +626,10 @@ def _match(event):
 # Poll for new events
 while True:
 event = self._qmp.pull_event(wait=timeout)
+if event is None:
+# NB: None is only returned when timeout is false-ish.
+# Timeouts raise QMPTimeoutError instead!
+break
 if _match(event):
 return event
 self._events.append(event)
-- 
2.21.3

[PATCH v5 13/16] python/machine.py: fix _popen access

[John Snow]

As always, Optional[T] causes problems with unchecked access. Add a
helper that asserts the pipe is present before we attempt to talk with
it.

Signed-off-by: John Snow 
---
 python/qemu/machine.py | 16 +++-
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/python/qemu/machine.py b/python/qemu/machine.py
index 9760a2badb..2ba5e3134e 100644
--- a/python/qemu/machine.py
+++ b/python/qemu/machine.py
@@ -120,7 +120,7 @@ def __init__(self, binary, args=None, wrapper=None, 
name=None,
 # Runstate
 self._qemu_log_path = None
 self._qemu_log_file = None
-self._popen = None
+self._popen: Optional['subprocess.Popen[bytes]'] = None
 self._events = []
 self._iolog = None
 self._qmp_set = True   # Enable QMP monitor by default.
@@ -233,6 +233,12 @@ def is_running(self):
 """Returns true if the VM is running."""
 return self._popen is not None and self._popen.poll() is None
 
+@property
+def _subp(self) -> 'subprocess.Popen[bytes]':
+if self._popen is None:
+raise QEMUMachineError('Subprocess pipe not present')
+return self._popen
+
 def exitcode(self):
 """Returns the exit code if possible, or None."""
 if self._popen is None:
@@ -243,7 +249,7 @@ def get_pid(self):
 """Returns the PID of the running process, or None."""
 if not self.is_running():
 return None
-return self._popen.pid
+return self._subp.pid
 
 def _load_io_log(self):
 if self._qemu_log_path is not None:
@@ -404,8 +410,8 @@ def _hard_shutdown(self) -> None:
 waiting for the QEMU process to terminate.
 """
 self._early_cleanup()
-self._popen.kill()
-self._popen.wait(timeout=60)
+self._subp.kill()
+self._subp.wait(timeout=60)
 
 def _soft_shutdown(self, has_quit: bool = False,
timeout: Optional[int] = 3) -> None:
@@ -429,7 +435,7 @@ def _soft_shutdown(self, has_quit: bool = False,
 self._qmp.cmd('quit')
 
 # May raise subprocess.TimeoutExpired
-self._popen.wait(timeout=timeout)
+self._subp.wait(timeout=timeout)
 
 def _do_shutdown(self, has_quit: bool = False,
  timeout: Optional[int] = 3) -> None:
-- 
2.21.3

[PATCH v5 07/16] python/machine.py: Fix monitor address typing

[John Snow]

Prior to this, it's difficult for mypy to intuit what the concrete type
of the monitor address is; it has difficulty inferring the type across
two variables.

Create _monitor_address as a property that always returns a valid
address to simplify static type analysis.

To preserve our ability to clean up, use a simple boolean to indicate
whether or not we should try to clean up the sock file after execution.

Signed-off-by: John Snow 
Reviewed-by: Kevin Wolf 
---
 python/qemu/machine.py | 45 +++---
 1 file changed, 29 insertions(+), 16 deletions(-)

diff --git a/python/qemu/machine.py b/python/qemu/machine.py
index 8ca3d508df..bab1b1921f 100644
--- a/python/qemu/machine.py
+++ b/python/qemu/machine.py
@@ -25,10 +25,14 @@
 import signal
 import socket
 import tempfile
-from typing import Optional, Type
+from typing import (
+Optional,
+Type,
+)
 from types import TracebackType
 
 from . import qmp
+from .qmp import SocketAddrT
 
 LOG = logging.getLogger(__name__)
 
@@ -68,7 +72,8 @@ class QEMUMachine:
 """
 
 def __init__(self, binary, args=None, wrapper=None, name=None,
- test_dir="/var/tmp", monitor_address=None,
+ test_dir="/var/tmp",
+ monitor_address: Optional[SocketAddrT] = None,
  socket_scm_helper=None, sock_dir=None):
 '''
 Initialize a QEMUMachine
@@ -91,8 +96,14 @@ def __init__(self, binary, args=None, wrapper=None, 
name=None,
 if sock_dir is None:
 sock_dir = test_dir
 self._name = name
-self._monitor_address = monitor_address
-self._vm_monitor = None
+if monitor_address is not None:
+self._monitor_address = monitor_address
+self._remove_monitor_sockfile = False
+else:
+self._monitor_address = os.path.join(
+sock_dir, f"{name}-monitor.sock"
+)
+self._remove_monitor_sockfile = True
 self._qemu_log_path = None
 self._qemu_log_file = None
 self._popen = None
@@ -231,15 +242,17 @@ def _load_io_log(self):
 
 def _base_args(self):
 args = ['-display', 'none', '-vga', 'none']
+
 if self._qmp_set:
 if isinstance(self._monitor_address, tuple):
-moncdev = "socket,id=mon,host=%s,port=%s" % (
-self._monitor_address[0],
-self._monitor_address[1])
+moncdev = "socket,id=mon,host={},port={}".format(
+*self._monitor_address
+)
 else:
-moncdev = 'socket,id=mon,path=%s' % self._vm_monitor
+moncdev = f"socket,id=mon,path={self._monitor_address}"
 args.extend(['-chardev', moncdev, '-mon',
  'chardev=mon,mode=control'])
+
 if self._machine is not None:
 args.extend(['-machine', self._machine])
 for _ in range(self._console_index):
@@ -264,14 +277,14 @@ def _pre_launch(self):
 self._qemu_log_file = open(self._qemu_log_path, 'wb')
 
 if self._qmp_set:
-if self._monitor_address is not None:
-self._vm_monitor = self._monitor_address
-else:
-self._vm_monitor = os.path.join(self._sock_dir,
-self._name + "-monitor.sock")
-self._remove_files.append(self._vm_monitor)
-self._qmp = qmp.QEMUMonitorProtocol(self._vm_monitor, server=True,
-nickname=self._name)
+if self._remove_monitor_sockfile:
+assert isinstance(self._monitor_address, str)
+self._remove_files.append(self._monitor_address)
+self._qmp = qmp.QEMUMonitorProtocol(
+self._monitor_address,
+server=True,
+nickname=self._name
+)
 
 def _post_launch(self):
 if self._qmp:
-- 
2.21.3

[PATCH v5 04/16] python/qmp.py: Do not return None from cmd_obj

[John Snow]

This makes typing the qmp library difficult, as it necessitates wrapping
Optional[] around the type for every return type up the stack. At some
point, it becomes difficult to discern or remember why it's None instead
of the expected object.

Use the python exception system to tell us exactly why we didn't get an
object. Remove this special-cased return.

Signed-off-by: John Snow 
Reviewed-by: Kevin Wolf 
---
 python/qemu/qmp.py | 14 +-
 1 file changed, 5 insertions(+), 9 deletions(-)

diff --git a/python/qemu/qmp.py b/python/qemu/qmp.py
index aa8a666b8a..ef3c919b76 100644
--- a/python/qemu/qmp.py
+++ b/python/qemu/qmp.py
@@ -225,22 +225,18 @@ def accept(self, timeout=15.0):
 self.__sockfile = self.__sock.makefile(mode='r')
 return self.__negotiate_capabilities()
 
-def cmd_obj(self, qmp_cmd):
+def cmd_obj(self, qmp_cmd: QMPMessage) -> QMPMessage:
 """
 Send a QMP command to the QMP Monitor.
 
 @param qmp_cmd: QMP command to be sent as a Python dict
-@return QMP response as a Python dict or None if the connection has
-been closed
+@return QMP response as a Python dict
 """
 self.logger.debug(">>> %s", qmp_cmd)
-try:
-self.__sock.sendall(json.dumps(qmp_cmd).encode('utf-8'))
-except OSError as err:
-if err.errno == errno.EPIPE:
-return None
-raise err
+self.__sock.sendall(json.dumps(qmp_cmd).encode('utf-8'))
 resp = self.__json_read()
+if resp is None:
+raise QMPConnectError("Unexpected empty reply from server")
 self.logger.debug("<<< %s", resp)
 return resp
 
-- 
2.21.3

[PATCH v5 05/16] python/qmp.py: add casts to JSON deserialization

[John Snow]

mypy and python type hints are not powerful enough to properly describe
JSON messages in Python 3.6. The best we can do, generally, is describe
them as Dict[str, Any].

Add casts to coerce this type for static analysis; but do NOT enforce
this type at runtime in any way.

Note: Python 3.8 adds a TypedDict construct which allows for the
description of more arbitrary Dictionary shapes. There is a third-party
module, "Pydantic", which is compatible with 3.6 that can be used
instead of the JSON library that parses JSON messages to fully-typed
Python objects, and may be preferable in some cases.

(That is well beyond the scope of this commit or series.)

Signed-off-by: John Snow 
Reviewed-by: Kevin Wolf 
---
 python/qemu/qmp.py | 8 ++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/python/qemu/qmp.py b/python/qemu/qmp.py
index ef3c919b76..1ae36050a4 100644
--- a/python/qemu/qmp.py
+++ b/python/qemu/qmp.py
@@ -13,6 +13,7 @@
 import logging
 from typing import (
 Any,
+cast,
 Dict,
 Optional,
 TextIO,
@@ -130,7 +131,10 @@ def __json_read(self, only_event=False):
 data = self.__sockfile.readline()
 if not data:
 return None
-resp = json.loads(data)
+# By definition, any JSON received from QMP is a QMPMessage,
+# and we are asserting only at static analysis time that it
+# has a particular shape.
+resp: QMPMessage = json.loads(data)
 if 'event' in resp:
 self.logger.debug("<<< %s", resp)
 self.__events.append(resp)
@@ -262,7 +266,7 @@ def command(self, cmd, **kwds):
 ret = self.cmd(cmd, kwds)
 if 'error' in ret:
 raise QMPResponseError(ret)
-return ret['return']
+return cast(QMPReturnValue, ret['return'])
 
 def pull_event(self, wait=False):
 """
-- 
2.21.3

[PATCH v5 02/16] iotests.py: use qemu.qmp type aliases

[John Snow]

iotests.py should use the type definitions from qmp.py instead of its
own.

Signed-off-by: John Snow 
Reviewed-by: Kevin Wolf 
---
 tests/qemu-iotests/iotests.py | 9 +++--
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index f1e0733dda..4457786f09 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -35,13 +35,10 @@
 # pylint: disable=import-error, wrong-import-position
 sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'python'))
 from qemu import qtest
+from qemu.qmp import QMPMessage
 
 assert sys.version_info >= (3, 6)
 
-# Type Aliases
-QMPResponse = Dict[str, Any]
-
-
 # Use this logger for logging messages directly from the iotests module
 logger = logging.getLogger('qemu.iotests')
 logger.addHandler(logging.NullHandler())
@@ -554,7 +551,7 @@ def add_incoming(self, addr):
 self._args.append(addr)
 return self
 
-def hmp(self, command_line: str, use_log: bool = False) -> QMPResponse:
+def hmp(self, command_line: str, use_log: bool = False) -> QMPMessage:
 cmd = 'human-monitor-command'
 kwargs = {'command-line': command_line}
 if use_log:
@@ -575,7 +572,7 @@ def resume_drive(self, drive: str) -> None:
 self.hmp(f'qemu-io {drive} "remove_break bp_{drive}"')
 
 def hmp_qemu_io(self, drive: str, cmd: str,
-use_log: bool = False) -> QMPResponse:
+use_log: bool = False) -> QMPMessage:
 """Write to a given drive using an HMP command"""
 return self.hmp(f'qemu-io {drive} "{cmd}"', use_log=use_log)
 
-- 
2.21.3

[PATCH v5 01/16] python/qmp.py: Define common types

[John Snow]

Define some common types that we'll need to annotate a lot of other
functions going forward.

Signed-off-by: John Snow 
Reviewed-by: Kevin Wolf 
---
 python/qemu/qmp.py | 18 ++
 1 file changed, 18 insertions(+)

diff --git a/python/qemu/qmp.py b/python/qemu/qmp.py
index e64b6b5faa..8388c7b603 100644
--- a/python/qemu/qmp.py
+++ b/python/qemu/qmp.py
@@ -12,13 +12,31 @@
 import socket
 import logging
 from typing import (
+Any,
+Dict,
 Optional,
 TextIO,
 Type,
+Tuple,
+Union,
 )
 from types import TracebackType
 
 
+# QMPMessage is a QMP Message of any kind.
+# e.g. {'yee': 'haw'}
+#
+# QMPReturnValue is the inner value of return values only.
+# {'return': {}} is the QMPMessage,
+# {} is the QMPReturnValue.
+QMPMessage = Dict[str, Any]
+QMPReturnValue = Dict[str, Any]
+
+InternetAddrT = Tuple[str, str]
+UnixAddrT = str
+SocketAddrT = Union[InternetAddrT, UnixAddrT]
+
+
 class QMPError(Exception):
 """
 QMP base exception
-- 
2.21.3

[PATCH v5 06/16] python/qmp.py: add QMPProtocolError

[John Snow]

In the case that we receive a reply but are unable to understand it, use
this exception name to indicate that case.

Signed-off-by: John Snow 
Reviewed-by: Philippe Mathieu-Daudé 
Reviewed-by: Kevin Wolf 
---
 python/qemu/qmp.py | 10 ++
 1 file changed, 10 insertions(+)

diff --git a/python/qemu/qmp.py b/python/qemu/qmp.py
index 1ae36050a4..7935dababb 100644
--- a/python/qemu/qmp.py
+++ b/python/qemu/qmp.py
@@ -62,6 +62,12 @@ class QMPTimeoutError(QMPError):
 """
 
 
+class QMPProtocolError(QMPError):
+"""
+QMP protocol error; unexpected response
+"""
+
+
 class QMPResponseError(QMPError):
 """
 Represents erroneous QMP monitor reply
@@ -266,6 +272,10 @@ def command(self, cmd, **kwds):
 ret = self.cmd(cmd, kwds)
 if 'error' in ret:
 raise QMPResponseError(ret)
+if 'return' not in ret:
+raise QMPProtocolError(
+"'return' key not found in QMP response '{}'".format(str(ret))
+)
 return cast(QMPReturnValue, ret['return'])
 
 def pull_event(self, wait=False):
-- 
2.21.3

[PATCH v5 00/16] python: add mypy support to python/qemu

[John Snow]

Based-on: 20200710050649.32434-1-js...@redhat.com

This series modifies the python/qemu library to comply with mypy --strict,
pylint, and flake8.
This requires my "refactor shutdown" patch as a pre-requisite.

v5: (Things unchanged omitted)

003/16:[] [-C] 'python/qmp.py: re-absorb MonitorResponseError'
009/16:[] [-C] 'python/machine.py: Don't modify state in _base_args()'
012/16:[0004] [FC] 'python/machine.py: Add _qmp access shim'
013/16:[0002] [FC] 'python/machine.py: fix _popen access'
016/16:[0004] [FC] 'python/qemu: Add mypy type annotations'

--  Rebased on "refactor shutdown" v5
12: Dependent changes from more extensive shutdown() refactoring
13: Dependent changes; one less ._popen access.
16: Dependent changes; _post_shutdown return annotation fell down here.
   wait annotation got bumped up to the previous series.

v4:
 - Rebased on "refactor shutdown" v4
 - Fixed _qmp access for scripts that disable QMP

v3:
005: Removed a cast, per Kevin Wolf's tip
010: Renamed with correct function name;
 Rewrote docstring and added comments
016: Use SocketAddrT instead of Union[Tuple[str,str],str]

"v2":
- This version supports iotests 297
- Many patches merged by Phil are removed
- Replaces iotests.py type aliases with centralized ones
  (See patch 2)
- Imports etc are reworked to use the non-installable
  package layout instead. (Mostly important for patch 3)

Testing this out:
- You'll need Python3.6+
- I encourage you to use a virtual environment!
- You don't necessarily need these exact versions, but I didn't test the
  lower bounds, use older versions at your peril:
  - pylint==2.5.0
  - mypy=0.770
  - flake8=3.7.8

> cd ~/src/qemu/python/
> flake8 qemu
> mypy --strict qemu
> cd qemu
> pylint *.py

These should all 100% pass.

John Snow (16):
  python/qmp.py: Define common types
  iotests.py: use qemu.qmp type aliases
  python/qmp.py: re-absorb MonitorResponseError
  python/qmp.py: Do not return None from cmd_obj
  python/qmp.py: add casts to JSON deserialization
  python/qmp.py: add QMPProtocolError
  python/machine.py: Fix monitor address typing
  python/machine.py: reorder __init__
  python/machine.py: Don't modify state in _base_args()
  python/machine.py: Handle None events in events_wait
  python/machine.py: use qmp.command
  python/machine.py: Add _qmp access shim
  python/machine.py: fix _popen access
  python/qemu: make 'args' style arguments immutable
  iotests.py: Adjust HMP kwargs typing
  python/qemu: Add mypy type annotations

 python/qemu/accel.py  |   8 +-
 python/qemu/machine.py| 296 --
 python/qemu/qmp.py| 111 +
 python/qemu/qtest.py  |  53 +++---
 scripts/render_block_graph.py |   7 +-
 tests/qemu-iotests/iotests.py |  11 +-
 6 files changed, 301 insertions(+), 185 deletions(-)

-- 
2.21.3

[PATCH v5 03/16] python/qmp.py: re-absorb MonitorResponseError

[John Snow]

When I initially split this out, I considered this more of a machine
error than a QMP protocol error, but I think that's misguided.

Move this back to qmp.py and name it QMPResponseError. Convert
qmp.command() to use this exception type.

Signed-off-by: John Snow 
Reviewed-by: Philippe Mathieu-Daudé 
Reviewed-by: Kevin Wolf 
---
 python/qemu/machine.py| 15 +--
 python/qemu/qmp.py| 17 +++--
 scripts/render_block_graph.py |  7 +--
 3 files changed, 21 insertions(+), 18 deletions(-)

diff --git a/python/qemu/machine.py b/python/qemu/machine.py
index d08a8e4a6e..8ca3d508df 100644
--- a/python/qemu/machine.py
+++ b/python/qemu/machine.py
@@ -55,19 +55,6 @@ class AbnormalShutdown(QEMUMachineError):
 """
 
 
-class MonitorResponseError(qmp.QMPError):
-"""
-Represents erroneous QMP monitor reply
-"""
-def __init__(self, reply):
-try:
-desc = reply["error"]["desc"]
-except KeyError:
-desc = reply
-super().__init__(desc)
-self.reply = reply
-
-
 class QEMUMachine:
 """
 A QEMU VM
@@ -519,7 +506,7 @@ def command(self, cmd, conv_keys=True, **args):
 if reply is None:
 raise qmp.QMPError("Monitor is closed")
 if "error" in reply:
-raise MonitorResponseError(reply)
+raise qmp.QMPResponseError(reply)
 return reply["return"]
 
 def get_qmp_event(self, wait=False):
diff --git a/python/qemu/qmp.py b/python/qemu/qmp.py
index 8388c7b603..aa8a666b8a 100644
--- a/python/qemu/qmp.py
+++ b/python/qemu/qmp.py
@@ -61,6 +61,19 @@ class QMPTimeoutError(QMPError):
 """
 
 
+class QMPResponseError(QMPError):
+"""
+Represents erroneous QMP monitor reply
+"""
+def __init__(self, reply: QMPMessage):
+try:
+desc = reply['error']['desc']
+except KeyError:
+desc = reply
+super().__init__(desc)
+self.reply = reply
+
+
 class QEMUMonitorProtocol:
 """
 Provide an API to connect to QEMU via QEMU Monitor Protocol (QMP) and then
@@ -251,8 +264,8 @@ def command(self, cmd, **kwds):
 Build and send a QMP command to the monitor, report errors if any
 """
 ret = self.cmd(cmd, kwds)
-if "error" in ret:
-raise Exception(ret['error']['desc'])
+if 'error' in ret:
+raise QMPResponseError(ret)
 return ret['return']
 
 def pull_event(self, wait=False):
diff --git a/scripts/render_block_graph.py b/scripts/render_block_graph.py
index 409b4321f2..da6acf050d 100755
--- a/scripts/render_block_graph.py
+++ b/scripts/render_block_graph.py
@@ -25,7 +25,10 @@
 from graphviz import Digraph
 
 sys.path.append(os.path.join(os.path.dirname(__file__), '..', 'python'))
-from qemu.machine import MonitorResponseError
+from qemu.qmp import (
+QEMUMonitorProtocol,
+QMPResponseError,
+)
 
 
 def perm(arr):
@@ -102,7 +105,7 @@ def command(self, cmd):
 reply = json.loads(subprocess.check_output(ar))
 
 if 'error' in reply:
-raise MonitorResponseError(reply)
+raise QMPResponseError(reply)
 
 return reply['return']
 
-- 
2.21.3

[PATCH 11/13] nfs: add GUri-based URI parsing

[Marc-André Lureau]

Signed-off-by: Marc-André Lureau 
---
 block/nfs.c | 96 -
 1 file changed, 65 insertions(+), 31 deletions(-)

diff --git a/block/nfs.c b/block/nfs.c
index 93d719551d2..0b24044535d 100644
--- a/block/nfs.c
+++ b/block/nfs.c
@@ -77,6 +77,31 @@ typedef struct NFSRPC {
 
 static int nfs_parse_uri(const char *filename, QDict *options, Error **errp)
 {
+const char *scheme, *server, *path, *key, *value;
+
+#ifdef HAVE_GLIB_GURI
+g_autoptr(GUri) uri = NULL;
+g_autoptr(GHashTable) params = NULL;
+g_autoptr(GError) err = NULL;
+GHashTableIter iter;
+
+uri = g_uri_parse(filename, G_URI_FLAGS_ENCODED_QUERY, );
+if (!uri) {
+error_setg(errp, "Failed to parse NFS URI: %s", err->message);
+return -EINVAL;
+}
+
+params = g_uri_parse_params(g_uri_get_query(uri), -1,
+"&;", G_URI_PARAMS_NONE, );
+if (err) {
+error_report("Failed to parse NFS URI query: %s", err->message);
+return -EINVAL;
+}
+
+scheme = g_uri_get_scheme(uri);
+server = g_uri_get_host(uri);
+path = g_uri_get_path(uri);
+#else
 g_autoptr(URI) uri = NULL;
 g_autoptr(QueryParams) qp = NULL;
 int i;
@@ -86,58 +111,67 @@ static int nfs_parse_uri(const char *filename, QDict 
*options, Error **errp)
 error_setg(errp, "Invalid URI specified");
 return -EINVAL;
 }
-if (g_strcmp0(uri->scheme, "nfs") != 0) {
-error_setg(errp, "URI scheme must be 'nfs'");
+
+qp = query_params_parse(uri->query);
+if (!qp) {
+error_setg(errp, "could not parse query parameters");
 return -EINVAL;
 }
 
-if (!uri->server) {
-error_setg(errp, "missing hostname in URI");
+scheme = uri->scheme;
+server = uri->server;
+path = uri->path;
+#endif
+if (g_strcmp0(scheme, "nfs") != 0) {
+error_setg(errp, "URI scheme must be 'nfs'");
 return -EINVAL;
 }
 
-if (!uri->path) {
-error_setg(errp, "missing file path in URI");
+if (!server) {
+error_setg(errp, "missing hostname in URI");
 return -EINVAL;
 }
 
-qp = query_params_parse(uri->query);
-if (!qp) {
-error_setg(errp, "could not parse query parameters");
+if (!path) {
+error_setg(errp, "missing file path in URI");
 return -EINVAL;
 }
 
-qdict_put_str(options, "server.host", uri->server);
+qdict_put_str(options, "server.host", server);
 qdict_put_str(options, "server.type", "inet");
-qdict_put_str(options, "path", uri->path);
+qdict_put_str(options, "path", path);
 
+#ifdef HAVE_GLIB_GURI
+g_hash_table_iter_init(, params);
+while (g_hash_table_iter_next(, (void **), (void **))) {
+#else
 for (i = 0; i < qp->n; i++) {
+key = qp->p[i].name;
+value = qp->p[i].value;
+#endif
 unsigned long long val;
-if (!qp->p[i].value) {
-error_setg(errp, "Value for NFS parameter expected: %s",
-   qp->p[i].name);
+if (!value) {
+error_setg(errp, "Value for NFS parameter expected: %s", key);
 return -EINVAL;
 }
-if (parse_uint_full(qp->p[i].value, , 0)) {
-error_setg(errp, "Illegal value for NFS parameter: %s",
-   qp->p[i].name);
+if (parse_uint_full(value, , 0)) {
+error_setg(errp, "Illegal value for NFS parameter: %s", key);
 return -EINVAL;
 }
-if (!strcmp(qp->p[i].name, "uid")) {
-qdict_put_str(options, "user", qp->p[i].value);
-} else if (!strcmp(qp->p[i].name, "gid")) {
-qdict_put_str(options, "group", qp->p[i].value);
-} else if (!strcmp(qp->p[i].name, "tcp-syncnt")) {
-qdict_put_str(options, "tcp-syn-count", qp->p[i].value);
-} else if (!strcmp(qp->p[i].name, "readahead")) {
-qdict_put_str(options, "readahead-size", qp->p[i].value);
-} else if (!strcmp(qp->p[i].name, "pagecache")) {
-qdict_put_str(options, "page-cache-size", qp->p[i].value);
-} else if (!strcmp(qp->p[i].name, "debug")) {
-qdict_put_str(options, "debug", qp->p[i].value);
+if (!strcmp(key, "uid")) {
+qdict_put_str(options, "user", value);
+} else if (!strcmp(key, "gid")) {
+qdict_put_str(options, "group", value);
+} else if (!strcmp(key, "tcp-syncnt")) {
+qdict_put_str(options, "tcp-syn-count", value);
+} else if (!strcmp(key, "readahead")) {
+qdict_put_str(options, "readahead-size", value);
+} else if (!strcmp(key, "pagecache")) {
+qdict_put_str(options, "page-cache-size", value);
+} else if (!strcmp(key, "debug")) {
+qdict_put_str(options, "debug", value);
 } else {
-error_setg(errp, "Unknown NFS parameter name: %s",
-   qp->p[i].name);
+ 

[PATCH 12/13] gluster: add GUri-based URI parsing

[Marc-André Lureau]

Signed-off-by: Marc-André Lureau 
---
 block/gluster.c | 81 +
 1 file changed, 61 insertions(+), 20 deletions(-)

diff --git a/block/gluster.c b/block/gluster.c
index c06eca1c12f..2cad76deabf 100644
--- a/block/gluster.c
+++ b/block/gluster.c
@@ -288,9 +288,9 @@ static void glfs_clear_preopened(glfs_t *fs)
 }
 }
 
-static int parse_volume_options(BlockdevOptionsGluster *gconf, char *path)
+static int parse_volume_options(BlockdevOptionsGluster *gconf, const char 
*path)
 {
-char *p, *q;
+const char *p, *q;
 
 if (!path) {
 return -EINVAL;
@@ -349,56 +349,97 @@ static int qemu_gluster_parse_uri(BlockdevOptionsGluster 
*gconf,
   const char *filename)
 {
 SocketAddress *gsconf;
+bool is_unix = false;
+int ret, port;
+const char *scheme, *server, *path, *key = NULL, *value = NULL;
+size_t nparams;
+
+#ifdef HAVE_GLIB_GURI
+g_autoptr(GUri) uri = NULL;
+g_autoptr(GHashTable) params = NULL;
+g_autoptr(GError) err = NULL;
+
+uri = g_uri_parse(filename, G_URI_FLAGS_ENCODED_QUERY, );
+if (!uri) {
+error_report("Failed to parse gluster URI: %s", err->message);
+return -EINVAL;
+}
+
+params = g_uri_parse_params(g_uri_get_query(uri), -1,
+"&;", G_URI_PARAMS_NONE, );
+if (err) {
+error_report("Failed to parse gluster URI query: %s", err->message);
+return -EINVAL;
+}
+
+scheme = g_uri_get_scheme(uri);
+server = g_uri_get_host(uri);
+port = g_uri_get_port(uri);
+path = g_uri_get_path(uri);
+nparams = g_hash_table_size(params);
+g_hash_table_lookup_extended(params, "socket",
+ (void **), (void **));
+#else
 g_autoptr(URI) uri = NULL;
 g_autoptr(QueryParams) qp = NULL;
-bool is_unix = false;
-int ret;
 
 uri = uri_parse(filename);
 if (!uri) {
 return -EINVAL;
 }
 
+qp = query_params_parse(uri->query);
+
+scheme = uri->scheme;
+server = uri->server;
+port = uri->port;
+path = uri->path;
+nparams = qp->n;
+if (nparams > 0) {
+key = qp->p[0].name;
+value = qp->p[0].value;
+}
+#endif
+
+if (nparams > 1 || (is_unix && !nparams) || (!is_unix && nparams)) {
+return -EINVAL;
+}
+
 gconf->server = g_new0(SocketAddressList, 1);
 gconf->server->value = gsconf = g_new0(SocketAddress, 1);
 
 /* transport */
-if (!uri->scheme || !strcmp(uri->scheme, "gluster")) {
+if (!scheme || !strcmp(scheme, "gluster")) {
 gsconf->type = SOCKET_ADDRESS_TYPE_INET;
-} else if (!strcmp(uri->scheme, "gluster+tcp")) {
+} else if (!strcmp(scheme, "gluster+tcp")) {
 gsconf->type = SOCKET_ADDRESS_TYPE_INET;
-} else if (!strcmp(uri->scheme, "gluster+unix")) {
+} else if (!strcmp(scheme, "gluster+unix")) {
 gsconf->type = SOCKET_ADDRESS_TYPE_UNIX;
 is_unix = true;
-} else if (!strcmp(uri->scheme, "gluster+rdma")) {
+} else if (!strcmp(scheme, "gluster+rdma")) {
 gsconf->type = SOCKET_ADDRESS_TYPE_INET;
 warn_report("rdma feature is not supported, falling back to tcp");
 } else {
 return -EINVAL;
 }
 
-ret = parse_volume_options(gconf, uri->path);
+ret = parse_volume_options(gconf, path);
 if (ret < 0) {
 return ret;
 }
 
-qp = query_params_parse(uri->query);
-if (qp->n > 1 || (is_unix && !qp->n) || (!is_unix && qp->n)) {
-return -EINVAL;
-}
-
 if (is_unix) {
-if (uri->server || uri->port) {
+if (server || port) {
 return -EINVAL;
 }
-if (strcmp(qp->p[0].name, "socket")) {
+if (g_strcmp0(key, "socket")) {
 return -EINVAL;
 }
-gsconf->u.q_unix.path = g_strdup(qp->p[0].value);
+gsconf->u.q_unix.path = g_strdup(value);
 } else {
-gsconf->u.inet.host = g_strdup(uri->server ? uri->server : 
"localhost");
-if (uri->port) {
-gsconf->u.inet.port = g_strdup_printf("%d", uri->port);
+gsconf->u.inet.host = g_strdup(server ? server : "localhost");
+if (port) {
+gsconf->u.inet.port = g_strdup_printf("%d", port);
 } else {
 gsconf->u.inet.port = g_strdup_printf("%d", GLUSTER_DEFAULT_PORT);
 }
-- 
2.27.0.221.ga08a83db2b

[PATCH 10/13] sheepdog: add GUri-based URI parsing

[Marc-André Lureau]

Signed-off-by: Marc-André Lureau 
---
 block/sheepdog.c | 99 ++--
 1 file changed, 71 insertions(+), 28 deletions(-)

diff --git a/block/sheepdog.c b/block/sheepdog.c
index 3403adfc2cd..3f3f5b7dba9 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -1003,24 +1003,48 @@ static void sd_config_done(SheepdogConfig *cfg)
 static void sd_parse_uri(SheepdogConfig *cfg, const char *filename,
  Error **errp)
 {
-g_autoptr(QueryParams) qp = NULL;
-g_autoptr(URI) uri = NULL;
+const char *scheme, *server, *path, *fragment, *socket = NULL;
+int port;
 bool is_unix;
 
-memset(cfg, 0, sizeof(*cfg));
+#ifdef HAVE_GLIB_GURI
+g_autoptr(GUri) uri = NULL;
+g_autoptr(GHashTable) params = NULL;
+g_autoptr(GError) err = NULL;
+
+uri = g_uri_parse(filename, G_URI_FLAGS_ENCODED_QUERY, );
+if (!uri) {
+error_report("Failed to parse sheepdog URI: %s", err->message);
+return;
+}
+scheme = g_uri_get_scheme(uri);
+server = g_uri_get_host(uri);
+port = g_uri_get_port(uri);
+path = g_uri_get_path(uri);
+fragment = g_uri_get_fragment(uri);
+#else
+g_autoptr(QueryParams) qp = NULL;
+g_autoptr(URI) uri = NULL;
 
 uri = uri_parse(filename);
 if (!uri) {
 error_setg(errp, "invalid URI '%s'", filename);
 return;
 }
+scheme = uri->scheme;
+server = uri->server;
+port = uri->port;
+path = uri->path;
+fragment = uri->fragment;
+#endif
+memset(cfg, 0, sizeof(*cfg));
 
 /* transport */
-if (!g_strcmp0(uri->scheme, "sheepdog")) {
+if (!g_strcmp0(scheme, "sheepdog")) {
 is_unix = false;
-} else if (!g_strcmp0(uri->scheme, "sheepdog+tcp")) {
+} else if (!g_strcmp0(scheme, "sheepdog+tcp")) {
 is_unix = false;
-} else if (!g_strcmp0(uri->scheme, "sheepdog+unix")) {
+} else if (!g_strcmp0(scheme, "sheepdog+unix")) {
 is_unix = true;
 } else {
 error_setg(errp, "URI scheme must be 'sheepdog', 'sheepdog+tcp',"
@@ -1028,52 +1052,71 @@ static void sd_parse_uri(SheepdogConfig *cfg, const 
char *filename,
 return;
 }
 
-if (uri->path == NULL || !strcmp(uri->path, "/")) {
+#ifdef HAVE_GLIB_GURI
+params = g_uri_parse_params(g_uri_get_query(uri), -1,
+"&;", G_URI_PARAMS_NONE, );
+if (err) {
+error_report("Failed to parse sheepdog URI query: %s", err->message);
+return;
+}
+if ((is_unix && g_hash_table_size(params) != 1) ||
+(!is_unix && g_hash_table_size(params) != 0)) {
+error_setg(errp, "unexpected query parameters");
+return;
+}
+if (is_unix) {
+socket = g_hash_table_lookup(params, "socket");
+}
+#else
+qp = query_params_parse(uri->query);
+if (qp->n > 1 || (is_unix && !qp->n) || (!is_unix && qp->n)) {
+error_setg(errp, "unexpected query parameters");
+return;
+}
+if (is_unix) {
+if (!g_str_equal(qp->p[0].name, "socket")) {
+error_setg(errp, "unexpected query parameters");
+return;
+}
+socket = qp->p[0].value;
+}
+#endif
+if (path == NULL || !strcmp(path, "/")) {
 error_setg(errp, "missing file path in URI");
 return;
 }
-if (g_strlcpy(cfg->vdi, uri->path + 1, SD_MAX_VDI_LEN)
+if (g_strlcpy(cfg->vdi, path + 1, SD_MAX_VDI_LEN)
 >= SD_MAX_VDI_LEN) {
 error_setg(errp, "VDI name is too long");
 return;
 }
 
-qp = query_params_parse(uri->query);
-
 if (is_unix) {
 /* sheepdog+unix:///vdiname?socket=path */
-if (uri->server || uri->port) {
+if (server || port > 0) {
 error_setg(errp, "URI scheme %s doesn't accept a server address",
-   uri->scheme);
+   scheme);
 return;
 }
-if (!qp->n) {
+if (!socket) {
 error_setg(errp,
"URI scheme %s requires query parameter 'socket'",
-   uri->scheme);
-return;
-}
-if (qp->n != 1 || strcmp(qp->p[0].name, "socket")) {
-error_setg(errp, "unexpected query parameters");
+   scheme);
 return;
 }
-cfg->path = g_strdup(qp->p[0].value);
+cfg->path = g_strdup(socket);
 } else {
 /* sheepdog[+tcp]://[host:port]/vdiname */
-if (qp->n) {
-error_setg(errp, "unexpected query parameters");
-return;
-}
-cfg->host = g_strdup(uri->server);
-cfg->port = uri->port;
+cfg->host = g_strdup(server);
+cfg->port = port;
 }
 
 /* snapshot tag */
-if (uri->fragment) {
-if (!sd_parse_snapid_or_tag(uri->fragment,
+if (fragment) {
+if (!sd_parse_snapid_or_tag(fragment,
 >snap_id, cfg->tag)) {

[PATCH 13/13] ssh: add GUri-based URI parsing

[Marc-André Lureau]

Signed-off-by: Marc-André Lureau 
---
 block/ssh.c | 75 +
 1 file changed, 58 insertions(+), 17 deletions(-)

diff --git a/block/ssh.c b/block/ssh.c
index c8f6ad79e3c..d2bc6277613 100644
--- a/block/ssh.c
+++ b/block/ssh.c
@@ -180,9 +180,37 @@ static void sftp_error_trace(BDRVSSHState *s, const char 
*op)
 
 static int parse_uri(const char *filename, QDict *options, Error **errp)
 {
+g_autofree char *port_str = NULL;
+const char *scheme, *server, *path, *user, *key, *value;
+gint port;
+
+#ifdef HAVE_GLIB_GURI
+g_autoptr(GUri) uri = NULL;
+g_autoptr(GHashTable) params = NULL;
+g_autoptr(GError) err = NULL;
+GHashTableIter iter;
+
+uri = g_uri_parse(filename, G_URI_FLAGS_ENCODED_QUERY, );
+if (!uri) {
+error_setg(errp, "Failed to parse SSH URI: %s", err->message);
+return -EINVAL;
+}
+
+params = g_uri_parse_params(g_uri_get_query(uri), -1,
+"&;", G_URI_PARAMS_NONE, );
+if (err) {
+error_report("Failed to parse SSH URI query: %s", err->message);
+return -EINVAL;
+}
+
+scheme = g_uri_get_scheme(uri);
+user = g_uri_get_user(uri);
+server = g_uri_get_host(uri);
+path = g_uri_get_path(uri);
+port = g_uri_get_port(uri);
+#else
 g_autoptr(URI) uri = NULL;
 g_autoptr(QueryParams) qp = NULL;
-g_autofree char *port_str = NULL;
 int i;
 
 uri = uri_parse(filename);
@@ -190,44 +218,57 @@ static int parse_uri(const char *filename, QDict 
*options, Error **errp)
 return -EINVAL;
 }
 
-if (g_strcmp0(uri->scheme, "ssh") != 0) {
-error_setg(errp, "URI scheme must be 'ssh'");
+qp = query_params_parse(uri->query);
+if (!qp) {
+error_setg(errp, "could not parse query parameters");
 return -EINVAL;
 }
 
-if (!uri->server || strcmp(uri->server, "") == 0) {
-error_setg(errp, "missing hostname in URI");
+scheme = uri->scheme;
+user = uri->user;
+server = uri->server;
+path = uri->path;
+port = uri->port;
+#endif
+if (g_strcmp0(scheme, "ssh") != 0) {
+error_setg(errp, "URI scheme must be 'ssh'");
 return -EINVAL;
 }
 
-if (!uri->path || strcmp(uri->path, "") == 0) {
-error_setg(errp, "missing remote path in URI");
+if (!server || strcmp(server, "") == 0) {
+error_setg(errp, "missing hostname in URI");
 return -EINVAL;
 }
 
-qp = query_params_parse(uri->query);
-if (!qp) {
-error_setg(errp, "could not parse query parameters");
+if (!path || strcmp(path, "") == 0) {
+error_setg(errp, "missing remote path in URI");
 return -EINVAL;
 }
 
-if(uri->user && strcmp(uri->user, "") != 0) {
-qdict_put_str(options, "user", uri->user);
+if (user && strcmp(user, "") != 0) {
+qdict_put_str(options, "user", user);
 }
 
-qdict_put_str(options, "server.host", uri->server);
+qdict_put_str(options, "server.host", server);
 
-port_str = g_strdup_printf("%d", uri->port ?: 22);
+port_str = g_strdup_printf("%d", port ?: 22);
 qdict_put_str(options, "server.port", port_str);
 
-qdict_put_str(options, "path", uri->path);
+qdict_put_str(options, "path", path);
 
 /* Pick out any query parameters that we understand, and ignore
  * the rest.
  */
+#ifdef HAVE_GLIB_GURI
+g_hash_table_iter_init(, params);
+while (g_hash_table_iter_next(, (void **), (void **))) {
+#else
 for (i = 0; i < qp->n; ++i) {
-if (strcmp(qp->p[i].name, "host_key_check") == 0) {
-qdict_put_str(options, "host_key_check", qp->p[i].value);
+key = qp->p[i].name;
+value = qp->p[i].value;
+#endif
+if (g_strcmp0(key, "host_key_check") == 0) {
+qdict_put_str(options, "host_key_check", value);
 }
 }
 
-- 
2.27.0.221.ga08a83db2b

[PATCH 07/13] block/gluster: auto-ify URI parsing variables

[Marc-André Lureau]

Signed-off-by: Marc-André Lureau 
---
 block/gluster.c | 27 +--
 1 file changed, 9 insertions(+), 18 deletions(-)

diff --git a/block/gluster.c b/block/gluster.c
index 31233cac696..c06eca1c12f 100644
--- a/block/gluster.c
+++ b/block/gluster.c
@@ -349,10 +349,10 @@ static int qemu_gluster_parse_uri(BlockdevOptionsGluster 
*gconf,
   const char *filename)
 {
 SocketAddress *gsconf;
-URI *uri;
-QueryParams *qp = NULL;
+g_autoptr(URI) uri = NULL;
+g_autoptr(QueryParams) qp = NULL;
 bool is_unix = false;
-int ret = 0;
+int ret;
 
 uri = uri_parse(filename);
 if (!uri) {
@@ -374,29 +374,25 @@ static int qemu_gluster_parse_uri(BlockdevOptionsGluster 
*gconf,
 gsconf->type = SOCKET_ADDRESS_TYPE_INET;
 warn_report("rdma feature is not supported, falling back to tcp");
 } else {
-ret = -EINVAL;
-goto out;
+return -EINVAL;
 }
 
 ret = parse_volume_options(gconf, uri->path);
 if (ret < 0) {
-goto out;
+return ret;
 }
 
 qp = query_params_parse(uri->query);
 if (qp->n > 1 || (is_unix && !qp->n) || (!is_unix && qp->n)) {
-ret = -EINVAL;
-goto out;
+return -EINVAL;
 }
 
 if (is_unix) {
 if (uri->server || uri->port) {
-ret = -EINVAL;
-goto out;
+return -EINVAL;
 }
 if (strcmp(qp->p[0].name, "socket")) {
-ret = -EINVAL;
-goto out;
+return -EINVAL;
 }
 gsconf->u.q_unix.path = g_strdup(qp->p[0].value);
 } else {
@@ -408,12 +404,7 @@ static int qemu_gluster_parse_uri(BlockdevOptionsGluster 
*gconf,
 }
 }
 
-out:
-if (qp) {
-query_params_free(qp);
-}
-uri_free(uri);
-return ret;
+return 0;
 }
 
 static struct glfs *qemu_gluster_glfs_init(BlockdevOptionsGluster *gconf,
-- 
2.27.0.221.ga08a83db2b

[PATCH 05/13] block/ssh: auto-ify URI parsing variables

[Marc-André Lureau]

Signed-off-by: Marc-André Lureau 
---
 block/ssh.c | 23 +++
 1 file changed, 7 insertions(+), 16 deletions(-)

diff --git a/block/ssh.c b/block/ssh.c
index 098dbe03c15..c8f6ad79e3c 100644
--- a/block/ssh.c
+++ b/block/ssh.c
@@ -180,9 +180,9 @@ static void sftp_error_trace(BDRVSSHState *s, const char 
*op)
 
 static int parse_uri(const char *filename, QDict *options, Error **errp)
 {
-URI *uri = NULL;
-QueryParams *qp;
-char *port_str;
+g_autoptr(URI) uri = NULL;
+g_autoptr(QueryParams) qp = NULL;
+g_autofree char *port_str = NULL;
 int i;
 
 uri = uri_parse(filename);
@@ -192,23 +192,23 @@ static int parse_uri(const char *filename, QDict 
*options, Error **errp)
 
 if (g_strcmp0(uri->scheme, "ssh") != 0) {
 error_setg(errp, "URI scheme must be 'ssh'");
-goto err;
+return -EINVAL;
 }
 
 if (!uri->server || strcmp(uri->server, "") == 0) {
 error_setg(errp, "missing hostname in URI");
-goto err;
+return -EINVAL;
 }
 
 if (!uri->path || strcmp(uri->path, "") == 0) {
 error_setg(errp, "missing remote path in URI");
-goto err;
+return -EINVAL;
 }
 
 qp = query_params_parse(uri->query);
 if (!qp) {
 error_setg(errp, "could not parse query parameters");
-goto err;
+return -EINVAL;
 }
 
 if(uri->user && strcmp(uri->user, "") != 0) {
@@ -219,7 +219,6 @@ static int parse_uri(const char *filename, QDict *options, 
Error **errp)
 
 port_str = g_strdup_printf("%d", uri->port ?: 22);
 qdict_put_str(options, "server.port", port_str);
-g_free(port_str);
 
 qdict_put_str(options, "path", uri->path);
 
@@ -232,15 +231,7 @@ static int parse_uri(const char *filename, QDict *options, 
Error **errp)
 }
 }
 
-query_params_free(qp);
-uri_free(uri);
 return 0;
-
- err:
-if (uri) {
-  uri_free(uri);
-}
-return -EINVAL;
 }
 
 static bool ssh_has_filename_options_conflict(QDict *options, Error **errp)
-- 
2.27.0.221.ga08a83db2b

[PATCH 04/13] block/sheepdog: auto-ify URI parsing variables

[Marc-André Lureau]

Since we are going to introduce URI parsing alternative, I changed the
way SheepdogConfig takes care of host/path & URI/QueryParams lifetimes.

Signed-off-by: Marc-André Lureau 
---
 block/sheepdog.c | 72 
 1 file changed, 30 insertions(+), 42 deletions(-)

diff --git a/block/sheepdog.c b/block/sheepdog.c
index 27a30d17f4c..3403adfc2cd 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -986,39 +986,33 @@ static bool sd_parse_snapid_or_tag(const char *str,
 }
 
 typedef struct {
-const char *path;   /* non-null iff transport is tcp */
-const char *host;   /* valid when transport is tcp */
+char *path; /* non-null iff transport is tcp */
+char *host; /* valid when transport is tcp */
 int port;   /* valid when transport is tcp */
 char vdi[SD_MAX_VDI_LEN];
 char tag[SD_MAX_VDI_TAG_LEN];
 uint32_t snap_id;
-/* Remainder is only for sd_config_done() */
-URI *uri;
-QueryParams *qp;
 } SheepdogConfig;
 
 static void sd_config_done(SheepdogConfig *cfg)
 {
-if (cfg->qp) {
-query_params_free(cfg->qp);
-}
-uri_free(cfg->uri);
+g_clear_pointer(>host, g_free);
+g_clear_pointer(>path, g_free);
 }
 
 static void sd_parse_uri(SheepdogConfig *cfg, const char *filename,
  Error **errp)
 {
-Error *err = NULL;
-QueryParams *qp = NULL;
+g_autoptr(QueryParams) qp = NULL;
+g_autoptr(URI) uri = NULL;
 bool is_unix;
-URI *uri;
 
 memset(cfg, 0, sizeof(*cfg));
 
-cfg->uri = uri = uri_parse(filename);
+uri = uri_parse(filename);
 if (!uri) {
-error_setg(, "invalid URI '%s'", filename);
-goto out;
+error_setg(errp, "invalid URI '%s'", filename);
+return;
 }
 
 /* transport */
@@ -1029,48 +1023,48 @@ static void sd_parse_uri(SheepdogConfig *cfg, const 
char *filename,
 } else if (!g_strcmp0(uri->scheme, "sheepdog+unix")) {
 is_unix = true;
 } else {
-error_setg(, "URI scheme must be 'sheepdog', 'sheepdog+tcp',"
+error_setg(errp, "URI scheme must be 'sheepdog', 'sheepdog+tcp',"
" or 'sheepdog+unix'");
-goto out;
+return;
 }
 
 if (uri->path == NULL || !strcmp(uri->path, "/")) {
-error_setg(, "missing file path in URI");
-goto out;
+error_setg(errp, "missing file path in URI");
+return;
 }
 if (g_strlcpy(cfg->vdi, uri->path + 1, SD_MAX_VDI_LEN)
 >= SD_MAX_VDI_LEN) {
-error_setg(, "VDI name is too long");
-goto out;
+error_setg(errp, "VDI name is too long");
+return;
 }
 
-cfg->qp = qp = query_params_parse(uri->query);
+qp = query_params_parse(uri->query);
 
 if (is_unix) {
 /* sheepdog+unix:///vdiname?socket=path */
 if (uri->server || uri->port) {
-error_setg(, "URI scheme %s doesn't accept a server address",
+error_setg(errp, "URI scheme %s doesn't accept a server address",
uri->scheme);
-goto out;
+return;
 }
 if (!qp->n) {
-error_setg(,
+error_setg(errp,
"URI scheme %s requires query parameter 'socket'",
uri->scheme);
-goto out;
+return;
 }
 if (qp->n != 1 || strcmp(qp->p[0].name, "socket")) {
-error_setg(, "unexpected query parameters");
-goto out;
+error_setg(errp, "unexpected query parameters");
+return;
 }
-cfg->path = qp->p[0].value;
+cfg->path = g_strdup(qp->p[0].value);
 } else {
 /* sheepdog[+tcp]://[host:port]/vdiname */
 if (qp->n) {
-error_setg(, "unexpected query parameters");
-goto out;
+error_setg(errp, "unexpected query parameters");
+return;
 }
-cfg->host = uri->server;
+cfg->host = g_strdup(uri->server);
 cfg->port = uri->port;
 }
 
@@ -1078,19 +1072,13 @@ static void sd_parse_uri(SheepdogConfig *cfg, const 
char *filename,
 if (uri->fragment) {
 if (!sd_parse_snapid_or_tag(uri->fragment,
 >snap_id, cfg->tag)) {
-error_setg(, "'%s' is not a valid snapshot ID",
+error_setg(errp, "'%s' is not a valid snapshot ID",
uri->fragment);
-goto out;
+return;
 }
 } else {
 cfg->snap_id = CURRENT_VDI_ID; /* search current vdi */
 }
-
-out:
-if (err) {
-error_propagate(errp, err);
-sd_config_done(cfg);
-}
 }
 
 /*
@@ -1184,7 +1172,7 @@ static void sd_parse_filename(const char *filename, QDict 
*options,
 }
 if (err) {
 error_propagate(errp, err);
-return;
+goto end;
 }
 
 if 

[PATCH 03/13] block/vxhs: auto-ify URI parsing variables

[Marc-André Lureau]

Signed-off-by: Marc-André Lureau 
---
 block/vxhs.c | 10 +++---
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/block/vxhs.c b/block/vxhs.c
index d79fc97df66..5d61cfb7548 100644
--- a/block/vxhs.c
+++ b/block/vxhs.c
@@ -174,14 +174,12 @@ static QemuOptsList runtime_tcp_opts = {
  */
 static int vxhs_parse_uri(const char *filename, QDict *options)
 {
-URI *uri = NULL;
-char *port;
-int ret = 0;
+g_autoptr(URI) uri = NULL;
+g_autofree char *port = NULL;
 
 trace_vxhs_parse_uri_filename(filename);
 uri = uri_parse(filename);
 if (!uri || !uri->server || !uri->path) {
-uri_free(uri);
 return -EINVAL;
 }
 
@@ -190,15 +188,13 @@ static int vxhs_parse_uri(const char *filename, QDict 
*options)
 if (uri->port) {
 port = g_strdup_printf("%d", uri->port);
 qdict_put_str(options, VXHS_OPT_SERVER ".port", port);
-g_free(port);
 }
 
 qdict_put_str(options, "vdisk-id", uri->path);
 
 trace_vxhs_parse_uri_hostinfo(uri->server, uri->port);
-uri_free(uri);
 
-return ret;
+return 0;
 }
 
 static void vxhs_parse_filename(const char *filename, QDict *options,
-- 
2.27.0.221.ga08a83db2b

[PATCH 08/13] build-sys: add HAVE_GLIB_GURI

[Marc-André Lureau]

Signed-off-by: Marc-André Lureau 
---
 configure | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/configure b/configure
index ee6c3c6792a..cd2fc120aed 100755
--- a/configure
+++ b/configure
@@ -3924,6 +3924,10 @@ if $pkg_config --atleast-version=$glib_req_ver 
gio-unix-2.0; then
 gio_libs="$gio_libs $($pkg_config --libs gio-unix-2.0)"
 fi
 
+if $pkg_config --atleast-version=2.65.0 glib-2.0; then
+glib_guri=yes
+fi
+
 # Sanity check that the current size_t matches the
 # size that glib thinks it should be. This catches
 # problems on multi-arch where people try to build
@@ -7377,6 +7381,9 @@ if test "$gio" = "yes" ; then
 echo "GIO_LIBS=$gio_libs" >> $config_host_mak
 echo "GDBUS_CODEGEN=$gdbus_codegen" >> $config_host_mak
 fi
+if test "$glib_guri" = "yes" ; then
+echo "HAVE_GLIB_GURI=y" >> $config_host_mak
+fi
 echo "CONFIG_TLS_PRIORITY=\"$tls_priority\"" >> $config_host_mak
 if test "$gnutls" = "yes" ; then
   echo "CONFIG_GNUTLS=y" >> $config_host_mak
-- 
2.27.0.221.ga08a83db2b

[PATCH 09/13] nbd: add GUri-based URI parsing version

[Marc-André Lureau]

Signed-off-by: Marc-André Lureau 
---
 block/nbd.c| 86 +++---
 util/Makefile.objs |  2 +-
 2 files changed, 66 insertions(+), 22 deletions(-)

diff --git a/block/nbd.c b/block/nbd.c
index faadcab442b..fdc4a53a98f 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -31,7 +31,10 @@
 #include "qemu/osdep.h"
 
 #include "trace.h"
+#ifndef HAVE_GLIB_GURI
 #include "qemu/uri.h"
+#endif
+#include "qemu/error-report.h"
 #include "qemu/option.h"
 #include "qemu/cutils.h"
 #include "qemu/main-loop.h"
@@ -1513,71 +1516,112 @@ static int nbd_client_connect(BlockDriverState *bs, 
Error **errp)
 /*
  * Parse nbd_open options
  */
-
 static int nbd_parse_uri(const char *filename, QDict *options)
 {
+const char *p, *scheme, *server, *socket = NULL;
+int port;
+bool is_unix;
+
+#ifdef HAVE_GLIB_GURI
+g_autoptr(GUri) uri = NULL;
+g_autoptr(GHashTable) params = NULL;
+g_autoptr(GError) err = NULL;
+
+uri = g_uri_parse(filename, G_URI_FLAGS_ENCODED_QUERY, );
+if (!uri) {
+error_report("Failed to parse NBD URI: %s", err->message);
+return -EINVAL;
+}
+
+p = g_uri_get_path(uri);
+scheme = g_uri_get_scheme(uri);
+server = g_uri_get_host(uri);
+port = g_uri_get_port(uri);
+#else
 g_autoptr(URI) uri = NULL;
 g_autoptr(QueryParams) qp = NULL;
-const char *p;
-bool is_unix;
 
 uri = uri_parse(filename);
 if (!uri) {
 return -EINVAL;
 }
 
+p = uri->path ? uri->path : "";
+scheme = uri->scheme;
+server = uri->server;
+port = uri->port;
+#endif
 /* transport */
-if (!g_strcmp0(uri->scheme, "nbd")) {
+if (!g_strcmp0(scheme, "nbd")) {
 is_unix = false;
-} else if (!g_strcmp0(uri->scheme, "nbd+tcp")) {
+} else if (!g_strcmp0(scheme, "nbd+tcp")) {
 is_unix = false;
-} else if (!g_strcmp0(uri->scheme, "nbd+unix")) {
+} else if (!g_strcmp0(scheme, "nbd+unix")) {
 is_unix = true;
 } else {
 return -EINVAL;
 }
-
-p = uri->path ? uri->path : "";
-if (p[0] == '/') {
-p++;
+#ifdef HAVE_GLIB_GURI
+params = g_uri_parse_params(g_uri_get_query(uri), -1,
+"&;", G_URI_PARAMS_NONE, );
+if (err) {
+error_report("Failed to parse NBD URI query: %s", err->message);
+return -EINVAL;
 }
-if (p[0]) {
-qdict_put_str(options, "export", p);
+if ((is_unix && g_hash_table_size(params) != 1) ||
+(!is_unix && g_hash_table_size(params) != 0)) {
+return -EINVAL;
 }
-
+if (is_unix) {
+socket = g_hash_table_lookup(params, "socket");
+}
+#else
 qp = query_params_parse(uri->query);
 if (qp->n > 1 || (is_unix && !qp->n) || (!is_unix && qp->n)) {
 return -EINVAL;
 }
+if (is_unix) {
+if (!g_str_equal(qp->p[0].name, "socket")) {
+return -EINVAL;
+}
+socket = qp->p[0].value;
+}
+#endif
+if (p[0] == '/') {
+p++;
+}
+if (p[0]) {
+qdict_put_str(options, "export", p);
+}
 
 if (is_unix) {
 /* nbd+unix:///export?socket=path */
-if (uri->server || uri->port || strcmp(qp->p[0].name, "socket")) {
+if (server || port > 0) {
 return -EINVAL;
 }
 qdict_put_str(options, "server.type", "unix");
-qdict_put_str(options, "server.path", qp->p[0].value);
+qdict_put_str(options, "server.path", socket);
 } else {
 QString *host;
 g_autofree char *port_str = NULL;
 
 /* nbd[+tcp]://host[:port]/export */
-if (!uri->server) {
+if (!server) {
 return -EINVAL;
 }
 
 /* strip braces from literal IPv6 address */
-if (uri->server[0] == '[') {
-host = qstring_from_substr(uri->server, 1,
-   strlen(uri->server) - 1);
+if (server[0] == '[') {
+host = qstring_from_substr(server, 1,
+   strlen(server) - 1);
 } else {
-host = qstring_from_str(uri->server);
+host = qstring_from_str(server);
 }
 
 qdict_put_str(options, "server.type", "inet");
 qdict_put(options, "server.host", host);
 
-port_str = g_strdup_printf("%d", uri->port ?: NBD_DEFAULT_PORT);
+port_str = g_strdup_printf("%d", port > 0 ? port : NBD_DEFAULT_PORT);
 qdict_put_str(options, "server.port", port_str);
 }
 
diff --git a/util/Makefile.objs b/util/Makefile.objs
index cc5e37177af..5a162178ae9 100644
--- a/util/Makefile.objs
+++ b/util/Makefile.objs
@@ -73,7 +73,7 @@ util-obj-y += qemu-timer.o
 util-obj-y += thread-pool.o
 util-obj-y += throttle.o
 util-obj-y += timed-average.o
-util-obj-y += uri.o
+util-obj-$(call lnot,$(HAVE_GLIB_GURI)) += uri.o
 
 util-obj-$(CONFIG_LINUX) += vfio-helpers.o
 util-obj-$(CONFIG_INOTIFY1) += filemonitor-inotify.o
-- 

[PATCH 06/13] block/nfs: auto-ify URI parsing variables

[Marc-André Lureau]

Signed-off-by: Marc-André Lureau 
---
 block/nfs.c | 32 
 1 file changed, 12 insertions(+), 20 deletions(-)

diff --git a/block/nfs.c b/block/nfs.c
index b1718d125a4..93d719551d2 100644
--- a/block/nfs.c
+++ b/block/nfs.c
@@ -77,34 +77,34 @@ typedef struct NFSRPC {
 
 static int nfs_parse_uri(const char *filename, QDict *options, Error **errp)
 {
-URI *uri = NULL;
-QueryParams *qp = NULL;
-int ret = -EINVAL, i;
+g_autoptr(URI) uri = NULL;
+g_autoptr(QueryParams) qp = NULL;
+int i;
 
 uri = uri_parse(filename);
 if (!uri) {
 error_setg(errp, "Invalid URI specified");
-goto out;
+return -EINVAL;
 }
 if (g_strcmp0(uri->scheme, "nfs") != 0) {
 error_setg(errp, "URI scheme must be 'nfs'");
-goto out;
+return -EINVAL;
 }
 
 if (!uri->server) {
 error_setg(errp, "missing hostname in URI");
-goto out;
+return -EINVAL;
 }
 
 if (!uri->path) {
 error_setg(errp, "missing file path in URI");
-goto out;
+return -EINVAL;
 }
 
 qp = query_params_parse(uri->query);
 if (!qp) {
 error_setg(errp, "could not parse query parameters");
-goto out;
+return -EINVAL;
 }
 
 qdict_put_str(options, "server.host", uri->server);
@@ -116,12 +116,12 @@ static int nfs_parse_uri(const char *filename, QDict 
*options, Error **errp)
 if (!qp->p[i].value) {
 error_setg(errp, "Value for NFS parameter expected: %s",
qp->p[i].name);
-goto out;
+return -EINVAL;
 }
 if (parse_uint_full(qp->p[i].value, , 0)) {
 error_setg(errp, "Illegal value for NFS parameter: %s",
qp->p[i].name);
-goto out;
+return -EINVAL;
 }
 if (!strcmp(qp->p[i].name, "uid")) {
 qdict_put_str(options, "user", qp->p[i].value);
@@ -138,18 +138,10 @@ static int nfs_parse_uri(const char *filename, QDict 
*options, Error **errp)
 } else {
 error_setg(errp, "Unknown NFS parameter name: %s",
qp->p[i].name);
-goto out;
+return -EINVAL;
 }
 }
-ret = 0;
-out:
-if (qp) {
-query_params_free(qp);
-}
-if (uri) {
-uri_free(uri);
-}
-return ret;
+return 0;
 }
 
 static bool nfs_has_filename_options_conflict(QDict *options, Error **errp)
-- 
2.27.0.221.ga08a83db2b

[PATCH 01/13] uri: add g_auto macros for URI & QueryParams

[Marc-André Lureau]

Signed-off-by: Marc-André Lureau 
---
 include/qemu/uri.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/qemu/uri.h b/include/qemu/uri.h
index d201c61260d..b246a59449b 100644
--- a/include/qemu/uri.h
+++ b/include/qemu/uri.h
@@ -105,6 +105,9 @@ struct QueryParams *query_params_new (int init_alloc);
 extern QueryParams *query_params_parse (const char *query);
 extern void query_params_free (QueryParams *ps);
 
+G_DEFINE_AUTOPTR_CLEANUP_FUNC(URI, uri_free)
+G_DEFINE_AUTOPTR_CLEANUP_FUNC(QueryParams, query_params_free)
+
 #ifdef __cplusplus
 }
 #endif
-- 
2.27.0.221.ga08a83db2b

[PATCH 00/13] RFC: use upcoming GUri for URI handling

[Marc-André Lureau]

Hi,

After years trying to add a glib API to handle URI, GLib 2.65.1 will finally
have one. As an exercice, I checked if the API fits qemu needs, and it seems to
be fine. It should be about as verbose as the current libxml based URI parser,
but the main benefit is that we will get rid of fairly complex URI
copied code in our tree.

The first few patches are code improvements mainly around g_auto, then the
patches to convert URI code over GUri. Obviously, it will take years before this
new API reaches old-stable distros. We may want to have a copy version of GUri,
instead of the current libxml copy as a fallback. Or we may want to keep both
current code and new GUri-based code side-by-side. I am more in favour of the
second approach, given that GUri is fresh, and may have subtle parsing
differences that better being spotted and fixed from unstable/newer distros
first. Maintaining the two side-by-side for some while shouldn't be a big
burdden, as they have a lot of similarities, and the code around it is pretty
stable.

thanks

Marc-André Lureau (13):
  uri: add g_auto macros for URI & QueryParams
  block/nbd: auto-ify URI parsing variables
  block/vxhs: auto-ify URI parsing variables
  block/sheepdog: auto-ify URI parsing variables
  block/ssh: auto-ify URI parsing variables
  block/nfs: auto-ify URI parsing variables
  block/gluster: auto-ify URI parsing variables
  build-sys: add HAVE_GLIB_GURI
  nbd: add GUri-based URI parsing version
  sheepdog: add GUri-based URI parsing
  nfs: add GUri-based URI parsing
  gluster: add GUri-based URI parsing
  ssh: add GUri-based URI parsing

 configure  |   7 +++
 include/qemu/uri.h |   3 +
 block/gluster.c| 102 +++---
 block/nbd.c| 109 +---
 block/nfs.c| 126 ++---
 block/sheepdog.c   | 153 +++--
 block/ssh.c|  94 +++-
 block/vxhs.c   |  10 +--
 util/Makefile.objs |   2 +-
 9 files changed, 383 insertions(+), 223 deletions(-)

-- 
2.27.0.221.ga08a83db2b

[PATCH 02/13] block/nbd: auto-ify URI parsing variables

[Marc-André Lureau]

Signed-off-by: Marc-André Lureau 
---
 block/nbd.c | 27 ---
 1 file changed, 8 insertions(+), 19 deletions(-)

diff --git a/block/nbd.c b/block/nbd.c
index eed160c5cda..faadcab442b 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -1516,10 +1516,9 @@ static int nbd_client_connect(BlockDriverState *bs, 
Error **errp)
 
 static int nbd_parse_uri(const char *filename, QDict *options)
 {
-URI *uri;
+g_autoptr(URI) uri = NULL;
+g_autoptr(QueryParams) qp = NULL;
 const char *p;
-QueryParams *qp = NULL;
-int ret = 0;
 bool is_unix;
 
 uri = uri_parse(filename);
@@ -1535,8 +1534,7 @@ static int nbd_parse_uri(const char *filename, QDict 
*options)
 } else if (!g_strcmp0(uri->scheme, "nbd+unix")) {
 is_unix = true;
 } else {
-ret = -EINVAL;
-goto out;
+return -EINVAL;
 }
 
 p = uri->path ? uri->path : "";
@@ -1549,26 +1547,23 @@ static int nbd_parse_uri(const char *filename, QDict 
*options)
 
 qp = query_params_parse(uri->query);
 if (qp->n > 1 || (is_unix && !qp->n) || (!is_unix && qp->n)) {
-ret = -EINVAL;
-goto out;
+return -EINVAL;
 }
 
 if (is_unix) {
 /* nbd+unix:///export?socket=path */
 if (uri->server || uri->port || strcmp(qp->p[0].name, "socket")) {
-ret = -EINVAL;
-goto out;
+return -EINVAL;
 }
 qdict_put_str(options, "server.type", "unix");
 qdict_put_str(options, "server.path", qp->p[0].value);
 } else {
 QString *host;
-char *port_str;
+g_autofree char *port_str = NULL;
 
 /* nbd[+tcp]://host[:port]/export */
 if (!uri->server) {
-ret = -EINVAL;
-goto out;
+return -EINVAL;
 }
 
 /* strip braces from literal IPv6 address */
@@ -1584,15 +1579,9 @@ static int nbd_parse_uri(const char *filename, QDict 
*options)
 
 port_str = g_strdup_printf("%d", uri->port ?: NBD_DEFAULT_PORT);
 qdict_put_str(options, "server.port", port_str);
-g_free(port_str);
 }
 
-out:
-if (qp) {
-query_params_free(qp);
-}
-uri_free(uri);
-return ret;
+return 0;
 }
 
 static bool nbd_has_filename_options_conflict(QDict *options, Error **errp)
-- 
2.27.0.221.ga08a83db2b

Re: [PULL 00/12] Block patches

[Eduardo Habkost]

On Thu, Jul 09, 2020 at 05:02:06PM +0200, Kevin Wolf wrote:
> Am 08.07.2020 um 00:05 hat Eduardo Habkost geschrieben:
> > On Tue, Jul 07, 2020 at 05:28:21PM +0200, Philippe Mathieu-Daudé wrote:
> > > On 6/26/20 12:25 PM, Stefan Hajnoczi wrote:
> > > > On Thu, Jun 25, 2020 at 02:31:14PM +0100, Peter Maydell wrote:
> > > >> On Wed, 24 Jun 2020 at 11:02, Stefan Hajnoczi  
> > > >> wrote:
> > > >>>
> > > >>> The following changes since commit 
> > > >>> 171199f56f5f9bdf1e5d670d09ef1351d8f01bae:
> > > >>>
> > > >>>   Merge remote-tracking branch 
> > > >>> 'remotes/alistair/tags/pull-riscv-to-apply-20200619-3' into staging 
> > > >>> (2020-06-22 14:45:25 +0100)
> > > >>>
> > > >>> are available in the Git repository at:
> > > >>>
> > > >>>   https://github.com/stefanha/qemu.git tags/block-pull-request
> > > >>>
> > > >>> for you to fetch changes up to 
> > > >>> 7838c67f22a81fcf669785cd6c0876438422071a:
> > > >>>
> > > >>>   block/nvme: support nested aio_poll() (2020-06-23 15:46:08 +0100)
> > > >>>
> > > >>> 
> > > >>> Pull request
> > > >>>
> > > >>> 
> > > >>
> > > >> Failure on iotest 030, x86-64 Linux:
> > > >>
> > > >>   TESTiotest-qcow2: 030 [fail]
> > > >> QEMU  --
> > > >> "/home/petmay01/linaro/qemu-for-merges/build/alldbg/tests/qemu-iotests/../../x86_64-softmmu/qemu-system-x86_64"
> > > >> -nodefaults -display none -accel qtest
> > > >> QEMU_IMG  --
> > > >> "/home/petmay01/linaro/qemu-for-merges/build/alldbg/tests/qemu-iotests/../../qemu-img"
> > > >> QEMU_IO   --
> > > >> "/home/petmay01/linaro/qemu-for-merges/build/alldbg/tests/qemu-iotests/../../qemu-io"
> > > >>  --cache writeback --aio threads -f qcow2
> > > >> QEMU_NBD  --
> > > >> "/home/petmay01/linaro/qemu-for-merges/build/alldbg/tests/qemu-iotests/../../qemu-nbd"
> > > >> IMGFMT-- qcow2 (compat=1.1)
> > > >> IMGPROTO  -- file
> > > >> PLATFORM  -- Linux/x86_64 e104462 4.15.0-76-generic
> > > >> TEST_DIR  --
> > > >> /home/petmay01/linaro/qemu-for-merges/build/alldbg/tests/qemu-iotests/scratch
> > > >> SOCK_DIR  -- /tmp/tmp.8tgdDjoZcO
> > > >> SOCKET_SCM_HELPER --
> > > >> /home/petmay01/linaro/qemu-for-merges/build/alldbg/tests/qemu-iotest/socket_scm_helper
> > > >>
> > > >> --- /home/petmay01/linaro/qemu-for-merges/tests/qemu-iotests/030.out
> > > >>  2019-07-15 17:18:35.251364738 +0100
> > > >> +++ 
> > > >> /home/petmay01/linaro/qemu-for-merges/build/alldbg/tests/qemu-iotests/030.out.bad
> > > >>   2020-06-25 14:04:28.500534007 +0100
> > > >> @@ -1,5 +1,17 @@
> > > >> -...
> > > >> +.F.
> > > >> +==
> > > >> +FAIL: test_stream_parallel (__main__.TestParallelOps)
> > > >> +--
> > > >> +Traceback (most recent call last):
> > > >> +  File "030", line 246, in test_stream_parallel
> > > >> +self.assert_qmp(result, 'return', {})
> > > >> +  File 
> > > >> "/home/petmay01/linaro/qemu-for-merges/tests/qemu-iotests/iotests.py",
> > > >> line 848, in assert_qmp
> > > >> +result = self.dictpath(d, path)
> > > >> +  File 
> > > >> "/home/petmay01/linaro/qemu-for-merges/tests/qemu-iotests/iotests.py",
> > > >> line 822, in dictpath
> > > >> +self.fail(f'failed path traversal for "{path}" in "{d}"')
> > > >> +AssertionError: failed path traversal for "return" in "{'error':
> > > >> {'class': 'DeviceNotActive', 'desc': "Block job 'stream-node8' not
> > > >> found"}}"
> > > >> +
> > > >>  --
> > > >>  Ran 27 tests
> > > >>
> > > >> -OK
> > > >> +FAILED (failures=1)
> > > > 
> > > > Strange, I can't reproduce this failure on my pull request branch or on
> > > > qemu.git/master.
> > > > 
> > > > Is this failure deterministic? Are you sure it is introduced by this
> > > > pull request?
> > > 
> > > Probably not introduced by this pullreq, but I also hit it on FreeBSD:
> > > https://cirrus-ci.com/task/4620718312783872?command=main#L5803
> > > 
> > >   TESTiotest-qcow2: 030 [fail]
> > > QEMU  --
> > > "/tmp/cirrus-ci-build/build/tests/qemu-iotests/../../aarch64-softmmu/qemu-system-aarch64"
> > > -nodefaults -display none -machine virt -accel qtest
> > > QEMU_IMG  --
> > > "/tmp/cirrus-ci-build/build/tests/qemu-iotests/../../qemu-img"
> > > QEMU_IO   --
> > > "/tmp/cirrus-ci-build/build/tests/qemu-iotests/../../qemu-io"  --cache
> > > writeback --aio threads -f qcow2
> > > QEMU_NBD  --
> > > "/tmp/cirrus-ci-build/build/tests/qemu-iotests/../../qemu-nbd"
> > > IMGFMT-- qcow2 (compat=1.1)
> > > IMGPROTO  -- file
> > > PLATFORM  -- FreeBSD/amd64 cirrus-task-4620718312783872 12.1-RELEASE
> > > TEST_DIR  -- /tmp/cirrus-ci-build/build/tests/qemu-iotests/scratch
> > > SOCK_DIR  -- 

Re: [PATCH v5 5/5] vhost-user-blk: default num_queues to -smp N

[Raphael Norwitz]

On Mon, Jul 6, 2020 at 7:00 AM Stefan Hajnoczi  wrote:
>
> Automatically size the number of request virtqueues to match the number
> of vCPUs.  This ensures that completion interrupts are handled on the
> same vCPU that submitted the request.  No IPI is necessary to complete
> an I/O request and performance is improved.  The maximum number of MSI-X
> vectors and virtqueues limit are respected.
>
> Signed-off-by: Stefan Hajnoczi 
> Reviewed-by: Cornelia Huck 
> ---
>  include/hw/virtio/vhost-user-blk.h | 2 ++
>  hw/block/vhost-user-blk.c  | 6 +-
>  hw/core/machine.c  | 1 +
>  hw/virtio/vhost-user-blk-pci.c | 4 
>  4 files changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/include/hw/virtio/vhost-user-blk.h 
> b/include/hw/virtio/vhost-user-blk.h
> index 34ad6f0c0e..292d17147c 100644
> --- a/include/hw/virtio/vhost-user-blk.h
> +++ b/include/hw/virtio/vhost-user-blk.h
> @@ -25,6 +25,8 @@
>  #define VHOST_USER_BLK(obj) \
>  OBJECT_CHECK(VHostUserBlk, (obj), TYPE_VHOST_USER_BLK)
>
> +#define VHOST_USER_BLK_AUTO_NUM_QUEUES UINT16_MAX
> +
>  typedef struct VHostUserBlk {
>  VirtIODevice parent_obj;
>  CharBackend chardev;
> diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
> index a00b854736..39aec42dae 100644
> --- a/hw/block/vhost-user-blk.c
> +++ b/hw/block/vhost-user-blk.c
> @@ -420,6 +420,9 @@ static void vhost_user_blk_device_realize(DeviceState 
> *dev, Error **errp)
>  return;
>  }
>
> +if (s->num_queues == VHOST_USER_BLK_AUTO_NUM_QUEUES) {
> +s->num_queues = 1;
> +}

What is this check for? Is it just a backstop to ensure that
num_queues is set to 1 if vhost-user-blk-pci doesn't update it?

>  if (!s->num_queues || s->num_queues > VIRTIO_QUEUE_MAX) {
>  error_setg(errp, "vhost-user-blk: invalid number of IO queues");
>  return;
> @@ -531,7 +534,8 @@ static const VMStateDescription vmstate_vhost_user_blk = {
>
>  static Property vhost_user_blk_properties[] = {
>  DEFINE_PROP_CHR("chardev", VHostUserBlk, chardev),
> -DEFINE_PROP_UINT16("num-queues", VHostUserBlk, num_queues, 1),
> +DEFINE_PROP_UINT16("num-queues", VHostUserBlk, num_queues,
> +   VHOST_USER_BLK_AUTO_NUM_QUEUES),
>  DEFINE_PROP_UINT32("queue-size", VHostUserBlk, queue_size, 128),
>  DEFINE_PROP_BIT("config-wce", VHostUserBlk, config_wce, 0, true),
>  DEFINE_PROP_END_OF_LIST(),
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index 845f6476cb..31bfaacdb5 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -30,6 +30,7 @@
>
>  GlobalProperty hw_compat_5_0[] = {
>  { "vhost-scsi", "num_queues", "1"},
> +{ "vhost-user-blk", "num-queues", "1"},
>  { "vhost-user-scsi", "num_queues", "1"},
>  { "virtio-balloon-device", "page-poison", "false" },
>  { "virtio-blk-device", "num-queues", "1"},
> diff --git a/hw/virtio/vhost-user-blk-pci.c b/hw/virtio/vhost-user-blk-pci.c
> index 4f5d5cbf44..a62a71e067 100644
> --- a/hw/virtio/vhost-user-blk-pci.c
> +++ b/hw/virtio/vhost-user-blk-pci.c
> @@ -54,6 +54,10 @@ static void vhost_user_blk_pci_realize(VirtIOPCIProxy 
> *vpci_dev, Error **errp)
>  VHostUserBlkPCI *dev = VHOST_USER_BLK_PCI(vpci_dev);
>  DeviceState *vdev = DEVICE(>vdev);
>
> +if (dev->vdev.num_queues == VHOST_USER_BLK_AUTO_NUM_QUEUES) {
> +dev->vdev.num_queues = virtio_pci_optimal_num_queues(0);
> +}
> +
>  if (vpci_dev->nvectors == DEV_NVECTORS_UNSPECIFIED) {
>  vpci_dev->nvectors = dev->vdev.num_queues + 1;
>  }
> --
> 2.26.2
>

Re: [PATCH 2/2] hw/sd/sdcard: Do not allow invalid SD card sizes

[Niek Linnenbank]

On Thu, Jul 9, 2020 at 4:15 PM Peter Maydell 
wrote:

> On Thu, 9 Jul 2020 at 14:56, Philippe Mathieu-Daudé 
> wrote:
> >
> > On 7/7/20 10:29 PM, Niek Linnenbank wrote:
> > > So I manually copy & pasted the change into hw/sd/sd.c to test it.
> > > It looks like the check works, but my concern is that with this change,
> > > we will be getting this error on 'off-the-shelf' images as well.
> > > For example, the latest Raspbian image size also isn't a power of two:
> > >
> > > $ ./arm-softmmu/qemu-system-arm -M raspi2 -sd
> > > ~/Downloads/2020-05-27-raspios-buster-lite-armhf.img -nographic
> > > WARNING: Image format was not specified for
> > > '/home/me/Downloads/2020-05-27-raspios-buster-lite-armhf.img' and
> > > probing guessed raw.
> > >  Automatically detecting the format is dangerous for raw
> images,
> > > write operations on block 0 will be restricted.
> > >  Specify the 'raw' format explicitly to remove the
> restrictions.
> > > qemu-system-arm: Invalid SD card size: 1.73 GiB (expecting at least 2
> GiB)
> > >
> > > If we do decide that the change is needed, I would like to propose that
> > > we also give the user some instructions
> > > on how to fix it, maybe some 'dd' command?
> >
> > On POSIX we can suggest to use 'truncate -s 2G' from coreutils.
> > This is not in the default Darwin packages.
> > On Windows I have no clue.
>
> dd/truncate etc won't work if the image file is not raw (eg if
> it's qcow2). The only chance you have of something that's actually
> generic would probably involve "qemu-img resize". But I'm a bit
> wary of having an error message that recommends that, because
> what if we got it wrong?
>

Yeah good point Peter, I see what you mean. As I wrote to Philippe,
i'll try to make a small patch with some instructions in the OrangePi board
documentation,
so then we'll at least have something there to help the user.

Regards,
Niek


>
> thanks
> -- PMM
>


-- 
Niek Linnenbank

Re: [PATCH 2/2] hw/sd/sdcard: Do not allow invalid SD card sizes

[Niek Linnenbank]

On Thu, Jul 9, 2020 at 3:56 PM Philippe Mathieu-Daudé 
wrote:

> On 7/7/20 10:29 PM, Niek Linnenbank wrote:
> > Hi Philippe,
> >
> > Just tried out your patch on latest master, and I noticed I couldn't
> > apply it without getting this error:
> >
> > $ git am ~/Downloads/patches/\[PATCH\ 2_2\]\ hw_sd_sdcard\:\ Do\ not\
> > allow\ invalid\ SD\ card\ sizes\ -\ Philippe\ Mathieu-Daudé\
> > \mailto:f4...@amsat.org>\>\ -\ 2020-07-07\ 1521.eml
> > Applying: hw/sd/sdcard: Do not allow invalid SD card sizes
> > error: patch failed: hw/sd/sd.c:2130
> > error: hw/sd/sd.c: patch does not apply
> > Patch failed at 0001 hw/sd/sdcard: Do not allow invalid SD card sizes
> > Use 'git am --show-current-patch' to see the failed patch
> > When you have resolved this problem, run "git am --continue".
> > If you prefer to skip this patch, run "git am --skip" instead.
> > To restore the original branch and stop patching, run "git am --abort".
> >
> > The first patch did go OK. Maybe this one just needs to be rebased, or I
> > made a mistake.
>
> Sorry it was not clear on the cover:
>
>   Part 1 is already reviewed:
>   https://www.mail-archive.com/qemu-devel@nongnu.org/msg718150.html
>   Based-on: <20200630133912.9428-1-f4...@amsat.org>
>
> This series is based on the "Part 1".
>
> > So I manually copy & pasted the change into hw/sd/sd.c to test it.
> > It looks like the check works, but my concern is that with this change,
> > we will be getting this error on 'off-the-shelf' images as well.
> > For example, the latest Raspbian image size also isn't a power of two:
> >
> > $ ./arm-softmmu/qemu-system-arm -M raspi2 -sd
> > ~/Downloads/2020-05-27-raspios-buster-lite-armhf.img -nographic
> > WARNING: Image format was not specified for
> > '/home/me/Downloads/2020-05-27-raspios-buster-lite-armhf.img' and
> > probing guessed raw.
> >  Automatically detecting the format is dangerous for raw images,
> > write operations on block 0 will be restricted.
> >  Specify the 'raw' format explicitly to remove the restrictions.
> > qemu-system-arm: Invalid SD card size: 1.73 GiB (expecting at least 2
> GiB)
> >
> > If we do decide that the change is needed, I would like to propose that
> > we also give the user some instructions
> > on how to fix it, maybe some 'dd' command?
>
> On POSIX we can suggest to use 'truncate -s 2G' from coreutils.
> This is not in the default Darwin packages.
> On Windows I have no clue.
>
> > In my opinion that should
> > also go in some of the documentation file(s),
> > possibly also in the one for the OrangePi PC at
> > docs/system/arm/orangepi.rst (I can also provide a patch for that if you
> > wish).
>
> Good idea, if you can send that patch that would a precious help,
> and I'd include it with the other patches :)
>

OK Philipe. Then I'll prepare a patch and try send it to the list somewhere
this weekend.


>
> Note that this was your orangepi-pc acceptance test that catched
> this bug!
> See https://travis-ci.org/github/philmd/qemu/jobs/705653532#L5672:
>
>
Oh cool, that is great. Looks like it is working pretty well then. But lets
be fair, I think it was you that contributed that part ;-)


>  CPU: ARMv7 Processor [410fc075] revision 5 (ARMv7), cr=50c5387d
>  OF: fdt: Machine model: Xunlong Orange Pi PC
>  Kernel command line: printk.time=0 console=ttyS0,115200
> root=/dev/mmcblk0 rootwait rw panic=-1 noreboot
>  sunxi-mmc 1c0f000.mmc: Linked as a consumer to regulator.2
>  sunxi-mmc 1c0f000.mmc: Got CD GPIO
>  sunxi-mmc 1c0f000.mmc: initialized, max. request size: 16384 KB
>  mmc0: host does not support reading read-only switch, assuming
> write-enable
>  mmc0: Problem switching card into high-speed mode!
>  mmc0: new SD card at address 4567
>  mmcblk0: mmc0:4567 QEMU! 60.0 MiB
>  EXT4-fs (mmcblk0): mounting ext2 file system using the ext4 subsystem
>  EXT4-fs (mmcblk0): mounted filesystem without journal. Opts: (null)
>  VFS: Mounted root (ext2 filesystem) on device 179:0.
>  EXT4-fs (mmcblk0): re-mounted. Opts: block_validity,barrier,user_xattr,acl
>  Populating /dev using udev: udevd[204]: starting version 3.2.7
>  udevadm settle failed
>  done
>  udevd[205]: worker [208]
> /devices/platform/soc/1c0f000.mmc/mmc_host/mmc0/mmc0:4567/block/mmcblk0
> is taking a long time
> Runner error occurred: Timeout reached
> Original status: ERROR
>
> (I'll add that in the commit description too).
>

OK thanks!

>
> Thanks for your testing/review!
>
> > Kind regards,
> >
> > Niek
> >
> >
> > On Tue, Jul 7, 2020 at 6:11 PM Philippe Mathieu-Daudé  > > wrote:
> >
> > On 7/7/20 6:06 PM, Peter Maydell wrote:
> > > On Tue, 7 Jul 2020 at 17:04, Alistair Francis
> > mailto:alistai...@gmail.com>> wrote:
> > >>
> > >> On Tue, Jul 7, 2020 at 6:22 AM Philippe Mathieu-Daudé
> > mailto:f4...@amsat.org>> wrote:
> > >>>
> > >>> QEMU allows to create SD card with unrealistic sizes. This could
> > work,
> > >>> but some guests (at least Linux) consider sizes 

Re: [PATCH 2/2] hw/sd/sdcard: Do not allow invalid SD card sizes

[Alistair Francis]

On Thu, Jul 9, 2020 at 7:35 AM Philippe Mathieu-Daudé  wrote:
>
> On 7/9/20 4:15 PM, Peter Maydell wrote:
> > On Thu, 9 Jul 2020 at 14:56, Philippe Mathieu-Daudé  wrote:
> >>
> >> On 7/7/20 10:29 PM, Niek Linnenbank wrote:
> >>> So I manually copy & pasted the change into hw/sd/sd.c to test it.
> >>> It looks like the check works, but my concern is that with this change,
> >>> we will be getting this error on 'off-the-shelf' images as well.
> >>> For example, the latest Raspbian image size also isn't a power of two:
> >>>
> >>> $ ./arm-softmmu/qemu-system-arm -M raspi2 -sd
> >>> ~/Downloads/2020-05-27-raspios-buster-lite-armhf.img -nographic
> >>> WARNING: Image format was not specified for
> >>> '/home/me/Downloads/2020-05-27-raspios-buster-lite-armhf.img' and
> >>> probing guessed raw.
> >>>  Automatically detecting the format is dangerous for raw images,
> >>> write operations on block 0 will be restricted.
> >>>  Specify the 'raw' format explicitly to remove the restrictions.
> >>> qemu-system-arm: Invalid SD card size: 1.73 GiB (expecting at least 2 GiB)
> >>>
> >>> If we do decide that the change is needed, I would like to propose that
> >>> we also give the user some instructions
> >>> on how to fix it, maybe some 'dd' command?
> >>
> >> On POSIX we can suggest to use 'truncate -s 2G' from coreutils.
> >> This is not in the default Darwin packages.
> >> On Windows I have no clue.
> >
> > dd/truncate etc won't work if the image file is not raw (eg if
> > it's qcow2).
>
> Good catch...
>
> > The only chance you have of something that's actually
> > generic would probably involve "qemu-img resize". But I'm a bit
> > wary of having an error message that recommends that, because
> > what if we got it wrong?
>
> I am not sure what to recommend then.
>
> Would that work as hint?
>
>   qemu-system-arm -M raspi2 -sd ./buster-lite-armhf.img
>   qemu-system-arm: Invalid SD card size: 1.73 GiB
>   SD card size has to be a power of 2, e.g. 2GiB.

That sounds good to me. That's enough for a user to figure out the next step.

If you want you could also add: "qemu-img might be able to help." or
something like that.

Alistair

Re: [PATCH v7 14/47] stream: Deal with filters

[Andrey Shinkevich]

On 09.07.2020 17:52, Andrey Shinkevich wrote:

On 25.06.2020 18:21, Max Reitz wrote:

Because of the (not so recent anymore) changes that make the stream job
independent of the base node and instead track the node above it, we
have to split that "bottom" node into two cases: The bottom COW node,
and the node directly above the base node (which may be an R/W filter
or the bottom COW node).

Signed-off-by: Max Reitz 
---
  qapi/block-core.json |  4 +++
  block/stream.c   | 63 
  blockdev.c   |  4 ++-
  3 files changed, 53 insertions(+), 18 deletions(-)


...

+    BlockDriverState *base_overlay = bdrv_find_overlay(bs, base);
+    BlockDriverState *above_base;
  -    if (bdrv_freeze_backing_chain(bs, bottom, errp) < 0) {
+    if (!base_overlay) {
+    error_setg(errp, "'%s' is not in the backing chain of '%s'",
+   base->node_name, bs->node_name);


Sorry, I am not clear with the error message.

In this case, there is no an intermediate COW node but the base, if 
not NULL, is


in the backing chain of bs, isn't it?


I am discarding this question. No need to answer.

Andrey



+    return;
+    }
+

Re: [PATCH v7 14/47] stream: Deal with filters

[Andrey Shinkevich]

On 25.06.2020 18:21, Max Reitz wrote:

Because of the (not so recent anymore) changes that make the stream job
independent of the base node and instead track the node above it, we
have to split that "bottom" node into two cases: The bottom COW node,
and the node directly above the base node (which may be an R/W filter
or the bottom COW node).

Signed-off-by: Max Reitz 
---
  qapi/block-core.json |  4 +++
  block/stream.c   | 63 
  blockdev.c   |  4 ++-
  3 files changed, 53 insertions(+), 18 deletions(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index b20332e592..df87855429 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2486,6 +2486,10 @@
  # On successful completion the image file is updated to drop the backing file
  # and the BLOCK_JOB_COMPLETED event is emitted.
  #
+# In case @device is a filter node, block-stream modifies the first non-filter
+# overlay node below it to point to base's backing node (or NULL if @base was


Forgot one thing. To me, it would be more understandable to read

"...to point to the base as backing node..." because it may be thought 
as a backing


node of the base.

Andrey


+# not specified) instead of modifying @device itself.
+#
  # @job-id: identifier for the newly-created block job. If
  #  omitted, the device name will be used. (Since 2.7)
  #

Re: [PULL 18/31] block/core: add generic infrastructure for x-blockdev-amend qmp command

[Peter Maydell]

On Mon, 6 Jul 2020 at 11:05, Max Reitz  wrote:
>
> From: Maxim Levitsky 
>
> blockdev-amend will be used similiar to blockdev-create
> to allow on the fly changes of the structure of the format based block 
> devices.
>
> Current plan is to first support encryption keyslot management for luks
> based formats (raw and embedded in qcow2)
>
> Signed-off-by: Maxim Levitsky 
> Reviewed-by: Daniel P. Berrangé 
> Message-Id: <20200608094030.670121-12-mlevi...@redhat.com>
> Signed-off-by: Max Reitz 

Hi; Coverity reports a possible issue with this function
(CID 1430268):

> +void qmp_x_blockdev_amend(const char *job_id,
> +  const char *node_name,
> +  BlockdevAmendOptions *options,
> +  bool has_force,
> +  bool force,
> +  Error **errp)
> +{
> +BlockdevAmendJob *s;
> +const char *fmt = BlockdevDriver_str(options->driver);
> +BlockDriver *drv = bdrv_find_format(fmt);
> +BlockDriverState *bs = bdrv_find_node(node_name);

bdrv_find_node() can return NULL (we check for this
in almost all callsites)...

> +if (bs->drv != drv) {

...but here we dereference it unconditionally.

> +error_setg(errp,
> +   "x-blockdev-amend doesn't support changing the block 
> driver");
> +return;
> +}

thanks
-- PMM

Re: [PULL 00/12] Block patches

[Kevin Wolf]

Am 08.07.2020 um 00:05 hat Eduardo Habkost geschrieben:
> On Tue, Jul 07, 2020 at 05:28:21PM +0200, Philippe Mathieu-Daudé wrote:
> > On 6/26/20 12:25 PM, Stefan Hajnoczi wrote:
> > > On Thu, Jun 25, 2020 at 02:31:14PM +0100, Peter Maydell wrote:
> > >> On Wed, 24 Jun 2020 at 11:02, Stefan Hajnoczi  
> > >> wrote:
> > >>>
> > >>> The following changes since commit 
> > >>> 171199f56f5f9bdf1e5d670d09ef1351d8f01bae:
> > >>>
> > >>>   Merge remote-tracking branch 
> > >>> 'remotes/alistair/tags/pull-riscv-to-apply-20200619-3' into staging 
> > >>> (2020-06-22 14:45:25 +0100)
> > >>>
> > >>> are available in the Git repository at:
> > >>>
> > >>>   https://github.com/stefanha/qemu.git tags/block-pull-request
> > >>>
> > >>> for you to fetch changes up to 7838c67f22a81fcf669785cd6c0876438422071a:
> > >>>
> > >>>   block/nvme: support nested aio_poll() (2020-06-23 15:46:08 +0100)
> > >>>
> > >>> 
> > >>> Pull request
> > >>>
> > >>> 
> > >>
> > >> Failure on iotest 030, x86-64 Linux:
> > >>
> > >>   TESTiotest-qcow2: 030 [fail]
> > >> QEMU  --
> > >> "/home/petmay01/linaro/qemu-for-merges/build/alldbg/tests/qemu-iotests/../../x86_64-softmmu/qemu-system-x86_64"
> > >> -nodefaults -display none -accel qtest
> > >> QEMU_IMG  --
> > >> "/home/petmay01/linaro/qemu-for-merges/build/alldbg/tests/qemu-iotests/../../qemu-img"
> > >> QEMU_IO   --
> > >> "/home/petmay01/linaro/qemu-for-merges/build/alldbg/tests/qemu-iotests/../../qemu-io"
> > >>  --cache writeback --aio threads -f qcow2
> > >> QEMU_NBD  --
> > >> "/home/petmay01/linaro/qemu-for-merges/build/alldbg/tests/qemu-iotests/../../qemu-nbd"
> > >> IMGFMT-- qcow2 (compat=1.1)
> > >> IMGPROTO  -- file
> > >> PLATFORM  -- Linux/x86_64 e104462 4.15.0-76-generic
> > >> TEST_DIR  --
> > >> /home/petmay01/linaro/qemu-for-merges/build/alldbg/tests/qemu-iotests/scratch
> > >> SOCK_DIR  -- /tmp/tmp.8tgdDjoZcO
> > >> SOCKET_SCM_HELPER --
> > >> /home/petmay01/linaro/qemu-for-merges/build/alldbg/tests/qemu-iotest/socket_scm_helper
> > >>
> > >> --- /home/petmay01/linaro/qemu-for-merges/tests/qemu-iotests/030.out
> > >>  2019-07-15 17:18:35.251364738 +0100
> > >> +++ 
> > >> /home/petmay01/linaro/qemu-for-merges/build/alldbg/tests/qemu-iotests/030.out.bad
> > >>   2020-06-25 14:04:28.500534007 +0100
> > >> @@ -1,5 +1,17 @@
> > >> -...
> > >> +.F.
> > >> +==
> > >> +FAIL: test_stream_parallel (__main__.TestParallelOps)
> > >> +--
> > >> +Traceback (most recent call last):
> > >> +  File "030", line 246, in test_stream_parallel
> > >> +self.assert_qmp(result, 'return', {})
> > >> +  File 
> > >> "/home/petmay01/linaro/qemu-for-merges/tests/qemu-iotests/iotests.py",
> > >> line 848, in assert_qmp
> > >> +result = self.dictpath(d, path)
> > >> +  File 
> > >> "/home/petmay01/linaro/qemu-for-merges/tests/qemu-iotests/iotests.py",
> > >> line 822, in dictpath
> > >> +self.fail(f'failed path traversal for "{path}" in "{d}"')
> > >> +AssertionError: failed path traversal for "return" in "{'error':
> > >> {'class': 'DeviceNotActive', 'desc': "Block job 'stream-node8' not
> > >> found"}}"
> > >> +
> > >>  --
> > >>  Ran 27 tests
> > >>
> > >> -OK
> > >> +FAILED (failures=1)
> > > 
> > > Strange, I can't reproduce this failure on my pull request branch or on
> > > qemu.git/master.
> > > 
> > > Is this failure deterministic? Are you sure it is introduced by this
> > > pull request?
> > 
> > Probably not introduced by this pullreq, but I also hit it on FreeBSD:
> > https://cirrus-ci.com/task/4620718312783872?command=main#L5803
> > 
> >   TESTiotest-qcow2: 030 [fail]
> > QEMU  --
> > "/tmp/cirrus-ci-build/build/tests/qemu-iotests/../../aarch64-softmmu/qemu-system-aarch64"
> > -nodefaults -display none -machine virt -accel qtest
> > QEMU_IMG  --
> > "/tmp/cirrus-ci-build/build/tests/qemu-iotests/../../qemu-img"
> > QEMU_IO   --
> > "/tmp/cirrus-ci-build/build/tests/qemu-iotests/../../qemu-io"  --cache
> > writeback --aio threads -f qcow2
> > QEMU_NBD  --
> > "/tmp/cirrus-ci-build/build/tests/qemu-iotests/../../qemu-nbd"
> > IMGFMT-- qcow2 (compat=1.1)
> > IMGPROTO  -- file
> > PLATFORM  -- FreeBSD/amd64 cirrus-task-4620718312783872 12.1-RELEASE
> > TEST_DIR  -- /tmp/cirrus-ci-build/build/tests/qemu-iotests/scratch
> > SOCK_DIR  -- /tmp/tmp.aZ5pxFLF
> > SOCKET_SCM_HELPER --
> > --- /tmp/cirrus-ci-build/tests/qemu-iotests/030.out 2020-07-07
> > 14:48:48.123804000 +
> > +++ /tmp/cirrus-ci-build/build/tests/qemu-iotests/030.out.bad   
> > 2020-07-07
> > 15:05:07.863685000 +
> > @@ -1,5 +1,17 @@
> > 

Re: [PATCH v7 14/47] stream: Deal with filters

[Andrey Shinkevich]

On 25.06.2020 18:21, Max Reitz wrote:

Because of the (not so recent anymore) changes that make the stream job
independent of the base node and instead track the node above it, we
have to split that "bottom" node into two cases: The bottom COW node,
and the node directly above the base node (which may be an R/W filter
or the bottom COW node).

Signed-off-by: Max Reitz 
---
  qapi/block-core.json |  4 +++
  block/stream.c   | 63 
  blockdev.c   |  4 ++-
  3 files changed, 53 insertions(+), 18 deletions(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index b20332e592..df87855429 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2486,6 +2486,10 @@
  # On successful completion the image file is updated to drop the backing file
  # and the BLOCK_JOB_COMPLETED event is emitted.
  #
+# In case @device is a filter node, block-stream modifies the first non-filter
+# overlay node below it to point to base's backing node (or NULL if @base was
+# not specified) instead of modifying @device itself.
+#
  # @job-id: identifier for the newly-created block job. If
  #  omitted, the device name will be used. (Since 2.7)
  #
diff --git a/block/stream.c b/block/stream.c
index aa2e7af98e..b9c1141656 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -31,7 +31,8 @@ enum {
  
  typedef struct StreamBlockJob {

  BlockJob common;
-BlockDriverState *bottom;
+BlockDriverState *base_overlay; /* COW overlay (stream from this) */
+BlockDriverState *above_base;   /* Node directly above the base */


Keeping the base_overlay is enough to complete the stream job.

The above_base may disappear during the job and we can't rely on it.


  BlockdevOnError on_error;
  char *backing_file_str;
  bool bs_read_only;
@@ -53,7 +54,7 @@ static void stream_abort(Job *job)
  
  if (s->chain_frozen) {

  BlockJob *bjob = >common;
-bdrv_unfreeze_backing_chain(blk_bs(bjob->blk), s->bottom);
+bdrv_unfreeze_backing_chain(blk_bs(bjob->blk), s->above_base);
  }
  }
  
@@ -62,14 +63,15 @@ static int stream_prepare(Job *job)

  StreamBlockJob *s = container_of(job, StreamBlockJob, common.job);
  BlockJob *bjob = >common;
  BlockDriverState *bs = blk_bs(bjob->blk);
-BlockDriverState *base = backing_bs(s->bottom);
+BlockDriverState *unfiltered_bs = bdrv_skip_filters(bs);
+BlockDriverState *base = bdrv_filter_or_cow_bs(s->above_base);


The initial base node may be a top node for a concurrent commit job and

may disappear. It is true for the above_base as well.

base = bdrv_filter_or_cow_bs(s->base_overlay) is more reliable.


  Error *local_err = NULL;
  int ret = 0;
  
-bdrv_unfreeze_backing_chain(bs, s->bottom);

+bdrv_unfreeze_backing_chain(bs, s->above_base);
  s->chain_frozen = false;
  
-if (bs->backing) {

+if (bdrv_cow_child(unfiltered_bs)) {
  const char *base_id = NULL, *base_fmt = NULL;
  if (base) {
  base_id = s->backing_file_str;
@@ -77,8 +79,8 @@ static int stream_prepare(Job *job)
  base_fmt = base->drv->format_name;
  }
  }
-bdrv_set_backing_hd(bs, base, _err);
-ret = bdrv_change_backing_file(bs, base_id, base_fmt);
+bdrv_set_backing_hd(unfiltered_bs, base, _err);
+ret = bdrv_change_backing_file(unfiltered_bs, base_id, base_fmt);
  if (local_err) {
  error_report_err(local_err);
  return -EPERM;
@@ -109,14 +111,15 @@ static int coroutine_fn stream_run(Job *job, Error **errp)
  StreamBlockJob *s = container_of(job, StreamBlockJob, common.job);
  BlockBackend *blk = s->common.blk;
  BlockDriverState *bs = blk_bs(blk);
-bool enable_cor = !backing_bs(s->bottom);
+BlockDriverState *unfiltered_bs = bdrv_skip_filters(bs);
+bool enable_cor = !bdrv_cow_child(s->base_overlay);
  int64_t len;
  int64_t offset = 0;
  uint64_t delay_ns = 0;
  int error = 0;
  int64_t n = 0; /* bytes */
  
-if (bs == s->bottom) {

+if (unfiltered_bs == s->base_overlay) {
  /* Nothing to stream */
  return 0;
  }
@@ -150,13 +153,14 @@ static int coroutine_fn stream_run(Job *job, Error **errp)
  
  copy = false;
  
-ret = bdrv_is_allocated(bs, offset, STREAM_CHUNK, );

+ret = bdrv_is_allocated(unfiltered_bs, offset, STREAM_CHUNK, );
  if (ret == 1) {
  /* Allocated in the top, no need to copy.  */
  } else if (ret >= 0) {
  /* Copy if allocated in the intermediate images.  Limit to the
   * known-unallocated area [offset, offset+n*BDRV_SECTOR_SIZE).  */
-ret = bdrv_is_allocated_above(backing_bs(bs), s->bottom, true,
+ret = bdrv_is_allocated_above(bdrv_cow_bs(unfiltered_bs),
+  s->base_overlay, true,
   

Re: [PATCH 2/2] hw/sd/sdcard: Do not allow invalid SD card sizes

[Philippe Mathieu-Daudé]

On 7/9/20 4:15 PM, Peter Maydell wrote:
> On Thu, 9 Jul 2020 at 14:56, Philippe Mathieu-Daudé  wrote:
>>
>> On 7/7/20 10:29 PM, Niek Linnenbank wrote:
>>> So I manually copy & pasted the change into hw/sd/sd.c to test it.
>>> It looks like the check works, but my concern is that with this change,
>>> we will be getting this error on 'off-the-shelf' images as well.
>>> For example, the latest Raspbian image size also isn't a power of two:
>>>
>>> $ ./arm-softmmu/qemu-system-arm -M raspi2 -sd
>>> ~/Downloads/2020-05-27-raspios-buster-lite-armhf.img -nographic
>>> WARNING: Image format was not specified for
>>> '/home/me/Downloads/2020-05-27-raspios-buster-lite-armhf.img' and
>>> probing guessed raw.
>>>  Automatically detecting the format is dangerous for raw images,
>>> write operations on block 0 will be restricted.
>>>  Specify the 'raw' format explicitly to remove the restrictions.
>>> qemu-system-arm: Invalid SD card size: 1.73 GiB (expecting at least 2 GiB)
>>>
>>> If we do decide that the change is needed, I would like to propose that
>>> we also give the user some instructions
>>> on how to fix it, maybe some 'dd' command?
>>
>> On POSIX we can suggest to use 'truncate -s 2G' from coreutils.
>> This is not in the default Darwin packages.
>> On Windows I have no clue.
> 
> dd/truncate etc won't work if the image file is not raw (eg if
> it's qcow2).

Good catch...

> The only chance you have of something that's actually
> generic would probably involve "qemu-img resize". But I'm a bit
> wary of having an error message that recommends that, because
> what if we got it wrong?

I am not sure what to recommend then.

Would that work as hint?

  qemu-system-arm -M raspi2 -sd ./buster-lite-armhf.img
  qemu-system-arm: Invalid SD card size: 1.73 GiB
  SD card size has to be a power of 2, e.g. 2GiB.

Re: [PATCH 2/2] hw/sd/sdcard: Do not allow invalid SD card sizes

[Peter Maydell]

On Thu, 9 Jul 2020 at 14:56, Philippe Mathieu-Daudé  wrote:
>
> On 7/7/20 10:29 PM, Niek Linnenbank wrote:
> > So I manually copy & pasted the change into hw/sd/sd.c to test it.
> > It looks like the check works, but my concern is that with this change,
> > we will be getting this error on 'off-the-shelf' images as well.
> > For example, the latest Raspbian image size also isn't a power of two:
> >
> > $ ./arm-softmmu/qemu-system-arm -M raspi2 -sd
> > ~/Downloads/2020-05-27-raspios-buster-lite-armhf.img -nographic
> > WARNING: Image format was not specified for
> > '/home/me/Downloads/2020-05-27-raspios-buster-lite-armhf.img' and
> > probing guessed raw.
> >  Automatically detecting the format is dangerous for raw images,
> > write operations on block 0 will be restricted.
> >  Specify the 'raw' format explicitly to remove the restrictions.
> > qemu-system-arm: Invalid SD card size: 1.73 GiB (expecting at least 2 GiB)
> >
> > If we do decide that the change is needed, I would like to propose that
> > we also give the user some instructions
> > on how to fix it, maybe some 'dd' command?
>
> On POSIX we can suggest to use 'truncate -s 2G' from coreutils.
> This is not in the default Darwin packages.
> On Windows I have no clue.

dd/truncate etc won't work if the image file is not raw (eg if
it's qcow2). The only chance you have of something that's actually
generic would probably involve "qemu-img resize". But I'm a bit
wary of having an error message that recommends that, because
what if we got it wrong?

thanks
-- PMM

Re: [PATCH v10 31/34] qcow2: Add the 'extended_l2' option and the QCOW2_INCOMPAT_EXTL2 bit

[Max Reitz]

On 03.07.20 17:58, Alberto Garcia wrote:
> Now that the implementation of subclusters is complete we can finally
> add the necessary options to create and read images with this feature,
> which we call "extended L2 entries".
> 
> Signed-off-by: Alberto Garcia 
> Reviewed-by: Eric Blake 
> Reviewed-by: Max Reitz 
> ---

After running basically all test combinations I usually run, I noticed
that 191 should also be modified to drop the extended_l2 option from the
qemu-img info output (down under “checking image base” and “checking
image layer”, where there are sed //D calls already).  Otherwise, it’ll
break with compat=0.10 (for no good reason).

(It needs the format-specific info because it needs to check the
encryption information, so it just manually drops everything it doesn’t
need.)

Max




signature.asc
Description: OpenPGP digital signature

Re: [PATCH 2/2] hw/sd/sdcard: Do not allow invalid SD card sizes

[Philippe Mathieu-Daudé]

On 7/7/20 10:29 PM, Niek Linnenbank wrote:
> Hi Philippe,
> 
> Just tried out your patch on latest master, and I noticed I couldn't
> apply it without getting this error:
> 
> $ git am ~/Downloads/patches/\[PATCH\ 2_2\]\ hw_sd_sdcard\:\ Do\ not\
> allow\ invalid\ SD\ card\ sizes\ -\ Philippe\ Mathieu-Daudé\
> \mailto:f4...@amsat.org>\>\ -\ 2020-07-07\ 1521.eml
> Applying: hw/sd/sdcard: Do not allow invalid SD card sizes
> error: patch failed: hw/sd/sd.c:2130
> error: hw/sd/sd.c: patch does not apply
> Patch failed at 0001 hw/sd/sdcard: Do not allow invalid SD card sizes
> Use 'git am --show-current-patch' to see the failed patch
> When you have resolved this problem, run "git am --continue".
> If you prefer to skip this patch, run "git am --skip" instead.
> To restore the original branch and stop patching, run "git am --abort".
> 
> The first patch did go OK. Maybe this one just needs to be rebased, or I
> made a mistake.

Sorry it was not clear on the cover:

  Part 1 is already reviewed:
  https://www.mail-archive.com/qemu-devel@nongnu.org/msg718150.html
  Based-on: <20200630133912.9428-1-f4...@amsat.org>

This series is based on the "Part 1".

> So I manually copy & pasted the change into hw/sd/sd.c to test it.
> It looks like the check works, but my concern is that with this change,
> we will be getting this error on 'off-the-shelf' images as well.
> For example, the latest Raspbian image size also isn't a power of two:
> 
> $ ./arm-softmmu/qemu-system-arm -M raspi2 -sd
> ~/Downloads/2020-05-27-raspios-buster-lite-armhf.img -nographic
> WARNING: Image format was not specified for
> '/home/me/Downloads/2020-05-27-raspios-buster-lite-armhf.img' and
> probing guessed raw.
>          Automatically detecting the format is dangerous for raw images,
> write operations on block 0 will be restricted.
>          Specify the 'raw' format explicitly to remove the restrictions.
> qemu-system-arm: Invalid SD card size: 1.73 GiB (expecting at least 2 GiB)
> 
> If we do decide that the change is needed, I would like to propose that
> we also give the user some instructions
> on how to fix it, maybe some 'dd' command?

On POSIX we can suggest to use 'truncate -s 2G' from coreutils.
This is not in the default Darwin packages.
On Windows I have no clue.

> In my opinion that should
> also go in some of the documentation file(s),
> possibly also in the one for the OrangePi PC at
> docs/system/arm/orangepi.rst (I can also provide a patch for that if you
> wish).

Good idea, if you can send that patch that would a precious help,
and I'd include it with the other patches :)

Note that this was your orangepi-pc acceptance test that catched
this bug!
See https://travis-ci.org/github/philmd/qemu/jobs/705653532#L5672:

 CPU: ARMv7 Processor [410fc075] revision 5 (ARMv7), cr=50c5387d
 OF: fdt: Machine model: Xunlong Orange Pi PC
 Kernel command line: printk.time=0 console=ttyS0,115200
root=/dev/mmcblk0 rootwait rw panic=-1 noreboot
 sunxi-mmc 1c0f000.mmc: Linked as a consumer to regulator.2
 sunxi-mmc 1c0f000.mmc: Got CD GPIO
 sunxi-mmc 1c0f000.mmc: initialized, max. request size: 16384 KB
 mmc0: host does not support reading read-only switch, assuming write-enable
 mmc0: Problem switching card into high-speed mode!
 mmc0: new SD card at address 4567
 mmcblk0: mmc0:4567 QEMU! 60.0 MiB
 EXT4-fs (mmcblk0): mounting ext2 file system using the ext4 subsystem
 EXT4-fs (mmcblk0): mounted filesystem without journal. Opts: (null)
 VFS: Mounted root (ext2 filesystem) on device 179:0.
 EXT4-fs (mmcblk0): re-mounted. Opts: block_validity,barrier,user_xattr,acl
 Populating /dev using udev: udevd[204]: starting version 3.2.7
 udevadm settle failed
 done
 udevd[205]: worker [208]
/devices/platform/soc/1c0f000.mmc/mmc_host/mmc0/mmc0:4567/block/mmcblk0
is taking a long time
Runner error occurred: Timeout reached
Original status: ERROR

(I'll add that in the commit description too).

Thanks for your testing/review!

> Kind regards,
> 
> Niek
> 
> 
> On Tue, Jul 7, 2020 at 6:11 PM Philippe Mathieu-Daudé  > wrote:
> 
> On 7/7/20 6:06 PM, Peter Maydell wrote:
> > On Tue, 7 Jul 2020 at 17:04, Alistair Francis
> mailto:alistai...@gmail.com>> wrote:
> >>
> >> On Tue, Jul 7, 2020 at 6:22 AM Philippe Mathieu-Daudé
> mailto:f4...@amsat.org>> wrote:
> >>>
> >>> QEMU allows to create SD card with unrealistic sizes. This could
> work,
> >>> but some guests (at least Linux) consider sizes that are not a power
> >>> of 2 as a firmware bug and fix the card size to the next power of 2.
> >>>
> >>> Before CVE-2020-13253 fix, this would allow OOB read/write accesses
> >>> past the image size end.
> >>>
> >>> CVE-2020-13253 has been fixed as:
> >>>
> >>>     Read command is rejected if BLOCK_LEN_ERROR or ADDRESS_ERROR
> >>>     occurred and no data transfer is performed.
> >>>
> >>>     Write command is rejected if BLOCK_LEN_ERROR or ADDRESS_ERROR
> >>>  

[PATCH] block: Avoid stale pointer dereference in blk_get_aio_context()

[Greg Kurz]

It is possible for blk_remove_bs() to race with blk_drain_all(), causing
the latter to dereference a stale blk->root pointer:


  blk_remove_bs(blk)
   bdrv_root_unref_child(blk->root)
child_bs = blk->root->bs
bdrv_detach_child(blk->root)
 ...
 g_free(blk->root) <== blk->root becomes stale
bdrv_unref(child_bs) < yield at some point

A blk_drain_all() can be triggered by some guest action in the
meantime, eg. on POWER, SLOF might disable bus mastering on
a virtio-scsi-pci device:

  virtio_write_config()
   virtio_pci_stop_ioeventfd()
virtio_bus_stop_ioeventfd()
 virtio_scsi_dataplane_stop()
  blk_drain_all()
   blk_get_aio_context()
   bs = blk->root ? blk->root->bs : NULL
^
  stale

Then, depending on one's luck, QEMU either crashes with SEGV or
hits the assertion in blk_get_aio_context().

blk->root is set by blk_insert_bs() which calls bdrv_root_attach_child()
first. The blk_remove_bs() function should rollback the changes made
by blk_insert_bs() in the opposite order (or it should be documented
somewhere why this isn't the case). Clear blk->root before calling
bdrv_root_unref_child() in blk_remove_bs().

Signed-off-by: Greg Kurz 
---
 block/block-backend.c |4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/block/block-backend.c b/block/block-backend.c
index 6936b25c836c..0bf0188133e3 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -808,6 +808,7 @@ void blk_remove_bs(BlockBackend *blk)
 {
 ThrottleGroupMember *tgm = >public.throttle_group_member;
 BlockDriverState *bs;
+BdrvChild *root;
 
 notifier_list_notify(>remove_bs_notifiers, blk);
 if (tgm->throttle_state) {
@@ -825,8 +826,9 @@ void blk_remove_bs(BlockBackend *blk)
  * to avoid that and a potential QEMU crash.
  */
 blk_drain(blk);
-bdrv_root_unref_child(blk->root);
+root = blk->root;
 blk->root = NULL;
+bdrv_root_unref_child(root);
 }
 
 /*

Re: [PATCH v10 31/34] qcow2: Add the 'extended_l2' option and the QCOW2_INCOMPAT_EXTL2 bit

[Alberto Garcia]

On Thu 09 Jul 2020 03:07:29 PM CEST, Max Reitz wrote:
> On 03.07.20 17:58, Alberto Garcia wrote:
>> Now that the implementation of subclusters is complete we can finally
>> add the necessary options to create and read images with this feature,
>> which we call "extended L2 entries".
>> 
>> Signed-off-by: Alberto Garcia 
>> Reviewed-by: Eric Blake 
>> Reviewed-by: Max Reitz 
>> ---
>
> (This requires quite a bit of a rebase, but nothing spectacularly
> interesting, I think.)

Yeah I noticed, I can send v11 after the rebase.

Berto

[PATCH 4/6] block, migration: add bdrv_finalize_vmstate helper

[Denis V. Lunev]

Right now bdrv_fclose() is just calling bdrv_flush().

The problem is that migration code is working inefficiently from block
layer terms and are frequently called for very small pieces of
unaligned data. Block layer is capable to work this way, but this is very
slow.

This patch is a preparation for the introduction of the intermediate
buffer at block driver state. It would be beneficial to separate
conventional bdrv_flush() from closing QEMU file from migration code.

The patch also forces bdrv_finalize_vmstate() operation inside
synchronous blk_save_vmstate() operation. This helper is used from
qemu-io only.

Signed-off-by: Denis V. Lunev 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
CC: Kevin Wolf 
CC: Max Reitz 
CC: Stefan Hajnoczi 
CC: Fam Zheng 
CC: Juan Quintela 
CC: "Dr. David Alan Gilbert" 
CC: Denis Plotnikov 
---
 block/block-backend.c |  6 +-
 block/io.c| 15 +++
 include/block/block.h |  5 +
 migration/savevm.c|  4 
 4 files changed, 29 insertions(+), 1 deletion(-)

diff --git a/block/block-backend.c b/block/block-backend.c
index 1c6e53bbde..5bb11c8e01 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -2177,16 +2177,20 @@ int blk_truncate(BlockBackend *blk, int64_t offset, 
bool exact,
 int blk_save_vmstate(BlockBackend *blk, const uint8_t *buf,
  int64_t pos, int size)
 {
-int ret;
+int ret, ret2;
 
 if (!blk_is_available(blk)) {
 return -ENOMEDIUM;
 }
 
 ret = bdrv_save_vmstate(blk_bs(blk), buf, pos, size);
+ret2 = bdrv_finalize_vmstate(blk_bs(blk));
 if (ret < 0) {
 return ret;
 }
+if (ret2 < 0) {
+return ret2;
+}
 
 if (!blk->enable_write_cache) {
 ret = bdrv_flush(blk_bs(blk));
diff --git a/block/io.c b/block/io.c
index b6564e34c5..be1fac0be8 100644
--- a/block/io.c
+++ b/block/io.c
@@ -2724,6 +2724,21 @@ int bdrv_readv_vmstate(BlockDriverState *bs, 
QEMUIOVector *qiov, int64_t pos)
 return bdrv_rw_vmstate(bs, qiov, pos, true);
 }
 
+static int coroutine_fn bdrv_co_finalize_vmstate(BlockDriverState *bs)
+{
+return 0;
+}
+
+static int coroutine_fn bdrv_finalize_vmstate_co_entry(void *opaque)
+{
+return bdrv_co_finalize_vmstate(opaque);
+}
+
+int bdrv_finalize_vmstate(BlockDriverState *bs)
+{
+return bdrv_run_co(bs, bdrv_finalize_vmstate_co_entry, bs);
+}
+
 /**/
 /* async I/Os */
 
diff --git a/include/block/block.h b/include/block/block.h
index bca3bb831c..124eb8d422 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -567,6 +567,11 @@ int bdrv_save_vmstate(BlockDriverState *bs, const uint8_t 
*buf,
 
 int bdrv_load_vmstate(BlockDriverState *bs, uint8_t *buf,
   int64_t pos, int size);
+/*
+ * bdrv_finalize_vmstate() is mandatory to commit vmstate changes if
+ * bdrv_save_vmstate() was ever called.
+ */
+int bdrv_finalize_vmstate(BlockDriverState *bs);
 
 void bdrv_img_create(const char *filename, const char *fmt,
  const char *base_filename, const char *base_fmt,
diff --git a/migration/savevm.c b/migration/savevm.c
index 45c9dd9d8a..d8a94e312c 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -150,6 +150,10 @@ static ssize_t block_get_buffer(void *opaque, uint8_t 
*buf, int64_t pos,
 
 static int bdrv_fclose(void *opaque, Error **errp)
 {
+int err = bdrv_finalize_vmstate(opaque);
+if (err < 0) {
+return err;
+}
 return bdrv_flush(opaque);
 }
 
-- 
2.17.1

[PATCH 2/6] block/aio_task: drop aio_task_pool_wait_one() helper

[Denis V. Lunev]

It is not used outside the module.

Actually there are 2 kind of waiters:
- for a slot and
- for all tasks to finish
This patch limits external API to listed types.

Signed-off-by: Denis V. Lunev 
Suggested-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
CC: Kevin Wolf 
CC: Max Reitz 
CC: Stefan Hajnoczi 
CC: Fam Zheng 
CC: Juan Quintela 
CC: "Dr. David Alan Gilbert" 
CC: Denis Plotnikov 
---
 block/aio_task.c | 13 ++---
 include/block/aio_task.h |  1 -
 2 files changed, 2 insertions(+), 12 deletions(-)

diff --git a/block/aio_task.c b/block/aio_task.c
index cf62e5c58b..7ba15ff41f 100644
--- a/block/aio_task.c
+++ b/block/aio_task.c
@@ -54,26 +54,17 @@ static void coroutine_fn aio_task_co(void *opaque)
 qemu_co_queue_restart_all(>waiters);
 }
 
-void coroutine_fn aio_task_pool_wait_one(AioTaskPool *pool)
-{
-assert(pool->busy_tasks > 0);
-
-qemu_co_queue_wait(>waiters, NULL);
-
-assert(pool->busy_tasks < pool->max_busy_tasks);
-}
-
 void coroutine_fn aio_task_pool_wait_slot(AioTaskPool *pool)
 {
 while (pool->busy_tasks >= pool->max_busy_tasks) {
-aio_task_pool_wait_one(pool);
+qemu_co_queue_wait(>waiters, NULL);
 }
 }
 
 void coroutine_fn aio_task_pool_wait_all(AioTaskPool *pool)
 {
 while (pool->busy_tasks > 0) {
-aio_task_pool_wait_one(pool);
+qemu_co_queue_wait(>waiters, NULL);
 }
 }
 
diff --git a/include/block/aio_task.h b/include/block/aio_task.h
index 50bc1e1817..50b1c036c5 100644
--- a/include/block/aio_task.h
+++ b/include/block/aio_task.h
@@ -48,7 +48,6 @@ bool aio_task_pool_empty(AioTaskPool *pool);
 void coroutine_fn aio_task_pool_start_task(AioTaskPool *pool, AioTask *task);
 
 void coroutine_fn aio_task_pool_wait_slot(AioTaskPool *pool);
-void coroutine_fn aio_task_pool_wait_one(AioTaskPool *pool);
 void coroutine_fn aio_task_pool_wait_all(AioTaskPool *pool);
 
 #endif /* BLOCK_AIO_TASK_H */
-- 
2.17.1

[PATCH 6/6] block/io: improve loadvm performance

[Denis V. Lunev]

This patch creates intermediate buffer for reading from block driver
state and performs read-ahead to this buffer. Snapshot code performs
reads sequentially and thus we know what offsets will be required
and when they will become not needed.

Results are fantastic. Switch to snapshot times of 2GB Fedora 31 VM
over NVME storage are the following:
original fixed
cached:  1.84s   1.16s
non-cached: 12.74s   1.27s

The difference over HDD would be more significant :)

Signed-off-by: Denis V. Lunev 
CC: Vladimir Sementsov-Ogievskiy 
CC: Kevin Wolf 
CC: Max Reitz 
CC: Stefan Hajnoczi 
CC: Fam Zheng 
CC: Juan Quintela 
CC: Denis Plotnikov 
---
 block/block-backend.c |  12 +-
 block/io.c| 239 +-
 include/block/block_int.h |   3 +
 3 files changed, 250 insertions(+), 4 deletions(-)

diff --git a/block/block-backend.c b/block/block-backend.c
index 5bb11c8e01..09773b3e37 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -2201,11 +2201,21 @@ int blk_save_vmstate(BlockBackend *blk, const uint8_t 
*buf,
 
 int blk_load_vmstate(BlockBackend *blk, uint8_t *buf, int64_t pos, int size)
 {
+int ret, ret2;
+
 if (!blk_is_available(blk)) {
 return -ENOMEDIUM;
 }
 
-return bdrv_load_vmstate(blk_bs(blk), buf, pos, size);
+ret = bdrv_load_vmstate(blk_bs(blk), buf, pos, size);
+ret2 = bdrv_finalize_vmstate(blk_bs(blk));
+if (ret < 0) {
+return ret;
+}
+if (ret2 < 0) {
+return ret2;
+}
+return ret;
 }
 
 int blk_probe_blocksizes(BlockBackend *blk, BlockSizes *bsz)
diff --git a/block/io.c b/block/io.c
index 061d3239b9..510122fbc4 100644
--- a/block/io.c
+++ b/block/io.c
@@ -2739,6 +2739,194 @@ static int bdrv_co_do_save_vmstate(BlockDriverState 
*bs, QEMUIOVector *qiov,
 }
 }
 
+
+typedef struct BdrvLoadVMChunk {
+void *buf;
+uint64_t offset;
+ssize_t bytes;
+
+QLIST_ENTRY(BdrvLoadVMChunk) list;
+} BdrvLoadVMChunk;
+
+typedef struct BdrvLoadVMState {
+AioTaskPool *pool;
+
+int64_t offset;
+int64_t last_loaded;
+
+int chunk_count;
+QLIST_HEAD(, BdrvLoadVMChunk) chunks;
+QLIST_HEAD(, BdrvLoadVMChunk) loading;
+CoQueue waiters;
+} BdrvLoadVMState;
+
+typedef struct BdrvLoadVMStateTask {
+AioTask task;
+
+BlockDriverState *bs;
+BdrvLoadVMChunk *chunk;
+} BdrvLoadVMStateTask;
+
+static BdrvLoadVMChunk *bdrv_co_find_loadvmstate_chunk(int64_t pos,
+   BdrvLoadVMChunk *c)
+{
+for (; c != NULL; c = QLIST_NEXT(c, list)) {
+if (c->offset <= pos && c->offset + c->bytes > pos) {
+return c;
+}
+}
+
+return NULL;
+}
+
+static void bdrv_free_loadvm_chunk(BdrvLoadVMChunk *c)
+{
+qemu_vfree(c->buf);
+g_free(c);
+}
+
+static coroutine_fn int bdrv_co_vmstate_load_task_entry(AioTask *task)
+{
+int err = 0;
+BdrvLoadVMStateTask *t = container_of(task, BdrvLoadVMStateTask, task);
+BdrvLoadVMChunk *c = t->chunk;
+BdrvLoadVMState *state = t->bs->loadvm_state;
+QEMUIOVector qiov = QEMU_IOVEC_INIT_BUF(qiov, c->buf, c->bytes);
+
+bdrv_inc_in_flight(t->bs);
+err = t->bs->drv->bdrv_load_vmstate(t->bs, , c->offset);
+bdrv_dec_in_flight(t->bs);
+
+QLIST_REMOVE(c, list);
+if (err == 0) {
+QLIST_INSERT_HEAD(>chunks, c, list);
+} else {
+bdrv_free_loadvm_chunk(c);
+}
+qemu_co_queue_restart_all(>waiters);
+
+return err;
+}
+
+
+static void bdrv_co_loadvmstate_next(BlockDriverState *bs, BdrvLoadVMChunk *c)
+{
+BdrvLoadVMStateTask *t = g_new(BdrvLoadVMStateTask, 1);
+BdrvLoadVMState *state = bs->loadvm_state;
+
+c->offset = state->last_loaded;
+
+*t = (BdrvLoadVMStateTask) {
+.task.func = bdrv_co_vmstate_load_task_entry,
+.bs = bs,
+.chunk = c,
+};
+
+QLIST_INSERT_HEAD(>loading, t->chunk, list);
+state->chunk_count++;
+state->last_loaded += c->bytes;
+
+aio_task_pool_start_task(state->pool, >task);
+}
+
+
+static void bdrv_co_loadvmstate_start(BlockDriverState *bs)
+{
+int i;
+size_t buf_size = MAX(bdrv_get_cluster_size(bs), 1 * MiB);
+
+for (i = 0; i < BDRV_VMSTATE_WORKERS_MAX; i++) {
+BdrvLoadVMChunk *c = g_new0(BdrvLoadVMChunk, 1);
+
+c->buf = qemu_blockalign(bs, buf_size);
+c->bytes = buf_size;
+
+bdrv_co_loadvmstate_next(bs, c);
+}
+}
+
+static int bdrv_co_do_load_vmstate(BlockDriverState *bs, QEMUIOVector *qiov,
+   int64_t pos)
+{
+BdrvLoadVMState *state = bs->loadvm_state;
+BdrvLoadVMChunk *c;
+size_t off;
+int64_t start_pos = pos;
+
+if (state == NULL) {
+if (pos != 0) {
+goto slow_path;
+}
+
+state = g_new(BdrvLoadVMState, 1);
+*state = (BdrvLoadVMState) {
+.pool = aio_task_pool_new(BDRV_VMSTATE_WORKERS_MAX),
+.chunks = 

[PATCH 5/6] block/io: improve savevm performance

[Denis V. Lunev]

This patch does 2 standard basic things:
- it creates intermediate buffer for all writes from QEMU migration code
  to block driver,
- this buffer is sent to disk asynchronously, allowing several writes to
  run in parallel.

Thus bdrv_vmstate_write() is becoming asynchronous. All pending operations
completion are performed in newly invented bdrv_finalize_vmstate().

In general, migration code is fantastically inefficent (by observation),
buffers are not aligned and sent with arbitrary pieces, a lot of time
less than 100 bytes at a chunk, which results in read-modify-write
operations if target file descriptor is opened with O_DIRECT. It should
also be noted that all operations are performed into unallocated image
blocks, which also suffer due to partial writes to such new clusters
even on cached file descriptors.

Snapshot creation time (2 GB Fedora-31 VM running over NVME storage):
original fixed
cached:  1.79s   1.27s
non-cached:  3.29s   0.81s

The difference over HDD would be more significant :)

Signed-off-by: Denis V. Lunev 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
CC: Kevin Wolf 
CC: Max Reitz 
CC: Stefan Hajnoczi 
CC: Fam Zheng 
CC: Juan Quintela 
CC: "Dr. David Alan Gilbert" 
CC: Denis Plotnikov 
---
 block/io.c| 126 +-
 include/block/block_int.h |   8 +++
 2 files changed, 132 insertions(+), 2 deletions(-)

diff --git a/block/io.c b/block/io.c
index be1fac0be8..061d3239b9 100644
--- a/block/io.c
+++ b/block/io.c
@@ -26,6 +26,7 @@
 #include "trace.h"
 #include "sysemu/block-backend.h"
 #include "block/aio-wait.h"
+#include "block/aio_task.h"
 #include "block/blockjob.h"
 #include "block/blockjob_int.h"
 #include "block/block_int.h"
@@ -33,6 +34,7 @@
 #include "qapi/error.h"
 #include "qemu/error-report.h"
 #include "qemu/main-loop.h"
+#include "qemu/units.h"
 #include "sysemu/replay.h"
 
 /* Maximum bounce buffer for copy-on-read and write zeroes, in bytes */
@@ -2640,6 +2642,103 @@ typedef struct BdrvVmstateCo {
 boolis_read;
 } BdrvVmstateCo;
 
+typedef struct BdrvVMStateTask {
+AioTask task;
+
+BlockDriverState *bs;
+int64_t offset;
+void *buf;
+size_t bytes;
+} BdrvVMStateTask;
+
+typedef struct BdrvSaveVMState {
+AioTaskPool *pool;
+BdrvVMStateTask *t;
+} BdrvSaveVMState;
+
+
+static coroutine_fn int bdrv_co_vmstate_save_task_entry(AioTask *task)
+{
+int err = 0;
+BdrvVMStateTask *t = container_of(task, BdrvVMStateTask, task);
+
+if (t->bytes != 0) {
+QEMUIOVector qiov = QEMU_IOVEC_INIT_BUF(qiov, t->buf, t->bytes);
+
+bdrv_inc_in_flight(t->bs);
+err = t->bs->drv->bdrv_save_vmstate(t->bs, , t->offset);
+bdrv_dec_in_flight(t->bs);
+}
+
+qemu_vfree(t->buf);
+return err;
+}
+
+static BdrvVMStateTask *bdrv_vmstate_task_create(BlockDriverState *bs,
+ int64_t pos, size_t size)
+{
+BdrvVMStateTask *t = g_new(BdrvVMStateTask, 1);
+
+*t = (BdrvVMStateTask) {
+.task.func = bdrv_co_vmstate_save_task_entry,
+.buf = qemu_blockalign(bs, size),
+.offset = pos,
+.bs = bs,
+};
+
+return t;
+}
+
+static int bdrv_co_do_save_vmstate(BlockDriverState *bs, QEMUIOVector *qiov,
+   int64_t pos)
+{
+BdrvSaveVMState *state = bs->savevm_state;
+BdrvVMStateTask *t;
+size_t buf_size = MAX(bdrv_get_cluster_size(bs), 1 * MiB);
+size_t to_copy, off;
+
+if (state == NULL) {
+state = g_new(BdrvSaveVMState, 1);
+*state = (BdrvSaveVMState) {
+.pool = aio_task_pool_new(BDRV_VMSTATE_WORKERS_MAX),
+.t = bdrv_vmstate_task_create(bs, pos, buf_size),
+};
+
+bs->savevm_state = state;
+}
+
+if (aio_task_pool_status(state->pool) < 0) {
+/*
+ * The operation as a whole is unsuccessful. Prohibit all futher
+ * operations. If we clean here, new useless ops will come again.
+ * Thus we rely on caller for cleanup here.
+ */
+return aio_task_pool_status(state->pool);
+}
+
+t = state->t;
+if (t->offset + t->bytes != pos) {
+/* Normally this branch is not reachable from migration */
+return bs->drv->bdrv_save_vmstate(bs, qiov, pos);
+}
+
+off = 0;
+while (1) {
+to_copy = MIN(qiov->size - off, buf_size - t->bytes);
+qemu_iovec_to_buf(qiov, off, t->buf + t->bytes, to_copy);
+t->bytes += to_copy;
+if (t->bytes < buf_size) {
+return 0;
+}
+
+aio_task_pool_start_task(state->pool, >task);
+
+pos += to_copy;
+off += to_copy;
+state->t = t = bdrv_vmstate_task_create(bs, pos, buf_size);
+}
+}
+
 static int coroutine_fn
 bdrv_co_rw_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t pos,
bool is_read)
@@ -2655,7 +2754,7 @@ 

[PATCH 1/6] block/aio_task: allow start/wait task from any coroutine

[Denis V. Lunev]

From: Vladimir Sementsov-Ogievskiy 

Currently, aio task pool assumes that there is a main coroutine, which
creates tasks and wait for them. Let's remove the restriction by using
CoQueue. Code becomes clearer, interface more obvious.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Signed-off-by: Denis V. Lunev 
CC: Kevin Wolf 
CC: Max Reitz 
CC: Stefan Hajnoczi 
CC: Fam Zheng 
CC: Juan Quintela 
CC: "Dr. David Alan Gilbert" 
CC: Vladimir Sementsov-Ogievskiy 
CC: Denis Plotnikov 
---
 block/aio_task.c | 21 ++---
 1 file changed, 6 insertions(+), 15 deletions(-)

diff --git a/block/aio_task.c b/block/aio_task.c
index 88989fa248..cf62e5c58b 100644
--- a/block/aio_task.c
+++ b/block/aio_task.c
@@ -27,11 +27,10 @@
 #include "block/aio_task.h"
 
 struct AioTaskPool {
-Coroutine *main_co;
 int status;
 int max_busy_tasks;
 int busy_tasks;
-bool waiting;
+CoQueue waiters;
 };
 
 static void coroutine_fn aio_task_co(void *opaque)
@@ -52,31 +51,23 @@ static void coroutine_fn aio_task_co(void *opaque)
 
 g_free(task);
 
-if (pool->waiting) {
-pool->waiting = false;
-aio_co_wake(pool->main_co);
-}
+qemu_co_queue_restart_all(>waiters);
 }
 
 void coroutine_fn aio_task_pool_wait_one(AioTaskPool *pool)
 {
 assert(pool->busy_tasks > 0);
-assert(qemu_coroutine_self() == pool->main_co);
 
-pool->waiting = true;
-qemu_coroutine_yield();
+qemu_co_queue_wait(>waiters, NULL);
 
-assert(!pool->waiting);
 assert(pool->busy_tasks < pool->max_busy_tasks);
 }
 
 void coroutine_fn aio_task_pool_wait_slot(AioTaskPool *pool)
 {
-if (pool->busy_tasks < pool->max_busy_tasks) {
-return;
+while (pool->busy_tasks >= pool->max_busy_tasks) {
+aio_task_pool_wait_one(pool);
 }
-
-aio_task_pool_wait_one(pool);
 }
 
 void coroutine_fn aio_task_pool_wait_all(AioTaskPool *pool)
@@ -98,8 +89,8 @@ AioTaskPool *coroutine_fn aio_task_pool_new(int 
max_busy_tasks)
 {
 AioTaskPool *pool = g_new0(AioTaskPool, 1);
 
-pool->main_co = qemu_coroutine_self();
 pool->max_busy_tasks = max_busy_tasks;
+qemu_co_queue_init(>waiters);
 
 return pool;
 }
-- 
2.17.1

[PATCH v8 0/6] block: seriously improve savevm/loadvm performance

[Denis V. Lunev]

This series do standard basic things:
- it creates intermediate buffer for all writes from QEMU migration code
  to QCOW2 image,
- this buffer is sent to disk asynchronously, allowing several writes to
  run in parallel.

In general, migration code is fantastically inefficent (by observation),
buffers are not aligned and sent with arbitrary pieces, a lot of time
less than 100 bytes at a chunk, which results in read-modify-write
operations with non-cached operations. It should also be noted that all
operations are performed into unallocated image blocks, which also suffer
due to partial writes to such new clusters.

This patch series is an implementation of idea discussed in the RFC
posted by Denis Plotnikov
https://lists.gnu.org/archive/html/qemu-devel/2020-04/msg01925.html
Results with this series over NVME are better than original code
original rfcthis
cached:  1.79s  2.38s   1.27s
non-cached:  3.29s  1.31s   0.81s

Changes from v7:
- dropped lock from LoadVMState
- fixed assert in last patch
- dropped patch 1 as queued

Changes from v6:
- blk_load_vmstate kludges added (patchew problem fixed)

Changes from v5:
- loadvm optimizations added with Vladimir comments included

Changes from v4:
- added patch 4 with blk_save_vmstate() cleanup
- added R-By
- bdrv_flush_vmstate -> bdrv_finalize_vmstate
- fixed return code of bdrv_co_do_save_vmstate
- fixed typos in comments (Eric, thanks!)
- fixed patchew warnings

Changes from v3:
- rebased to master
- added patch 3 which removes aio_task_pool_wait_one()
- added R-By to patch 1
- patch 4 is rewritten via bdrv_run_co
- error path in blk_save_vmstate() is rewritten to call bdrv_flush_vmstate
  unconditionally
- added some comments
- fixes initialization in bdrv_co_vmstate_save_task_entry as suggested

Changes from v2:
- code moved from QCOW2 level to generic block level
- created bdrv_flush_vmstate helper to fix 022, 029 tests
- added recursive for bs->file in bdrv_co_flush_vmstate (fix 267)
- fixed blk_save_vmstate helper
- fixed coroutine wait as Vladimir suggested with waiting fixes from me

Changes from v1:
- patchew warning fixed
- fixed validation that only 1 waiter is allowed in patch 1

Signed-off-by: Denis V. Lunev 
CC: Kevin Wolf 
CC: Max Reitz 
CC: Stefan Hajnoczi 
CC: Fam Zheng 
CC: Juan Quintela 
CC: "Dr. David Alan Gilbert" 
CC: Vladimir Sementsov-Ogievskiy 
CC: Denis Plotnikov 

[PATCH 3/6] block/block-backend: remove always true check from blk_save_vmstate

[Denis V. Lunev]

bdrv_save_vmstate() returns either error with negative return value or
size. Thus this check is useless.

Signed-off-by: Denis V. Lunev 
Suggested-by: Eric Blake 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
CC: Kevin Wolf 
CC: Max Reitz 
CC: Stefan Hajnoczi 
CC: Fam Zheng 
CC: Juan Quintela 
CC: "Dr. David Alan Gilbert" 
CC: Denis Plotnikov 
---
 block/block-backend.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/block-backend.c b/block/block-backend.c
index 6936b25c83..1c6e53bbde 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -2188,7 +2188,7 @@ int blk_save_vmstate(BlockBackend *blk, const uint8_t 
*buf,
 return ret;
 }
 
-if (ret == size && !blk->enable_write_cache) {
+if (!blk->enable_write_cache) {
 ret = bdrv_flush(blk_bs(blk));
 }
 
-- 
2.17.1

Re: [PATCH v10 34/34] iotests: Add tests for qcow2 images with extended L2 entries

[Max Reitz]

On 03.07.20 17:58, Alberto Garcia wrote:
> Signed-off-by: Alberto Garcia 
> ---
>  tests/qemu-iotests/271 | 901 +
>  tests/qemu-iotests/271.out | 724 +
>  tests/qemu-iotests/group   |   1 +
>  3 files changed, 1626 insertions(+)
>  create mode 100755 tests/qemu-iotests/271
>  create mode 100644 tests/qemu-iotests/271.out

[...]

> diff --git a/tests/qemu-iotests/271.out b/tests/qemu-iotests/271.out
> new file mode 100644
> index 00..07c1e7b46d
> --- /dev/null
> +++ b/tests/qemu-iotests/271.out
> @@ -0,0 +1,724 @@

[...]

> +### qemu-img amend ###
> +
> +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1048576
> +qemu-img: Toggling extended L2 entries is not supported
> +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1048576
> +qemu-img: Toggling extended L2 entries is not supported

With Maxim’s patches, this code in patch 31 became unnecessary, so it
should now read “qemu-img: Invalid parameter 'extended_l2'\nThis option
is only supported for image creation”.

With that fixed:

Reviewed-by: Max Reitz 



signature.asc
Description: OpenPGP digital signature

Re: [PATCH v10 31/34] qcow2: Add the 'extended_l2' option and the QCOW2_INCOMPAT_EXTL2 bit

[Max Reitz]

On 03.07.20 17:58, Alberto Garcia wrote:
> Now that the implementation of subclusters is complete we can finally
> add the necessary options to create and read images with this feature,
> which we call "extended L2 entries".
> 
> Signed-off-by: Alberto Garcia 
> Reviewed-by: Eric Blake 
> Reviewed-by: Max Reitz 
> ---

(This requires quite a bit of a rebase, but nothing spectacularly
interesting, I think.)



signature.asc
Description: OpenPGP digital signature

[PULL v2] Block layer patches

[Kevin Wolf]

The following changes since commit 8796c64ecdfd34be394ea277a53df0c76996:

  Merge remote-tracking branch 
'remotes/kraxel/tags/audio-20200706-pull-request' into staging (2020-07-08 
16:33:59 +0100)

are available in the Git repository at:

  git://repo.or.cz/qemu/kevin.git tags/for-upstream

for you to fetch changes up to d1c824e681e423a6224c6831a493e9175fa02dc1:

  qemu-img: Deprecate use of -b without -F (2020-07-09 14:14:55 +0200)


Block layer patches:

- file-posix: Mitigate file fragmentation with extent size hints
- Tighten qemu-img rules on missing backing format
- qemu-img map: Don't limit block status request size


Eric Blake (10):
  qemu-img: Flush stdout before before potential stderr messages
  block: Finish deprecation of 'qemu-img convert -n -o'
  sheepdog: Add trivial backing_fmt support
  vmdk: Add trivial backing_fmt support
  qcow: Tolerate backing_fmt=
  block: Error if backing file fails during creation without -u
  qcow2: Deprecate use of qemu-img amend to change backing file
  iotests: Specify explicit backing format where sensible
  block: Add support to warn on backing file change without format
  qemu-img: Deprecate use of -b without -F

Kevin Wolf (2):
  qemu-img map: Don't limit block status request size
  file-posix: Mitigate file fragmentation with extent size hints

Max Reitz (1):
  iotests: Simplify _filter_img_create() a bit

 qapi/block-core.json | 11 +++--
 docs/system/deprecated.rst   | 58 ++
 docs/tools/qemu-img.rst  |  4 ++
 include/block/block.h|  4 +-
 include/block/block_int.h|  1 +
 block.c  | 53 +---
 block/file-posix.c   | 42 +++
 block/qcow.c | 20 -
 block/qcow2.c|  7 +++-
 block/sheepdog.c | 18 +++-
 block/stream.c   |  2 +-
 block/vmdk.c | 14 +++
 blockdev.c   |  3 +-
 qemu-img.c   | 20 +
 tests/qemu-iotests/017   |  2 +-
 tests/qemu-iotests/017.out   |  2 +-
 tests/qemu-iotests/018   |  2 +-
 tests/qemu-iotests/018.out   |  2 +-
 tests/qemu-iotests/019   |  5 ++-
 tests/qemu-iotests/019.out   |  2 +-
 tests/qemu-iotests/020   | 31 --
 tests/qemu-iotests/020.out   | 15 +--
 tests/qemu-iotests/024   |  8 ++--
 tests/qemu-iotests/024.out   |  5 ++-
 tests/qemu-iotests/028   |  4 +-
 tests/qemu-iotests/028.out   |  2 +-
 tests/qemu-iotests/030   | 26 
 tests/qemu-iotests/034   |  2 +-
 tests/qemu-iotests/034.out   |  2 +-
 tests/qemu-iotests/037   |  2 +-
 tests/qemu-iotests/037.out   |  2 +-
 tests/qemu-iotests/038   |  2 +-
 tests/qemu-iotests/038.out   |  2 +-
 tests/qemu-iotests/039   |  3 +-
 tests/qemu-iotests/039.out   |  2 +-
 tests/qemu-iotests/040   | 47 +++--
 tests/qemu-iotests/041   | 37 +++--
 tests/qemu-iotests/042   |  4 +-
 tests/qemu-iotests/043   | 18 
 tests/qemu-iotests/043.out   | 16 +---
 tests/qemu-iotests/046   |  2 +-
 tests/qemu-iotests/046.out   |  2 +-
 tests/qemu-iotests/049.out   |  8 ++--
 tests/qemu-iotests/050   |  4 +-
 tests/qemu-iotests/050.out   |  2 +-
 tests/qemu-iotests/051   |  2 +-
 tests/qemu-iotests/051.out   |  2 +-
 tests/qemu-iotests/051.pc.out|  2 +-
 tests/qemu-iotests/054.out   |  2 +-
 tests/qemu-iotests/056   |  3 +-
 tests/qemu-iotests/060   |  2 +-
 tests/qemu-iotests/060.out   |  2 +-
 tests/qemu-iotests/061   | 10 ++---
 tests/qemu-iotests/061.out   | 11 ++---
 tests/qemu-iotests/069   |  2 +-
 tests/qemu-iotests/069.out   |  2 +-
 tests/qemu-iotests/073   |  2 +-
 tests/qemu-iotests/073.out   |  2 +-
 tests/qemu-iotests/079.out   |  2 +-
 tests/qemu-iotests/082   | 10 +++--
 tests/qemu-iotests/082.out   | 30 +++---
 tests/qemu-iotests/085   |  4 +-
 tests/qemu-iotests/085.out   |  6 +--
 tests/qemu-iotests/089   |  2 +-
 tests/qemu-iotests/089.out   |  2 +-
 tests/qemu-iotests/095   |  4 +-
 tests/qemu-iotests/095.out   |  4 +-
 tests/qemu-iotests/097   |  4 +-
 tests/qemu-iotests/097.out   | 16 
 tests/qemu-iotests/098   |  2 +-
 tests/qemu-iotests/098.out   |  8 ++--
 tests/qemu-iotests/110   |  4 +-
 tests/qemu-iotests/110.out   |  4 +-
 tests/qemu-iotests/111.out   |  2 +-
 tests/qemu-iotests/112.out   |  4 +-
 tests/qemu-iotests/114   | 12 ++
 tests/qemu-iotests/114.out   | 

Re: [PATCH] iotests: Simplify _filter_img_create() a bit

[Kevin Wolf]

Am 09.07.2020 um 13:02 hat Max Reitz geschrieben:
> Not only is it a bit stupid to try to filter multi-line "Formatting"
> output (because we only need it for a single test, which can easily be
> amended to no longer need it), it is also problematic when there can be
> output after a "Formatting" line that we do not want to filter as if it
> were part of it.
> 
> So rename _filter_img_create to _do_filter_img_create, let it filter
> only a single line, and let _filter_img_create loop over all input
> lines, calling _do_filter_img_create only on those that match
> /^Formatting/ (basically, what _filter_img_create_in_qmp did already).
> (And fix 020 to work with that.)
> 
> Reported-by: Kevin Wolf 
> Signed-off-by: Max Reitz 
> ---
> Kevin noted that the changes to _filter_img_create broke Eric's patch to
> flush the Formatting line out before a potential error message.  This
> patch should fix it (and the diff stat is negative, so that's nice).

Thanks, this fixed the problem. Applied to the block branch.

Kevin

Re: [PATCH v10 28/34] qcow2: Add subcluster support to qcow2_co_pwrite_zeroes()

[Max Reitz]

On 03.07.20 17:58, Alberto Garcia wrote:
> This works now at the subcluster level and pwrite_zeroes_alignment is
> updated accordingly.
> 
> qcow2_cluster_zeroize() is turned into qcow2_subcluster_zeroize() with
> the following changes:
> 
>- The request can now be subcluster-aligned.
> 
>- The cluster-aligned body of the request is still zeroized using
>  zero_in_l2_slice() as before.
> 
>- The subcluster-aligned head and tail of the request are zeroized
>  with the new zero_l2_subclusters() function.
> 
> There is just one thing to take into account for a possible future
> improvement: compressed clusters cannot be partially zeroized so
> zero_l2_subclusters() on the head or the tail can return -ENOTSUP.
> This makes the caller repeat the *complete* request and write actual
> zeroes to disk. This is sub-optimal because
> 
>1) if the head area was compressed we would still be able to use
>   the fast path for the body and possibly the tail.
> 
>2) if the tail area was compressed we are writing zeroes to the
>   head and the body areas, which are already zeroized.
> 
> Signed-off-by: Alberto Garcia 
> ---
>  block/qcow2.h |  4 +--
>  block/qcow2-cluster.c | 81 +++
>  block/qcow2.c | 33 +-
>  3 files changed, 94 insertions(+), 24 deletions(-)

Reviewed-by: Max Reitz 



signature.asc
Description: OpenPGP digital signature

Re: [PATCH v10 21/34] qcow2: Add subcluster support to qcow2_get_host_offset()

[Max Reitz]

On 03.07.20 17:58, Alberto Garcia wrote:
> The logic of this function remains pretty much the same, except that
> it uses count_contiguous_subclusters(), which combines the logic of
> count_contiguous_clusters() / count_contiguous_clusters_unallocated()
> and checks individual subclusters.
> 
> qcow2_cluster_to_subcluster_type() is not necessary as a separate
> function anymore so it's inlined into its caller.
> 
> Signed-off-by: Alberto Garcia 
> ---
>  block/qcow2.h |  38 ---
>  block/qcow2-cluster.c | 150 ++
>  2 files changed, 92 insertions(+), 96 deletions(-)

Reviewed-by: Max Reitz 



signature.asc
Description: OpenPGP digital signature

[PATCH] iotests: Simplify _filter_img_create() a bit

[Max Reitz]

Not only is it a bit stupid to try to filter multi-line "Formatting"
output (because we only need it for a single test, which can easily be
amended to no longer need it), it is also problematic when there can be
output after a "Formatting" line that we do not want to filter as if it
were part of it.

So rename _filter_img_create to _do_filter_img_create, let it filter
only a single line, and let _filter_img_create loop over all input
lines, calling _do_filter_img_create only on those that match
/^Formatting/ (basically, what _filter_img_create_in_qmp did already).
(And fix 020 to work with that.)

Reported-by: Kevin Wolf 
Signed-off-by: Max Reitz 
---
Kevin noted that the changes to _filter_img_create broke Eric's patch to
flush the Formatting line out before a potential error message.  This
patch should fix it (and the diff stat is negative, so that's nice).
---
 tests/qemu-iotests/020   | 29 ---
 tests/qemu-iotests/020.out   | 13 +--
 tests/qemu-iotests/141   |  2 +-
 tests/qemu-iotests/common.filter | 62 ++--
 4 files changed, 45 insertions(+), 61 deletions(-)

diff --git a/tests/qemu-iotests/020 b/tests/qemu-iotests/020
index 20f8f185d0..b488000cb9 100755
--- a/tests/qemu-iotests/020
+++ b/tests/qemu-iotests/020
@@ -115,18 +115,23 @@ TEST_IMG="$TEST_IMG.base" _make_test_img 1M
 # Create an image with a null backing file to which committing will fail (with
 # ENOSPC so we can distinguish the result from some generic EIO which may be
 # generated anywhere in the block layer)
-_make_test_img -b "json:{'driver': '$IMGFMT',
- 'file': {
- 'driver': 'blkdebug',
- 'inject-error': [{
- 'event': 'write_aio',
- 'errno': 28,
- 'once': true
- }],
- 'image': {
- 'driver': 'file',
- 'filename': '$TEST_IMG.base'
- }}}"
+backing="json:{'driver': '$IMGFMT',
+   'file': {
+   'driver': 'blkdebug',
+   'inject-error': [{
+   'event': 'write_aio',
+   'errno': 28,
+   'once': true
+   }],
+   'image': {
+   'driver': 'file',
+   'filename': '$TEST_IMG.base'
+   }}}"
+
+# Filter out newlines and collapse spaces
+backing=$(echo "$backing" | tr -d '\n' | tr -s ' ')
+
+_make_test_img -b "$backing"
 
 # Just write anything so committing will not be a no-op
 $QEMU_IO -c 'writev 0 64k' "$TEST_IMG" | _filter_qemu_io
diff --git a/tests/qemu-iotests/020.out b/tests/qemu-iotests/020.out
index 4b722b2dd0..4668ac59df 100644
--- a/tests/qemu-iotests/020.out
+++ b/tests/qemu-iotests/020.out
@@ -1079,18 +1079,7 @@ No errors were found on the image.
 Testing failing commit
 
 Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=1048576
-Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1048576 
backing_file=json:{'driver': 'IMGFMT',,
- 'file': {
- 'driver': 'blkdebug',,
- 'inject-error': [{
- 'event': 'write_aio',,
- 'errno': 28,,
- 'once': true
- }],,
- 'image': {
- 'driver': 'file',,
- 'filename': 'TEST_DIR/t.IMGFMT.base'
- }}}
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1048576 
backing_file=json:{'driver': 'IMGFMT',, 'file': { 'driver': 'blkdebug',, 
'inject-error': [{ 'event': 'write_aio',, 'errno': 28,, 'once': true }],, 
'image': { 'driver': 'file',, 'filename': 'TEST_DIR/t.IMGFMT.base' }}}
 wrote 65536/65536 bytes at offset 0
 64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 qemu-img: Block job failed: No space left on device
diff --git a/tests/qemu-iotests/141 b/tests/qemu-iotests/141
index 6d1b7b0d4c..5192d256e3 100755
--- a/tests/qemu-iotests/141
+++ b/tests/qemu-iotests/141
@@ -68,7 +68,7 @@ test_blockjob()
 _send_qemu_cmd $QEMU_HANDLE \
 "$1" \
 "$2" \
-| _filter_img_create_in_qmp | _filter_qmp_empty_return
+| _filter_img_create | _filter_qmp_empty_return
 
 # We want this to return an error because the block job is still running
 _send_qemu_cmd $QEMU_HANDLE \
diff --git a/tests/qemu-iotests/common.filter b/tests/qemu-iotests/common.filter
index d967adc59a..3833206327 100644
--- a/tests/qemu-iotests/common.filter
+++ b/tests/qemu-iotests/common.filter
@@ -119,8 +119,21 @@ _filter_actual_image_size()
 $SED -s 's/\("actual-size":\s*\)[0-9]\+/\1SIZE/g'
 }
 
+# Filename filters for 

Re: [PULL 00/12] Block layer patches

[Kevin Wolf]

Am 07.07.2020 um 18:34 hat Kevin Wolf geschrieben:
> The following changes since commit 7623b5ba017f61de5d7c2bba12c6feb3d55091b1:
> 
>   Merge remote-tracking branch 
> 'remotes/vivier2/tags/linux-user-for-5.1-pull-request' into staging 
> (2020-07-06 11:40:10 +0100)
> 
> are available in the Git repository at:
> 
>   git://repo.or.cz/qemu/kevin.git tags/for-upstream
> 
> for you to fetch changes up to 7bf114070834e1b0c947b7c2a1c96cb734eb6b86:
> 
>   qemu-img: Deprecate use of -b without -F (2020-07-07 18:18:06 +0200)
> 
> 
> Block layer patches:
> 
> - file-posix: Mitigate file fragmentation with extent size hints
> - Tighten qemu-img rules on missing backing format
> - qemu-img map: Don't limit block status request size

This has merge conflicts with Max's pull request that has been merged
since I sent this one, so I'll have to send a v2 once they are resolved
(unfortunately one of the conflicts is not completely trivial).

Kevin

Re: [PATCH 1/7] migration/savevm: respect qemu_fclose() error code in save_snapshot()

[Juan Quintela]

"Denis V. Lunev"  wrote:
> qemu_fclose() could return error, f.e. if bdrv_co_flush() will return
> the error.
>
> This validation will become more important once we will start waiting of
> asynchronous IO operations, started from bdrv_write_vmstate(), which are
> coming soon.
>
> Signed-off-by: Denis V. Lunev 
> Reviewed-by: "Dr. David Alan Gilbert" 
> Reviewed-by: Vladimir Sementsov-Ogievskiy 
> CC: Kevin Wolf 
> CC: Max Reitz 
> CC: Stefan Hajnoczi 
> CC: Fam Zheng 
> CC: Juan Quintela 
> CC: Denis Plotnikov 

Reviewed-by: Juan Quintela 

queued


> ---
>  migration/savevm.c | 8 ++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/migration/savevm.c b/migration/savevm.c
> index b979ea6e7f..da3dead4e9 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -2628,7 +2628,7 @@ int save_snapshot(const char *name, Error **errp)
>  {
>  BlockDriverState *bs, *bs1;
>  QEMUSnapshotInfo sn1, *sn = , old_sn1, *old_sn = _sn1;
> -int ret = -1;
> +int ret = -1, ret2;
>  QEMUFile *f;
>  int saved_vm_running;
>  uint64_t vm_state_size;
> @@ -2712,10 +2712,14 @@ int save_snapshot(const char *name, Error **errp)
>  }
>  ret = qemu_savevm_state(f, errp);
>  vm_state_size = qemu_ftell(f);
> -qemu_fclose(f);
> +ret2 = qemu_fclose(f);
>  if (ret < 0) {
>  goto the_end;
>  }
> +if (ret2 < 0) {
> +ret = ret2;
> +goto the_end;
> +}
>  
>  /* The bdrv_all_create_snapshot() call that follows acquires the 
> AioContext
>   * for itself.  BDRV_POLL_WHILE() does not support nested locking because

Re: [PATCH v10 02/34] qcow2: Convert qcow2_get_cluster_offset() into qcow2_get_host_offset()

[Max Reitz]

On 03.07.20 17:57, Alberto Garcia wrote:
> qcow2_get_cluster_offset() takes an (unaligned) guest offset and
> returns the (aligned) offset of the corresponding cluster in the qcow2
> image.
> 
> In practice none of the callers need to know where the cluster starts
> so this patch makes the function calculate and return the final host
> offset directly. The function is also renamed accordingly.
> 
> There is a pre-existing exception with compressed clusters: in this
> case the function returns the complete cluster descriptor (containing
> the offset and size of the compressed data). This does not change with
> this patch but it is now documented.
> 
> Signed-off-by: Alberto Garcia 
> ---
>  block/qcow2.h |  4 ++--
>  block/qcow2-cluster.c | 41 +++--
>  block/qcow2.c | 24 +++-
>  3 files changed, 32 insertions(+), 37 deletions(-)

Reviewed-by: Max Reitz 



signature.asc
Description: OpenPGP digital signature

Re: [PATCH v7 12/47] block: Use bdrv_filter_(bs|child) where obvious

[Andrey Shinkevich]

On 09.07.2020 11:59, Max Reitz wrote:

On 08.07.20 20:24, Andrey Shinkevich wrote:

On 25.06.2020 18:21, Max Reitz wrote:

Places that use patterns like

  if (bs->drv->is_filter && bs->file) {
  ... something about bs->file->bs ...
  }

should be

  BlockDriverState *filtered = bdrv_filter_bs(bs);
  if (filtered) {
  ... something about @filtered ...
  }

instead.

Signed-off-by: Max Reitz 
---
   block.c    | 31 ---
   block/io.c |  7 +--
   migration/block-dirty-bitmap.c |  8 +---
   3 files changed, 26 insertions(+), 20 deletions(-)


...

diff --git a/block/io.c b/block/io.c
index df8f2a98d4..385176b331 100644
--- a/block/io.c
+++ b/block/io.c
@@ -3307,6 +3307,7 @@ int coroutine_fn bdrv_co_truncate(BdrvChild
*child, int64_t offset, bool exact,
     Error **errp)
   {
   BlockDriverState *bs = child->bs;
+    BdrvChild *filtered;
   BlockDriver *drv = bs->drv;
   BdrvTrackedRequest req;
   int64_t old_size, new_bytes;
@@ -3358,6 +3359,8 @@ int coroutine_fn bdrv_co_truncate(BdrvChild
*child, int64_t offset, bool exact,
   goto out;
   }
   +    filtered = bdrv_filter_child(bs);
+

Isn't better to have this initialization right before the relevant
if/else block?

Hm, well, yes.  In this case, though, maybe not.  Patch 16 will add
another BdrvChild to be initialized here (@backing), and we need to
initialize that one here.  So I felt it made sense to group them together.

They got split up when I decided to put @filtered into this patch and
@backing into its own.  So now it may look a bit weird, but I feel like
after patch 16 it makes sense.

(I’m indifferent, basically.)

Max


Yes, it makes a sence. I am on the way to reviewing further and have not 
reached the 16th yet.


It is a minor thing anyway )) Thank you for your response.

Andrey

Re: [PATCH v7 02/47] block: Add chain helper functions

[Andrey Shinkevich]

On 09.07.2020 11:24, Max Reitz wrote:

On 08.07.20 19:20, Andrey Shinkevich wrote:

On 25.06.2020 18:21, Max Reitz wrote:

Add some helper functions for skipping filters in a chain of block
nodes.

Signed-off-by: Max Reitz 
---
   include/block/block_int.h |  3 +++
   block.c   | 55 +++
   2 files changed, 58 insertions(+)

diff --git a/include/block/block_int.h b/include/block/block_int.h
index bb3457c5e8..5da793bfc3 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -1382,6 +1382,9 @@ BdrvChild *bdrv_cow_child(BlockDriverState *bs);
   BdrvChild *bdrv_filter_child(BlockDriverState *bs);
   BdrvChild *bdrv_filter_or_cow_child(BlockDriverState *bs);
   BdrvChild *bdrv_primary_child(BlockDriverState *bs);
+BlockDriverState *bdrv_skip_implicit_filters(BlockDriverState *bs);
+BlockDriverState *bdrv_skip_filters(BlockDriverState *bs);
+BlockDriverState *bdrv_backing_chain_next(BlockDriverState *bs);
     static inline BlockDriverState *child_bs(BdrvChild *child)
   {
diff --git a/block.c b/block.c
index 5a42ef49fd..0a0b855261 100644
--- a/block.c
+++ b/block.c
@@ -7008,3 +7008,58 @@ BdrvChild *bdrv_primary_child(BlockDriverState
*bs)
     return NULL;
   }
+
+static BlockDriverState *bdrv_do_skip_filters(BlockDriverState *bs,
+  bool
stop_on_explicit_filter)
+{
+    BdrvChild *c;
+
+    if (!bs) {
+    return NULL;
+    }
+
+    while (!(stop_on_explicit_filter && !bs->implicit)) {
+    c = bdrv_filter_child(bs);
+    if (!c) {
+    break;
+    }
+    bs = c->bs;

Could it be child_bs(bs) ?

Well, in a sense, but not really.  We need to check whether there is a
child before overwriting @bs (because @bs must stay a non-NULL pointer),
so we wouldn’t have fewer lines of code if we replaced “BdrvChild *c” by
“BlockDriverState *child_bs”, and then used bdrv_child() to set child_bs.

(And because we have to check whether @c is NULL anyway, there is no
real reason to use child_bs(c) instead of c->bs afterwards.)


Got it, thanks.

Andrey


+    }

Reviewed-by: Andrey Shinkevich 

Thanks a lot for reviewing!


Pleasure!

Andrey

Re: [PATCH v7 00/47] block: Deal with filters

[Andrey Shinkevich]

On 09.07.2020 11:20, Max Reitz wrote:

On 08.07.20 22:47, Eric Blake wrote:

On 7/8/20 12:20 PM, Andrey Shinkevich wrote:

On 25.06.2020 18:21, Max Reitz wrote:

v6:
https://lists.nongnu.org/archive/html/qemu-devel/2019-08/msg01715.html

Branch: https://github.com/XanClic/qemu.git child-access-functions-v7
Branch: https://git.xanclic.moe/XanClic/qemu.git
child-access-functions-v7



I cloned the branch from the github and built successfully.

Running the iotests reports multiple errors of such a kind:

128: readarray -td '' formatting_line < <(sed -e 's/, fmt=/\x0/')

"./common.filter: line 128: readarray: -d: invalid option"

introduced with the commit

a7399eb iotests: Make _filter_img_create more active

You appear to be staging off an unreleased preliminary tree.  a7399eb is
not upstream; the upstream commit 'iotests: Make _filter_img_create more
active' is commit 57ee95ed, and while it uses readarray, it does not use
the problematic -d.  In other words, it looks like the problem was
caught and fixed in between the original patch creation and the pull
request.

Ah, sorry, my mail client’s threading layout hid this mail from me a bit.

Yes.  Well, no, it wasn’t fixed before the pull request, but it was
fixed in the second pull request.  But yes.

Max


I'm clear with it now. Thank you all for your explenations and time!

Andrey

Re: [PATCH v7 12/47] block: Use bdrv_filter_(bs|child) where obvious

[Max Reitz]

On 08.07.20 20:24, Andrey Shinkevich wrote:
> On 25.06.2020 18:21, Max Reitz wrote:
>> Places that use patterns like
>>
>>  if (bs->drv->is_filter && bs->file) {
>>  ... something about bs->file->bs ...
>>  }
>>
>> should be
>>
>>  BlockDriverState *filtered = bdrv_filter_bs(bs);
>>  if (filtered) {
>>  ... something about @filtered ...
>>  }
>>
>> instead.
>>
>> Signed-off-by: Max Reitz 
>> ---
>>   block.c    | 31 ---
>>   block/io.c |  7 +--
>>   migration/block-dirty-bitmap.c |  8 +---
>>   3 files changed, 26 insertions(+), 20 deletions(-)
>>
> ...
>> diff --git a/block/io.c b/block/io.c
>> index df8f2a98d4..385176b331 100644
>> --- a/block/io.c
>> +++ b/block/io.c
>> @@ -3307,6 +3307,7 @@ int coroutine_fn bdrv_co_truncate(BdrvChild
>> *child, int64_t offset, bool exact,
>>     Error **errp)
>>   {
>>   BlockDriverState *bs = child->bs;
>> +    BdrvChild *filtered;
>>   BlockDriver *drv = bs->drv;
>>   BdrvTrackedRequest req;
>>   int64_t old_size, new_bytes;
>> @@ -3358,6 +3359,8 @@ int coroutine_fn bdrv_co_truncate(BdrvChild
>> *child, int64_t offset, bool exact,
>>   goto out;
>>   }
>>   +    filtered = bdrv_filter_child(bs);
>> +
> 
> Isn't better to have this initialization right before the relevant
> if/else block?

Hm, well, yes.  In this case, though, maybe not.  Patch 16 will add
another BdrvChild to be initialized here (@backing), and we need to
initialize that one here.  So I felt it made sense to group them together.

They got split up when I decided to put @filtered into this patch and
@backing into its own.  So now it may look a bit weird, but I feel like
after patch 16 it makes sense.

(I’m indifferent, basically.)

Max



signature.asc
Description: OpenPGP digital signature

Re: [PATCH v7 02/47] block: Add chain helper functions

[Max Reitz]

On 08.07.20 19:20, Andrey Shinkevich wrote:
> On 25.06.2020 18:21, Max Reitz wrote:
>> Add some helper functions for skipping filters in a chain of block
>> nodes.
>>
>> Signed-off-by: Max Reitz 
>> ---
>>   include/block/block_int.h |  3 +++
>>   block.c   | 55 +++
>>   2 files changed, 58 insertions(+)
>>
>> diff --git a/include/block/block_int.h b/include/block/block_int.h
>> index bb3457c5e8..5da793bfc3 100644
>> --- a/include/block/block_int.h
>> +++ b/include/block/block_int.h
>> @@ -1382,6 +1382,9 @@ BdrvChild *bdrv_cow_child(BlockDriverState *bs);
>>   BdrvChild *bdrv_filter_child(BlockDriverState *bs);
>>   BdrvChild *bdrv_filter_or_cow_child(BlockDriverState *bs);
>>   BdrvChild *bdrv_primary_child(BlockDriverState *bs);
>> +BlockDriverState *bdrv_skip_implicit_filters(BlockDriverState *bs);
>> +BlockDriverState *bdrv_skip_filters(BlockDriverState *bs);
>> +BlockDriverState *bdrv_backing_chain_next(BlockDriverState *bs);
>>     static inline BlockDriverState *child_bs(BdrvChild *child)
>>   {
>> diff --git a/block.c b/block.c
>> index 5a42ef49fd..0a0b855261 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -7008,3 +7008,58 @@ BdrvChild *bdrv_primary_child(BlockDriverState
>> *bs)
>>     return NULL;
>>   }
>> +
>> +static BlockDriverState *bdrv_do_skip_filters(BlockDriverState *bs,
>> +  bool
>> stop_on_explicit_filter)
>> +{
>> +    BdrvChild *c;
>> +
>> +    if (!bs) {
>> +    return NULL;
>> +    }
>> +
>> +    while (!(stop_on_explicit_filter && !bs->implicit)) {
>> +    c = bdrv_filter_child(bs);
>> +    if (!c) {
>> +    break;
>> +    }
>> +    bs = c->bs;
> 
> Could it be child_bs(bs) ?

Well, in a sense, but not really.  We need to check whether there is a
child before overwriting @bs (because @bs must stay a non-NULL pointer),
so we wouldn’t have fewer lines of code if we replaced “BdrvChild *c” by
“BlockDriverState *child_bs”, and then used bdrv_child() to set child_bs.

(And because we have to check whether @c is NULL anyway, there is no
real reason to use child_bs(c) instead of c->bs afterwards.)

>> +    }
> Reviewed-by: Andrey Shinkevich 

Thanks a lot for reviewing!



signature.asc
Description: OpenPGP digital signature

Re: [PATCH v7 00/47] block: Deal with filters

[Max Reitz]

On 08.07.20 22:47, Eric Blake wrote:
> On 7/8/20 12:20 PM, Andrey Shinkevich wrote:
>> On 25.06.2020 18:21, Max Reitz wrote:
>>> v6:
>>> https://lists.nongnu.org/archive/html/qemu-devel/2019-08/msg01715.html
>>>
>>> Branch: https://github.com/XanClic/qemu.git child-access-functions-v7
>>> Branch: https://git.xanclic.moe/XanClic/qemu.git
>>> child-access-functions-v7
>>>
>>>
>> I cloned the branch from the github and built successfully.
>>
>> Running the iotests reports multiple errors of such a kind:
>>
>> 128: readarray -td '' formatting_line < <(sed -e 's/, fmt=/\x0/')
>>
>> "./common.filter: line 128: readarray: -d: invalid option"
>>
>> introduced with the commit
>>
>> a7399eb iotests: Make _filter_img_create more active
> 
> You appear to be staging off an unreleased preliminary tree.  a7399eb is
> not upstream; the upstream commit 'iotests: Make _filter_img_create more
> active' is commit 57ee95ed, and while it uses readarray, it does not use
> the problematic -d.  In other words, it looks like the problem was
> caught and fixed in between the original patch creation and the pull
> request.

Ah, sorry, my mail client’s threading layout hid this mail from me a bit.

Yes.  Well, no, it wasn’t fixed before the pull request, but it was
fixed in the second pull request.  But yes.

Max



signature.asc
Description: OpenPGP digital signature

Re: [PATCH v7 00/47] block: Deal with filters

[Max Reitz]

On 08.07.20 22:37, Eric Blake wrote:
> On 7/8/20 2:46 PM, Andrey Shinkevich wrote:
>>
>> On 08.07.2020 20:32, Eric Blake wrote:
>>> On 7/8/20 12:20 PM, Andrey Shinkevich wrote:
 On 25.06.2020 18:21, Max Reitz wrote:
> v6:
> https://lists.nongnu.org/archive/html/qemu-devel/2019-08/msg01715.html
>
> Branch: https://github.com/XanClic/qemu.git child-access-functions-v7
> Branch: https://git.xanclic.moe/XanClic/qemu.git
> child-access-functions-v7
>
>
 I cloned the branch from the github and built successfully.

 Running the iotests reports multiple errors of such a kind:

 128: readarray -td '' formatting_line < <(sed -e 's/, fmt=/\x0/')

 "./common.filter: line 128: readarray: -d: invalid option"

>>>
>>> Arrgh. If I'm reading bash's changelog correctly, readarray -d was
>>> introduced in bash 4.4, so I'm guessing you're still on 4.3 or
>>> earlier? What bash version and platform are you using?
>>>
>> My bash version is 4.2.46.
>>
>> It is the latest in the virtuozzolinux-base repository. I should
>> install the 4.4 package manually.
> 
> Well, if bash 4.2 is the default installed version on any of our
> platforms that meet our supported criteria, then we should instead fix
> the patch in question to avoid non-portable use of readarray.
> 
> Per https://repology.org/project/bash/versions (hinted from
> docs/system/build-platforms.rst), at least CentOS 7 still ships bash
> 4.2, and per 'make docker', centos7 is still a viable build target.  So
> we do indeed need to fix our regression.

There is no regression.  It’s just that I based this series on an
earlier version of “Make _filter_img_create more active” – when I sent a
pull request for that version, Peter already reported to me that it
failed on some test environments, so I revised it.

You’ll find there is no a7399eb in master; it’s 57ee95ed4ee there and
doesn’t use -d.

(My branch on github/gitea is still based on that older version, though,
because that’s what I wrote it on.)

Max



signature.asc
Description: OpenPGP digital signature

Re: [PATCH v3 01/18] hw/block/nvme: bump spec data structures to v1.3

[Klaus Jensen]

On Jul  8 21:47, Dmitry Fomichev wrote:
> 
> > -Original Message-
> > From: Klaus Jensen 
> > Sent: Wednesday, July 8, 2020 5:24 PM
> > To: Dmitry Fomichev 
> > Cc: qemu-block@nongnu.org; qemu-de...@nongnu.org; f...@euphon.net;
> > javier.g...@samsung.com; kw...@redhat.com; mre...@redhat.com;
> > mlevi...@redhat.com; phi...@redhat.com; kbu...@kernel.org;
> > k.jen...@samsung.com
> > Subject: Re: [PATCH v3 01/18] hw/block/nvme: bump spec data structures to
> > v1.3
> > 
> > On Jul  8 19:19, Dmitry Fomichev wrote:
> > > Looks good with a small nit (see below),
> > >
> > > Reviewed-by: Dmitry Fomichev 
> > >
> > > >
> > > On Mon, 2020-07-06 at 08:12 +0200, Klaus Jensen wrote:
> > > > +#define NVME_TEMP_TMPTH(temp) ((temp >>  0) & 0x)
> > >
> > > There is an extra space after temp >>
> > >
> > 
> > Good catch! I won't repost for this ;) - but I'll fix it and add it in
> > the qemu-nvme tree.
> 
> Yes, no need to repost :) Thanks for reviewing our ZNS series! I am working
> on addressing your comments and I am also starting to review your
> "AIO and address mapping refactoring" patchset.
> 

Since I think this patchset gets merged on nvme-next today, there is a
v2 on the way for that set.