date:20210401

[Question] qemu-img convert block alignment

2021-04-01 Thread Zhenyu Ye

Hi all,

commit 8dcd3c9b91 ("qemu-img: align result of is_allocated_sectors")
introduces block alignment when doing qemu-img convert. However, the
alignment is:

s.alignment = MAX(pow2floor(s.min_sparse),
  DIV_ROUND_UP(out_bs->bl.request_alignment,
   BDRV_SECTOR_SIZE));

(where the default s.min_sparse is 8)
When the target device's bl.request_alignment is smaller than 4K, this
will cause additional write-zero overhead and makes the size of target
file larger.

Is this as expected?  Should we change the MAX() to MIN()?


Thanks,
zhenyu

Re: [PATCH 0/6] Add debug interface to kick/call on purpose

2021-04-01 Thread Jason Wang




在 2021/3/30 下午3:29, Dongli Zhang 写道:


On 3/28/21 8:56 PM, Jason Wang wrote:

在 2021/3/27 上午5:16, Dongli Zhang 写道:

Hi Jason,

On 3/26/21 12:24 AM, Jason Wang wrote:

在 2021/3/26 下午1:44, Dongli Zhang 写道:

The virtio device/driver (e.g., vhost-scsi or vhost-net) may hang due to
the loss of doorbell kick, e.g.,

https://urldefense.com/v3/__https://lists.gnu.org/archive/html/qemu-devel/2018-12/msg01711.html__;!!GqivPVa7Brio!KS3pAU2cKjz4wgI4QSlE-YsJPhPG71nkE5_tGhaOf7mi_xvNxbvKkfn03rk5BNDLSEU$


... or due to the loss of IRQ, e.g., as fixed by linux kernel commit
fe200ae48ef5 ("genirq: Mark polled irqs and defer the real handler").

This patch introduces a new debug interface 'DeviceEvent' to DeviceClass
to help narrow down if the issue is due to loss of irq/kick. So far the new
interface handles only two events: 'call' and 'kick'. Any device (e.g.,
virtio/vhost or VFIO) may implement the interface (e.g., via eventfd, MSI-X
or legacy IRQ).

The 'call' is to inject irq on purpose by admin for a specific device (e.g.,
vhost-scsi) from QEMU/host to VM, while the 'kick' is to kick the doorbell
on purpose by admin at QEMU/host side for a specific device.


This device can be used as a workaround if call/kick is lost due to
virtualization software (e.g., kernel or QEMU) issue.

We may also implement the interface for VFIO PCI, e.g., to write to
VFIOPCIDevice->msi_vectors[i].interrupt will be able to inject IRQ to VM
on purpose. This is considered future work once the virtio part is done.


Below is from live crash analysis. Initially, the queue=2 has count=15 for
'kick' eventfd_ctx. Suppose there is data in vring avail while there is no
used available. We suspect this is because vhost-scsi was not notified by
VM. In order to narrow down and analyze the issue, we use live crash to
dump the current counter of eventfd for queue=2.

crash> eventfd_ctx 8f67f6bbe700
struct eventfd_ctx {
     kref = {
   refcount = {
     refs = {
   counter = 4
     }
   }
     },
     wqh = {
   lock = {
     {
   rlock = {
     raw_lock = {
   val = {
     counter = 0
   }
     }
   }
     }
   },
   head = {
     next = 0x8f841dc08e18,
     prev = 0x8f841dc08e18
   }
     },
     count = 15, ---> eventfd is 15 !!!
     flags = 526336
}

Now we kick the doorbell for vhost-scsi queue=2 on purpose for diagnostic
with this interface.

{ "execute": "x-debug-device-event",
     "arguments": { "dev": "/machine/peripheral/vscsi0",
    "event": "kick", "queue": 2 } }

The counter is increased to 16. Suppose the hang issue is resolved, it
indicates something bad is in software that the 'kick' is lost.

What do you mean by "software" here? And it looks to me you're testing whether
event_notifier_set() is called by virtio_queue_notify() here. If so, I'm not
sure how much value could we gain from a dedicated debug interface like this
consider there're a lot of exisinting general purpose debugging method like
tracing or gdb. I'd say the path from virtio_queue_notify() to
event_notifier_set() is only a very small fraction of the process of virtqueue
kick which is unlikey to be buggy. Consider usually the ioeventfd will be
offloaded to KVM, it's more a chance that something is wrong in setuping
ioeventfd instead of here. Irq is even more complicated.

Thank you very much!

I am not testing whether event_notifier_set() is called by 
virtio_queue_notify().

The 'software' indicates the data processing and event notification mechanism
involved with virtio/vhost PV driver frontend. E.g., while VM is waiting for an
extra IRQ, vhost side did not trigger IRQ, suppose vring_need_event()
erroneously returns false due to corrupted ring buffer status.


So there could be several factors that may block the notification:

1) eventfd bug (ioeventfd vs irqfd)
2) wrong virtqueue state (either driver or device)
3) missing barriers (either driver or device)
4) Qemu bug (irqchip and routing)
...

This is not only about whether notification is blocked.

It can also be used to help narrow down and understand if there is any
suspicious issue in blk-mq/scsi/netdev/napi code. The PV drivers are not only
drivers following virtio spec. It is closely related to many of other kernel
components.

Suppose IO was recovered after we inject an IRQ to vhost-scsi on purpose, we
will be able to analyze what may happen along the IO completion path starting
from when /where the IRQ is injected ... perhaps the root cause is not with
virtio but blk-mq/scsi (this is just an example).


In addition, this idea should help for vfio-pci. Suppose the developer for a
specific device driver suspects IO/networking hangs because of loss if IRQ, we
will be able to verify if that assumption is correct by injecting an IRQ on 
purpose.

Therefore, this is not only about virtio PV driver (frontend/backend), but also
used to help analyze the issue

Re: [BUG FIX][PATCH v3 0/3] vhost-user-blk: fix bug on device disconnection during initialization

2021-04-01 Thread Valentin Sinitsyn


On 01.04.2021 14:21, Denis Plotnikov wrote:

This is a series fixing a bug in host-user-blk.

More specifically, it's not just a bug but crasher.

Valentine


Is there any chance for it to be considered for the next rc?

Thanks!

Denis

On 29.03.2021 16:44, Denis Plotnikov wrote:


ping!

On 25.03.2021 18:12, Denis Plotnikov wrote:

v3:
   * 0003: a new patch added fixing the problem on vm shutdown
 I stumbled on this bug after v2 sending.
   * 0001: gramma fixing (Raphael)
   * 0002: commit message fixing (Raphael)

v2:
   * split the initial patch into two (Raphael)
   * rename init to realized (Raphael)
   * remove unrelated comment (Raphael)

When the vhost-user-blk device lose the connection to the daemon during
the initialization phase it kills qemu because of the assert in the code.
The series fixes the bug.

0001 is preparation for the fix
0002 fixes the bug, patch description has the full motivation for the series
0003 (added in v3) fix bug on vm shutdown

Denis Plotnikov (3):
   vhost-user-blk: use different event handlers on initialization
   vhost-user-blk: perform immediate cleanup if disconnect on
 initialization
   vhost-user-blk: add immediate cleanup on shutdown

  hw/block/vhost-user-blk.c | 79 ---
  1 file changed, 48 insertions(+), 31 deletions(-)

General question about parsing an rbd filename

2021-04-01 Thread Connor Kuehl


Hi,

block/rbd.c hints that:


 * Configuration values containing :, @, or = can be escaped with a
 * leading "\".


Right now, much of the parsing code will allow anyone to escape 
_anything_ so long as it's preceded by '\'.


Is this the intended behavior? Or should the parser be updated to allow 
escaping only certain sequences.


Just curious.

Thanks,

Connor

[PATCH v2 1/2] iotests/231: Update expected deprecation message

2021-04-01 Thread Connor Kuehl

The deprecation message in the expected output has technically been
wrong since the wrong version of a patch was applied to it. Because of
this, the test fails. Correct the expected output so that it passes.

Signed-off-by: Connor Kuehl 
Reviewed-by: Max Reitz 
---
Reworded the commit log and added Max's R-b.

 tests/qemu-iotests/231.out | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/tests/qemu-iotests/231.out b/tests/qemu-iotests/231.out
index 579ba11c16..747dd221bb 100644
--- a/tests/qemu-iotests/231.out
+++ b/tests/qemu-iotests/231.out
@@ -1,9 +1,7 @@
 QA output created by 231
-qemu-img: RBD options encoded in the filename as keyvalue pairs is deprecated. 
 Future versions may cease to parse these options in the future.
+qemu-img: warning: RBD options encoded in the filename as keyvalue pairs is 
deprecated
 unable to get monitor info from DNS SRV with service name: ceph-mon
-no monitors specified to connect to.
 qemu-img: Could not open 
'json:{'file.driver':'rbd','file.filename':'rbd:rbd/bogus:conf=BOGUS_CONF'}': 
error connecting: No such file or directory
 unable to get monitor info from DNS SRV with service name: ceph-mon
-no monitors specified to connect to.
 qemu-img: Could not open 
'json:{'file.driver':'rbd','file.pool':'rbd','file.image':'bogus','file.conf':'BOGUS_CONF'}':
 error connecting: No such file or directory
 *** done
-- 
2.30.2

[PATCH v2 2/2] block/rbd: Add an escape-aware strchr helper

2021-04-01 Thread Connor Kuehl

Sometimes the parser needs to further split a token it has collected
from the token input stream. Right now, it does a cursory check to see
if the relevant characters appear in the token to determine if it should
break it down further.

However, qemu_rbd_next_tok() will escape characters as it removes tokens
from the token stream and plain strchr() won't. This can make the
initial strchr() check slightly misleading since it implies
qemu_rbd_next_tok() will find the token and split on it, except the
reality is that qemu_rbd_next_tok() will pass over it if it is escaped.

Use a custom strchr to avoid mixing escaped and unescaped string
operations.

Reported-by: Han Han 
Fixes: https://bugzilla.redhat.com/1873913
Signed-off-by: Connor Kuehl 
---
 block/rbd.c| 20 ++--
 tests/qemu-iotests/231 |  4 
 tests/qemu-iotests/231.out |  3 +++
 3 files changed, 25 insertions(+), 2 deletions(-)

diff --git a/block/rbd.c b/block/rbd.c
index 9071a00e3f..c0e4d4a952 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -134,6 +134,22 @@ static char *qemu_rbd_next_tok(char *src, char delim, char 
**p)
 return src;
 }
 
+static char *qemu_rbd_strchr(char *src, char delim)
+{
+char *p;
+
+for (p = src; *p; ++p) {
+if (*p == delim) {
+return p;
+}
+if (*p == '\\') {
+++p;
+}
+}
+
+return NULL;
+}
+
 static void qemu_rbd_unescape(char *src)
 {
 char *p;
@@ -171,7 +187,7 @@ static void qemu_rbd_parse_filename(const char *filename, 
QDict *options,
 qemu_rbd_unescape(found_str);
 qdict_put_str(options, "pool", found_str);
 
-if (strchr(p, '@')) {
+if (qemu_rbd_strchr(p, '@')) {
 image_name = qemu_rbd_next_tok(p, '@', );
 
 found_str = qemu_rbd_next_tok(p, ':', );
@@ -181,7 +197,7 @@ static void qemu_rbd_parse_filename(const char *filename, 
QDict *options,
 image_name = qemu_rbd_next_tok(p, ':', );
 }
 /* Check for namespace in the image_name */
-if (strchr(image_name, '/')) {
+if (qemu_rbd_strchr(image_name, '/')) {
 found_str = qemu_rbd_next_tok(image_name, '/', _name);
 qemu_rbd_unescape(found_str);
 qdict_put_str(options, "namespace", found_str);
diff --git a/tests/qemu-iotests/231 b/tests/qemu-iotests/231
index 0f66d0ca36..8e6c6447c1 100755
--- a/tests/qemu-iotests/231
+++ b/tests/qemu-iotests/231
@@ -55,6 +55,10 @@ _filter_conf()
 $QEMU_IMG info 
"json:{'file.driver':'rbd','file.filename':'rbd:rbd/bogus:conf=${BOGUS_CONF}'}" 
2>&1 | _filter_conf
 $QEMU_IMG info 
"json:{'file.driver':'rbd','file.pool':'rbd','file.image':'bogus','file.conf':'${BOGUS_CONF}'}"
 2>&1 | _filter_conf
 
+# Regression test: the qemu-img invocation is expected to fail, but it should
+# not seg fault the parser.
+$QEMU_IMG create "rbd:rbd/aa\/bb:conf=${BOGUS_CONF}" 1M 2>&1 | _filter_conf
+
 # success, all done
 echo "*** done"
 rm -f $seq.full
diff --git a/tests/qemu-iotests/231.out b/tests/qemu-iotests/231.out
index 747dd221bb..a785a6e859 100644
--- a/tests/qemu-iotests/231.out
+++ b/tests/qemu-iotests/231.out
@@ -4,4 +4,7 @@ unable to get monitor info from DNS SRV with service name: 
ceph-mon
 qemu-img: Could not open 
'json:{'file.driver':'rbd','file.filename':'rbd:rbd/bogus:conf=BOGUS_CONF'}': 
error connecting: No such file or directory
 unable to get monitor info from DNS SRV with service name: ceph-mon
 qemu-img: Could not open 
'json:{'file.driver':'rbd','file.pool':'rbd','file.image':'bogus','file.conf':'BOGUS_CONF'}':
 error connecting: No such file or directory
+Formatting 'rbd:rbd/aa\/bb:conf=BOGUS_CONF', fmt=raw size=1048576
+unable to get monitor info from DNS SRV with service name: ceph-mon
+qemu-img: rbd:rbd/aa\/bb:conf=BOGUS_CONF: error connecting: No such file or 
directory
 *** done
-- 
2.30.2

[PATCH v2 0/2] Fix segfault in qemu_rbd_parse_filename

2021-04-01 Thread Connor Kuehl

Replaced the change to qemu_rbd_next_tok with a standalone escape-aware
helper for patch 2.

Connor Kuehl (2):
  iotests/231: Update expected deprecation message
  block/rbd: Add an escape-aware strchr helper

 block/rbd.c| 20 ++--
 tests/qemu-iotests/231 |  4 
 tests/qemu-iotests/231.out |  7 ---
 3 files changed, 26 insertions(+), 5 deletions(-)

-- 
2.30.2

issuing [block-]job-complete to jobs in STANDBY state

2021-04-01 Thread John Snow

Hi; downstream we've run into an issue where VMs under heavy load with 
many simultaneously concurrent block jobs running might occasionally 
flicker into the STANDBY state, during which time they will be unable to 
receive JOB COMPLETE commands. I assume this flicker is due to 
child_job_drained_begin().


BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1945635

It's safe to just retry this operation again, but it may be difficult to 
understand WHY the job is paused at the application level, since the 
flush event may be asynchronous and unpredictable.


We could define a transition to allow COMPLETE to be applied to STANDBY 
jobs, but is there any risk or drawback to doing so? On QMP's side, we 
do know the difference between a temporary pause and a user pause/error 
pause (Both use the user_pause flag.)


I imagine it's safe to continue rejecting COMPLETE commands if 
user_paused is set ("No, go fix this first!") and we could define a 
pathway for implicitly STANDBY jobs only. However, in this case, we 
don't really know how long STANDBY will last. Do we have the ability to 
easily accept an async "intent" to complete a job without tying up the 
monitor?


ATM I think only mirror uses .complete, but it looks like it tries to 
actually set up the pivot a good deal before delegating to the bottom 
half, so I worry it's not safe to try to run this when we are in the 
middle of a drain.


Any thoughts?

--js

Re: [PATCH 2/2] block/rbd: Don't unescape in qemu_rbd_next_tok()

2021-04-01 Thread Connor Kuehl


On 4/1/21 12:24 PM, Max Reitz wrote:

On 01.04.21 17:52, Connor Kuehl wrote:

That's qemu_rbd_unescape()'s job! No need to duplicate the labor.

Furthermore, this was causing some confusion in the parsing logic to
where the caller might test for the presence of a character to split on
like so:

if (strchr(image_name, '/')) {
 found_str = qemu_rbd_next_tok(image_name, '/', _name);
[..]

When qemu_rbd_next_tok() performs unescaping as a side effect, the
parser can get confused thinking that it can split this string, but
really the delimiter '/' gets unescaped and so qemu_rbd_next_tok() never
"finds" the delimiter and consumes the rest of the token input stream.


I don’t fully understand.  I understand the strchr() problem and the 
thing you explain next.  But I would have thought that’s a problem of 
using strchr(), i.e. that we’d need a custom function to do strchr() but 
consider escaped characters.  If it’s just about true/false like in your 
example, it could be a new version of qemu_rbd_next_tok() that does not 
modify the string.


I went back and forth a lot on the different ways this can be fixed 
before sending this, and I agree the most consistent fix here would be 
to add an escape-aware strchr. Initially, adding another libc-like 
function with more side effects wasn't as appealing to me as removing 
the side effects entirely to separate mechanism vs. policy. Your example 
below convinced me that this patch would split the token in unexpected 
ways. I'll send a v2 :-)


Thanks,

Connor


[..]
Should it?  I would have fully expected it to not be split and the 
parser complains that the input is invalid.  Or, let’s say, 
"foo\/bar/baz” should be split into “foo\/bar” and “baz”.

[PULL 1/9] vhost-user-blk: use different event handlers on initialization

2021-04-01 Thread Michael S. Tsirkin

From: Denis Plotnikov 

It is useful to use different connect/disconnect event handlers
on device initialization and operation as seen from the further
commit fixing a bug on device initialization.

This patch refactors the code to make use of them: we don't rely any
more on the VM state for choosing how to cleanup the device, instead
we explicitly use the proper event handler depending on whether
the device has been initialized.

Signed-off-by: Denis Plotnikov 
Reviewed-by: Raphael Norwitz 
Message-Id: <20210325151217.262793-2-den-plotni...@yandex-team.ru>
Reviewed-by: Michael S. Tsirkin 
Signed-off-by: Michael S. Tsirkin 
---
 hw/block/vhost-user-blk.c | 31 ---
 1 file changed, 24 insertions(+), 7 deletions(-)

diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
index b870a50e6b..1af95ec6aa 100644
--- a/hw/block/vhost-user-blk.c
+++ b/hw/block/vhost-user-blk.c
@@ -362,7 +362,18 @@ static void vhost_user_blk_disconnect(DeviceState *dev)
 vhost_dev_cleanup(>dev);
 }
 
-static void vhost_user_blk_event(void *opaque, QEMUChrEvent event);
+static void vhost_user_blk_event(void *opaque, QEMUChrEvent event,
+ bool realized);
+
+static void vhost_user_blk_event_realize(void *opaque, QEMUChrEvent event)
+{
+vhost_user_blk_event(opaque, event, false);
+}
+
+static void vhost_user_blk_event_oper(void *opaque, QEMUChrEvent event)
+{
+vhost_user_blk_event(opaque, event, true);
+}
 
 static void vhost_user_blk_chr_closed_bh(void *opaque)
 {
@@ -371,11 +382,12 @@ static void vhost_user_blk_chr_closed_bh(void *opaque)
 VHostUserBlk *s = VHOST_USER_BLK(vdev);
 
 vhost_user_blk_disconnect(dev);
-qemu_chr_fe_set_handlers(>chardev, NULL, NULL, vhost_user_blk_event,
-NULL, opaque, NULL, true);
+qemu_chr_fe_set_handlers(>chardev, NULL, NULL,
+vhost_user_blk_event_oper, NULL, opaque, NULL, true);
 }
 
-static void vhost_user_blk_event(void *opaque, QEMUChrEvent event)
+static void vhost_user_blk_event(void *opaque, QEMUChrEvent event,
+ bool realized)
 {
 DeviceState *dev = opaque;
 VirtIODevice *vdev = VIRTIO_DEVICE(dev);
@@ -406,7 +418,7 @@ static void vhost_user_blk_event(void *opaque, QEMUChrEvent 
event)
  * TODO: maybe it is a good idea to make the same fix
  * for other vhost-user devices.
  */
-if (runstate_is_running()) {
+if (realized) {
 AioContext *ctx = qemu_get_current_aio_context();
 
 qemu_chr_fe_set_handlers(>chardev, NULL, NULL, NULL, NULL,
@@ -473,8 +485,9 @@ static void vhost_user_blk_device_realize(DeviceState *dev, 
Error **errp)
 s->vhost_vqs = g_new0(struct vhost_virtqueue, s->num_queues);
 s->connected = false;
 
-qemu_chr_fe_set_handlers(>chardev,  NULL, NULL, vhost_user_blk_event,
- NULL, (void *)dev, NULL, true);
+qemu_chr_fe_set_handlers(>chardev,  NULL, NULL,
+ vhost_user_blk_event_realize, NULL, (void *)dev,
+ NULL, true);
 
 reconnect:
 if (qemu_chr_fe_wait_connected(>chardev, ) < 0) {
@@ -494,6 +507,10 @@ reconnect:
 goto reconnect;
 }
 
+/* we're fully initialized, now we can operate, so change the handler */
+qemu_chr_fe_set_handlers(>chardev,  NULL, NULL,
+ vhost_user_blk_event_oper, NULL, (void *)dev,
+ NULL, true);
 return;
 
 virtio_err:
-- 
MST

[PULL 3/9] vhost-user-blk: add immediate cleanup on shutdown

2021-04-01 Thread Michael S. Tsirkin

From: Denis Plotnikov 

Qemu crashes on shutdown if the chardev used by vhost-user-blk has been
finalized before the vhost-user-blk.

This happens with char-socket chardev operating in the listening mode (server).
The char-socket chardev emits "close" event at the end of finalizing when
its internal data is destroyed. This calls vhost-user-blk event handler
which in turn tries to manipulate with destroyed chardev by setting an empty
event handler for vhost-user-blk cleanup postponing.

This patch separates the shutdown case from the cleanup postponing removing
the need to set an event handler.

Signed-off-by: Denis Plotnikov 
Message-Id: <20210325151217.262793-4-den-plotni...@yandex-team.ru>
Reviewed-by: Michael S. Tsirkin 
Signed-off-by: Michael S. Tsirkin 
---
 hw/block/vhost-user-blk.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
index 4e215f71f1..0b5b9d44cd 100644
--- a/hw/block/vhost-user-blk.c
+++ b/hw/block/vhost-user-blk.c
@@ -411,7 +411,7 @@ static void vhost_user_blk_event(void *opaque, QEMUChrEvent 
event,
  * other code perform its own cleanup sequence using vhost_dev data
  * (e.g. vhost_dev_set_log).
  */
-if (realized) {
+if (realized && !runstate_check(RUN_STATE_SHUTDOWN)) {
 /*
  * A close event may happen during a read/write, but vhost
  * code assumes the vhost_dev remains setup, so delay the
-- 
MST

[PULL 2/9] vhost-user-blk: perform immediate cleanup if disconnect on initialization

2021-04-01 Thread Michael S. Tsirkin

From: Denis Plotnikov 

Commit 4bcad76f4c39 ("vhost-user-blk: delay vhost_user_blk_disconnect")
introduced postponing vhost_dev cleanup aiming to eliminate qemu aborts
because of connection problems with vhost-blk daemon.

However, it introdues a new problem. Now, any communication errors
during execution of vhost_dev_init() called by vhost_user_blk_device_realize()
lead to qemu abort on assert in vhost_dev_get_config().

This happens because vhost_user_blk_disconnect() is postponed but
it should have dropped s->connected flag by the time
vhost_user_blk_device_realize() performs a new connection opening.
On the connection opening, vhost_dev initialization in
vhost_user_blk_connect() relies on s->connection flag and
if it's not dropped, it skips vhost_dev initialization and returns
with success. Then, vhost_user_blk_device_realize()'s execution flow
goes to vhost_dev_get_config() where it's aborted on the assert.

To fix the problem this patch adds immediate cleanup on device
initialization(in vhost_user_blk_device_realize()) using different
event handlers for initialization and operation introduced in the
previous patch.
On initialization (in vhost_user_blk_device_realize()) we fully
control the initialization process. At that point, nobody can use the
device since it isn't initialized and we don't need to postpone any
cleanups, so we can do cleaup right away when there is a communication
problem with the vhost-blk daemon.
On operation we leave it as is, since the disconnect may happen when
the device is in use, so the device users may want to use vhost_dev's data
to do rollback before vhost_dev is re-initialized (e.g. in vhost_dev_set_log()).

Signed-off-by: Denis Plotnikov 
Reviewed-by: Raphael Norwitz 
Message-Id: <20210325151217.262793-3-den-plotni...@yandex-team.ru>
Reviewed-by: Michael S. Tsirkin 
Signed-off-by: Michael S. Tsirkin 
---
 hw/block/vhost-user-blk.c | 48 +++
 1 file changed, 24 insertions(+), 24 deletions(-)

diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
index 1af95ec6aa..4e215f71f1 100644
--- a/hw/block/vhost-user-blk.c
+++ b/hw/block/vhost-user-blk.c
@@ -402,38 +402,38 @@ static void vhost_user_blk_event(void *opaque, 
QEMUChrEvent event,
 break;
 case CHR_EVENT_CLOSED:
 /*
- * A close event may happen during a read/write, but vhost
- * code assumes the vhost_dev remains setup, so delay the
- * stop & clear. There are two possible paths to hit this
- * disconnect event:
- * 1. When VM is in the RUN_STATE_PRELAUNCH state. The
- * vhost_user_blk_device_realize() is a caller.
- * 2. In tha main loop phase after VM start.
- *
- * For p2 the disconnect event will be delayed. We can't
- * do the same for p1, because we are not running the loop
- * at this moment. So just skip this step and perform
- * disconnect in the caller function.
- *
- * TODO: maybe it is a good idea to make the same fix
- * for other vhost-user devices.
+ * Closing the connection should happen differently on device
+ * initialization and operation stages.
+ * On initalization, we want to re-start vhost_dev initialization
+ * from the very beginning right away when the connection is closed,
+ * so we clean up vhost_dev on each connection closing.
+ * On operation, we want to postpone vhost_dev cleanup to let the
+ * other code perform its own cleanup sequence using vhost_dev data
+ * (e.g. vhost_dev_set_log).
  */
 if (realized) {
+/*
+ * A close event may happen during a read/write, but vhost
+ * code assumes the vhost_dev remains setup, so delay the
+ * stop & clear.
+ */
 AioContext *ctx = qemu_get_current_aio_context();
 
 qemu_chr_fe_set_handlers(>chardev, NULL, NULL, NULL, NULL,
 NULL, NULL, false);
 aio_bh_schedule_oneshot(ctx, vhost_user_blk_chr_closed_bh, opaque);
-}
 
-/*
- * Move vhost device to the stopped state. The vhost-user device
- * will be clean up and disconnected in BH. This can be useful in
- * the vhost migration code. If disconnect was caught there is an
- * option for the general vhost code to get the dev state without
- * knowing its type (in this case vhost-user).
- */
-s->dev.started = false;
+/*
+ * Move vhost device to the stopped state. The vhost-user device
+ * will be clean up and disconnected in BH. This can be useful in
+ * the vhost migration code. If disconnect was caught there is an
+ * option for the general vhost code to get the dev state without
+ * knowing its type (in this case vhost-user).
+ */
+s->dev.started = false;
+

Re: [PATCH 2/2] block/rbd: Don't unescape in qemu_rbd_next_tok()

2021-04-01 Thread Max Reitz


On 01.04.21 17:52, Connor Kuehl wrote:

That's qemu_rbd_unescape()'s job! No need to duplicate the labor.

Furthermore, this was causing some confusion in the parsing logic to
where the caller might test for the presence of a character to split on
like so:

if (strchr(image_name, '/')) {
 found_str = qemu_rbd_next_tok(image_name, '/', _name);
[..]

When qemu_rbd_next_tok() performs unescaping as a side effect, the
parser can get confused thinking that it can split this string, but
really the delimiter '/' gets unescaped and so qemu_rbd_next_tok() never
"finds" the delimiter and consumes the rest of the token input stream.


I don’t fully understand.  I understand the strchr() problem and the 
thing you explain next.  But I would have thought that’s a problem of 
using strchr(), i.e. that we’d need a custom function to do strchr() but 
consider escaped characters.  If it’s just about true/false like in your 
example, it could be a new version of qemu_rbd_next_tok() that does not 
modify the string.


Or, well, actually, in your example, one could unconditionally invoke 
qemu_rbd_next_tok() and check whether *found_str is '\0' or not.



This is problematic because qemu_rbd_next_tok() also steals the input
pointer to the token stream and sets it to NULL. This causes a segfault
where the parser expects to split one string into two.


I would say that’s a problem of the caller.  It can pass a different 
variable for the out-pointer, so the input pointer isn’t stolen.



In this case, the parser is determining if a string is a
namespace/image_name pair like so:

"foo/bar"

And clearly it's looking to split it like so:

namespace:  foo
image_name: bar

but if the input is "foo\/bar", it *should* split into

namespace:  foo\
image_name: bar


Should it?  I would have fully expected it to not be split and the 
parser complains that the input is invalid.  Or, let’s say, 
"foo\/bar/baz” should be split into “foo\/bar” and “baz”.


Max


and its subordinate parts can be unescaped after tokenization.

So, instead of tokenizing *and* escaping all at once, do one before the
other to avoid stumbling into a segfault by confusing the parsing logic.

Reported-by: Han Han 
Fixes: https://bugzilla.redhat.com/1873913
Signed-off-by: Connor Kuehl 
---
  block/rbd.c| 3 ---
  tests/qemu-iotests/231 | 4 
  tests/qemu-iotests/231.out | 3 +++
  3 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/block/rbd.c b/block/rbd.c
index 9071a00e3f..9bed0863e5 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -123,9 +123,6 @@ static char *qemu_rbd_next_tok(char *src, char delim, char 
**p)
  if (*end == delim) {
  break;
  }
-if (*end == '\\' && end[1] != '\0') {
-end++;
-}
  }
  if (*end == delim) {
  *p = end + 1;
diff --git a/tests/qemu-iotests/231 b/tests/qemu-iotests/231
index 0f66d0ca36..8e6c6447c1 100755
--- a/tests/qemu-iotests/231
+++ b/tests/qemu-iotests/231
@@ -55,6 +55,10 @@ _filter_conf()
  $QEMU_IMG info 
"json:{'file.driver':'rbd','file.filename':'rbd:rbd/bogus:conf=${BOGUS_CONF}'}" 
2>&1 | _filter_conf
  $QEMU_IMG info 
"json:{'file.driver':'rbd','file.pool':'rbd','file.image':'bogus','file.conf':'${BOGUS_CONF}'}"
 2>&1 | _filter_conf
  
+# Regression test: the qemu-img invocation is expected to fail, but it should

+# not seg fault the parser.
+$QEMU_IMG create "rbd:rbd/aa\/bb:conf=${BOGUS_CONF}" 1M 2>&1 | _filter_conf
+
  # success, all done
  echo "*** done"
  rm -f $seq.full
diff --git a/tests/qemu-iotests/231.out b/tests/qemu-iotests/231.out
index 747dd221bb..a785a6e859 100644
--- a/tests/qemu-iotests/231.out
+++ b/tests/qemu-iotests/231.out
@@ -4,4 +4,7 @@ unable to get monitor info from DNS SRV with service name: 
ceph-mon
  qemu-img: Could not open 
'json:{'file.driver':'rbd','file.filename':'rbd:rbd/bogus:conf=BOGUS_CONF'}': 
error connecting: No such file or directory
  unable to get monitor info from DNS SRV with service name: ceph-mon
  qemu-img: Could not open 
'json:{'file.driver':'rbd','file.pool':'rbd','file.image':'bogus','file.conf':'BOGUS_CONF'}':
 error connecting: No such file or directory
+Formatting 'rbd:rbd/aa\/bb:conf=BOGUS_CONF', fmt=raw size=1048576
+unable to get monitor info from DNS SRV with service name: ceph-mon
+qemu-img: rbd:rbd/aa\/bb:conf=BOGUS_CONF: error connecting: No such file or 
directory
  *** done

Re: [PATCH 1/2] iotests/231: Update expected deprecation message

2021-04-01 Thread Connor Kuehl


On 4/1/21 12:07 PM, Max Reitz wrote:

On 01.04.21 18:52, Max Reitz wrote:

On 01.04.21 17:52, Connor Kuehl wrote:

The deprecation message changed slightly at some point in the past but
the expected output wasn't updated along with it; causing it to fail.
Fix it, so it passes.

Signed-off-by: Connor Kuehl 
---
  tests/qemu-iotests/231.out | 4 +---
  1 file changed, 1 insertion(+), 3 deletions(-)


Uh, well, you know what, I can’t find any version where there was any 
other output.  Even back in 66e6a735e97450ac50fcaf40f78600c688534cae, 
where this test was introduced, I get this diff.


What’s going on there?


Okay.  So:

Jeff’s original patch[1] included the “Future versions may cease to 
parse...” part.  v1 of his subsequent pull request[2] did, too.  But 
v2[3] didn’t.  Looks like Markus made a comment on v4 of the patch, and 
then Jeff fixed up the patch in his branch, but didn’t change the test. 
  In any case it’s clear that the reference output was wrong all along.


About the “no monitors specified” part...  The only place where I can 
find “no monitors” is in Jeff’s patches to add this iotest.  I have no 
idea where that orignated from.


So:

Reviewed-by: Max Reitz 


Thanks! And excellent sleuthing. This accidental conspiracy went much 
farther beyond the git log than I thought...


Connor




[1]
https://lists.nongnu.org/archive/html/qemu-block/2018-09/msg00282.html

[2]
https://lists.nongnu.org/archive/html/qemu-block/2018-09/msg00307.html

[3]
https://lists.nongnu.org/archive/html/qemu-block/2018-09/msg00592.html

Re: [PATCH 1/2] iotests/231: Update expected deprecation message

2021-04-01 Thread Max Reitz


On 01.04.21 18:52, Max Reitz wrote:

On 01.04.21 17:52, Connor Kuehl wrote:

The deprecation message changed slightly at some point in the past but
the expected output wasn't updated along with it; causing it to fail.
Fix it, so it passes.

Signed-off-by: Connor Kuehl 
---
  tests/qemu-iotests/231.out | 4 +---
  1 file changed, 1 insertion(+), 3 deletions(-)


Uh, well, you know what, I can’t find any version where there was any 
other output.  Even back in 66e6a735e97450ac50fcaf40f78600c688534cae, 
where this test was introduced, I get this diff.


What’s going on there?


Okay.  So:

Jeff’s original patch[1] included the “Future versions may cease to 
parse...” part.  v1 of his subsequent pull request[2] did, too.  But 
v2[3] didn’t.  Looks like Markus made a comment on v4 of the patch, and 
then Jeff fixed up the patch in his branch, but didn’t change the test. 
 In any case it’s clear that the reference output was wrong all along.


About the “no monitors specified” part...  The only place where I can 
find “no monitors” is in Jeff’s patches to add this iotest.  I have no 
idea where that orignated from.


So:

Reviewed-by: Max Reitz 


[1]
https://lists.nongnu.org/archive/html/qemu-block/2018-09/msg00282.html

[2]
https://lists.nongnu.org/archive/html/qemu-block/2018-09/msg00307.html

[3]
https://lists.nongnu.org/archive/html/qemu-block/2018-09/msg00592.html

Re: [PATCH 1/2] iotests/231: Update expected deprecation message

2021-04-01 Thread Max Reitz


On 01.04.21 17:52, Connor Kuehl wrote:

The deprecation message changed slightly at some point in the past but
the expected output wasn't updated along with it; causing it to fail.
Fix it, so it passes.

Signed-off-by: Connor Kuehl 
---
  tests/qemu-iotests/231.out | 4 +---
  1 file changed, 1 insertion(+), 3 deletions(-)


Uh, well, you know what, I can’t find any version where there was any 
other output.  Even back in 66e6a735e97450ac50fcaf40f78600c688534cae, 
where this test was introduced, I get this diff.


What’s going on there?

Max

[PATCH v2] iotests: add test for removing persistent bitmap from backing file

2021-04-01 Thread Vladimir Sementsov-Ogievskiy

Just demonstrate one of x-blockdev-reopen usecases. We can't simply
remove persistent bitmap from RO node (for example from backing file),
as we need to remove it from the image too. So, we should reopen the
node first.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---

v2: [by Max's review]
 - more imports for convenience
 - assert that qemu_img_create and qemu_img succeeds
 - add -F argument to qemu_img_create
 - fix error message

 .../tests/remove-bitmap-from-backing  | 69 +++
 .../tests/remove-bitmap-from-backing.out  |  6 ++
 2 files changed, 75 insertions(+)
 create mode 100755 tests/qemu-iotests/tests/remove-bitmap-from-backing
 create mode 100644 tests/qemu-iotests/tests/remove-bitmap-from-backing.out

diff --git a/tests/qemu-iotests/tests/remove-bitmap-from-backing 
b/tests/qemu-iotests/tests/remove-bitmap-from-backing
new file mode 100755
index 00..0ea4c36507
--- /dev/null
+++ b/tests/qemu-iotests/tests/remove-bitmap-from-backing
@@ -0,0 +1,69 @@
+#!/usr/bin/env python3
+#
+# Test removing persistent bitmap from backing
+#
+# Copyright (c) 2021 Virtuozzo International GmbH.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see .
+#
+
+import iotests
+from iotests import log, qemu_img_create, qemu_img, qemu_img_pipe
+
+iotests.script_initialize(supported_fmts=['qcow2'])
+
+top, base = iotests.file_path('top', 'base')
+size = '1M'
+
+assert qemu_img_create('-f', iotests.imgfmt, base, size) == 0
+assert qemu_img_create('-f', iotests.imgfmt, '-b', base,
+   '-F', iotests.imgfmt, top, size) == 0
+
+assert qemu_img('bitmap', '--add', base, 'bitmap0') == 0
+# Just assert that our method of checking bitmaps in the image works.
+assert 'bitmaps' in qemu_img_pipe('info', base)
+
+vm = iotests.VM().add_drive(top, 'backing.node-name=base')
+vm.launch()
+
+log('Trying to remove persistent bitmap from r-o base node, should fail:')
+vm.qmp_log('block-dirty-bitmap-remove', node='base', name='bitmap0')
+
+new_base_opts = {
+'node-name': 'base',
+'driver': 'qcow2',
+'file': {
+'driver': 'file',
+'filename':  base
+},
+'read-only': False
+}
+
+# Don't want to bother with filtering qmp_log for reopen command
+result = vm.qmp('x-blockdev-reopen', **new_base_opts)
+if result != {'return': {}}:
+log('Failed to reopen: ' + str(result))
+
+log('Remove persistent bitmap from base node reopened to RW:')
+vm.qmp_log('block-dirty-bitmap-remove', node='base', name='bitmap0')
+
+new_base_opts['read-only'] = True
+result = vm.qmp('x-blockdev-reopen', **new_base_opts)
+if result != {'return': {}}:
+log('Failed to reopen: ' + str(result))
+
+vm.shutdown()
+
+if 'bitmaps' in qemu_img_pipe('info', base):
+log('ERROR: Bitmap is still in the base image')
diff --git a/tests/qemu-iotests/tests/remove-bitmap-from-backing.out 
b/tests/qemu-iotests/tests/remove-bitmap-from-backing.out
new file mode 100644
index 00..c28af82c75
--- /dev/null
+++ b/tests/qemu-iotests/tests/remove-bitmap-from-backing.out
@@ -0,0 +1,6 @@
+Trying to remove persistent bitmap from r-o base node, should fail:
+{"execute": "block-dirty-bitmap-remove", "arguments": {"name": "bitmap0", 
"node": "base"}}
+{"error": {"class": "GenericError", "desc": "Bitmap 'bitmap0' is readonly and 
cannot be modified"}}
+Remove persistent bitmap from base node reopened to RW:
+{"execute": "block-dirty-bitmap-remove", "arguments": {"name": "bitmap0", 
"node": "base"}}
+{"return": {}}
-- 
2.29.2

Re: [PATCH] iotests/qsd-jobs: Filter events in the first test

2021-04-01 Thread Vladimir Sementsov-Ogievskiy


01.04.2021 18:30, Max Reitz wrote:

On 01.04.21 16:37, Vladimir Sementsov-Ogievskiy wrote:

01.04.2021 17:37, Vladimir Sementsov-Ogievskiy wrote:

01.04.2021 16:28, Max Reitz wrote:

The job may or may not be ready before the 'quit' is issued.  Whether it
is is irrelevant; for the purpose of the test, it only needs to still be
there.  Filter the job status change and READY events from the output so
it becomes reliable.

Reported-by: Peter Maydell
Suggested-by: Vladimir Sementsov-Ogievskiy
Signed-off-by: Max Reitz


Reviewed-by: Vladimir Sementsov-Ogievskiy



Hmm, in your mail (and in copypasted by r-b) there is no space between name and 
"<"..


Are you sure?  I can see the spaces in my original mail, and on
https://lists.nongnu.org/archive/html/qemu-devel/2021-04/msg00182.html

But OTOH I don’t see a space between your name and the < in your R-b...

Max



Ah yes. It's someone (possibly my thunderbird) eats spaces. I see them in you 
original mail, but not in quote in my first answer

--
Best regards,
Vladimir

Re: [PATCH v3 2/2] block: Add backend_defaults property

2021-04-01 Thread Stefan Hajnoczi

On Thu, Mar 11, 2021 at 12:39:16AM +0900, Akihiko Odaki wrote:
> backend_defaults property allow users to control if default block
> properties should be decided with backend information.
> 
> If it is off, any backend information will be discarded, which is
> suitable if you plan to perform live migration to a different disk backend.
> 
> If it is on, a block device may utilize backend information more
> aggressively.
> 
> By default, it is auto, which uses backend information from physical
> devices and ignores one from file. It is consistent with the older
> versions, and should be aligned with the user expectation that a file
> backend is more independent of host than a physical device backend.

Can BlockLimits pdiscard_alignment and opt_transfer be used instead of
adding discard_granularity and opt_io fields to BlockSizes? They seem to
do the same thing except the QEMU block layer currently uses BlockLimits
values internally to ensure that requests are suitable for the host
device.


signature.asc
Description: PGP signature

[PATCH 1/2] iotests/231: Update expected deprecation message

2021-04-01 Thread Connor Kuehl

The deprecation message changed slightly at some point in the past but
the expected output wasn't updated along with it; causing it to fail.
Fix it, so it passes.

Signed-off-by: Connor Kuehl 
---
 tests/qemu-iotests/231.out | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/tests/qemu-iotests/231.out b/tests/qemu-iotests/231.out
index 579ba11c16..747dd221bb 100644
--- a/tests/qemu-iotests/231.out
+++ b/tests/qemu-iotests/231.out
@@ -1,9 +1,7 @@
 QA output created by 231
-qemu-img: RBD options encoded in the filename as keyvalue pairs is deprecated. 
 Future versions may cease to parse these options in the future.
+qemu-img: warning: RBD options encoded in the filename as keyvalue pairs is 
deprecated
 unable to get monitor info from DNS SRV with service name: ceph-mon
-no monitors specified to connect to.
 qemu-img: Could not open 
'json:{'file.driver':'rbd','file.filename':'rbd:rbd/bogus:conf=BOGUS_CONF'}': 
error connecting: No such file or directory
 unable to get monitor info from DNS SRV with service name: ceph-mon
-no monitors specified to connect to.
 qemu-img: Could not open 
'json:{'file.driver':'rbd','file.pool':'rbd','file.image':'bogus','file.conf':'BOGUS_CONF'}':
 error connecting: No such file or directory
 *** done
-- 
2.30.2

[PATCH 0/2] Fix segfault in qemu_rbd_parse_filename

2021-04-01 Thread Connor Kuehl

Connor Kuehl (2):
  iotests/231: Update expected deprecation message
  block/rbd: Don't unescape in qemu_rbd_next_tok()

 block/rbd.c| 3 ---
 tests/qemu-iotests/231 | 4 
 tests/qemu-iotests/231.out | 7 ---
 3 files changed, 8 insertions(+), 6 deletions(-)

-- 
2.30.2

[PATCH 2/2] block/rbd: Don't unescape in qemu_rbd_next_tok()

2021-04-01 Thread Connor Kuehl

That's qemu_rbd_unescape()'s job! No need to duplicate the labor.

Furthermore, this was causing some confusion in the parsing logic to
where the caller might test for the presence of a character to split on
like so:

if (strchr(image_name, '/')) {
found_str = qemu_rbd_next_tok(image_name, '/', _name);
[..]

When qemu_rbd_next_tok() performs unescaping as a side effect, the
parser can get confused thinking that it can split this string, but
really the delimiter '/' gets unescaped and so qemu_rbd_next_tok() never
"finds" the delimiter and consumes the rest of the token input stream.

This is problematic because qemu_rbd_next_tok() also steals the input
pointer to the token stream and sets it to NULL. This causes a segfault
where the parser expects to split one string into two.

In this case, the parser is determining if a string is a
namespace/image_name pair like so:

"foo/bar"

And clearly it's looking to split it like so:

namespace:  foo
image_name: bar

but if the input is "foo\/bar", it *should* split into

namespace:  foo\
image_name: bar

and its subordinate parts can be unescaped after tokenization.

So, instead of tokenizing *and* escaping all at once, do one before the
other to avoid stumbling into a segfault by confusing the parsing logic.

Reported-by: Han Han 
Fixes: https://bugzilla.redhat.com/1873913
Signed-off-by: Connor Kuehl 
---
 block/rbd.c| 3 ---
 tests/qemu-iotests/231 | 4 
 tests/qemu-iotests/231.out | 3 +++
 3 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/block/rbd.c b/block/rbd.c
index 9071a00e3f..9bed0863e5 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -123,9 +123,6 @@ static char *qemu_rbd_next_tok(char *src, char delim, char 
**p)
 if (*end == delim) {
 break;
 }
-if (*end == '\\' && end[1] != '\0') {
-end++;
-}
 }
 if (*end == delim) {
 *p = end + 1;
diff --git a/tests/qemu-iotests/231 b/tests/qemu-iotests/231
index 0f66d0ca36..8e6c6447c1 100755
--- a/tests/qemu-iotests/231
+++ b/tests/qemu-iotests/231
@@ -55,6 +55,10 @@ _filter_conf()
 $QEMU_IMG info 
"json:{'file.driver':'rbd','file.filename':'rbd:rbd/bogus:conf=${BOGUS_CONF}'}" 
2>&1 | _filter_conf
 $QEMU_IMG info 
"json:{'file.driver':'rbd','file.pool':'rbd','file.image':'bogus','file.conf':'${BOGUS_CONF}'}"
 2>&1 | _filter_conf
 
+# Regression test: the qemu-img invocation is expected to fail, but it should
+# not seg fault the parser.
+$QEMU_IMG create "rbd:rbd/aa\/bb:conf=${BOGUS_CONF}" 1M 2>&1 | _filter_conf
+
 # success, all done
 echo "*** done"
 rm -f $seq.full
diff --git a/tests/qemu-iotests/231.out b/tests/qemu-iotests/231.out
index 747dd221bb..a785a6e859 100644
--- a/tests/qemu-iotests/231.out
+++ b/tests/qemu-iotests/231.out
@@ -4,4 +4,7 @@ unable to get monitor info from DNS SRV with service name: 
ceph-mon
 qemu-img: Could not open 
'json:{'file.driver':'rbd','file.filename':'rbd:rbd/bogus:conf=BOGUS_CONF'}': 
error connecting: No such file or directory
 unable to get monitor info from DNS SRV with service name: ceph-mon
 qemu-img: Could not open 
'json:{'file.driver':'rbd','file.pool':'rbd','file.image':'bogus','file.conf':'BOGUS_CONF'}':
 error connecting: No such file or directory
+Formatting 'rbd:rbd/aa\/bb:conf=BOGUS_CONF', fmt=raw size=1048576
+unable to get monitor info from DNS SRV with service name: ceph-mon
+qemu-img: rbd:rbd/aa\/bb:conf=BOGUS_CONF: error connecting: No such file or 
directory
 *** done
-- 
2.30.2

Re: [PATCH v3 1/2] block/file-posix: Optimize for macOS

2021-04-01 Thread Stefan Hajnoczi

On Thu, Mar 11, 2021 at 12:39:15AM +0900, Akihiko Odaki wrote:
> @@ -1586,6 +1589,7 @@ out:
>  }
>  }
>  
> +G_GNUC_UNUSED

Why isn't translate_err() used in the F_PUNCHHOLE case below?

If you really want to avoid using it on macOS, please add a #if with the
necessary conditions here so it's clear when this translate_err() is
needed.

> @@ -514,7 +515,8 @@ static int raw_probe_blocksizes(BlockDriverState *bs, 
> BlockSizes *bsz)
>  return ret;
>  }
>  
> -if (!QEMU_IS_ALIGNED(s->offset, MAX(bsz->log, bsz->phys))) {
> +size = MAX(bsz->log, bsz->phys);
> +if (size && !QEMU_IS_ALIGNED(s->offset, size)) {
>  return -ENOTSUP;
>  }
>  

This patch changes the semantics of bdrv_probe_blocksizes(). It used to
return -ENOTSUP when phys/log weren't available. Now it returns 0 and
the fields are 0. Please update the bdrv_probe_blocksizes doc comment in
include/block/block_int.h to mention phys and log, as well as that
fields can be set to 0 (or -1 in the case of discard_granularity).

signature.asc
Description: PGP signature

Re: [PATCH 2/2] iotests/qsd-jobs: Use common.qemu for the QSD

2021-04-01 Thread Max Reitz


On 01.04.21 16:44, Vladimir Sementsov-Ogievskiy wrote:

01.04.2021 16:28, Max Reitz wrote:

Using common.qemu allows us to wait for specific replies, so we can for
example wait for events.  This allows starting the active commit job and
then wait for it to be ready before quitting the QSD, so we the output
is always the same.

(Strictly speaking, this is only necessary for the first test in
qsd-jobs, but we might as well make the second one use common.qemu's
infrastructure, too.)



Reviewed-by: Vladimir Sementsov-Ogievskiy 


Reported-by: Peter Maydell 
Signed-off-by: Max Reitz 
---
  tests/qemu-iotests/tests/qsd-jobs | 55 ---
  tests/qemu-iotests/tests/qsd-jobs.out | 10 -
  2 files changed, 49 insertions(+), 16 deletions(-)

diff --git a/tests/qemu-iotests/tests/qsd-jobs 
b/tests/qemu-iotests/tests/qsd-jobs

index 972b6b3898..af7f886f15 100755
--- a/tests/qemu-iotests/tests/qsd-jobs
+++ b/tests/qemu-iotests/tests/qsd-jobs
@@ -38,6 +38,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
  cd ..
  . ./common.rc
  . ./common.filter
+. ./common.qemu
  _supported_fmt qcow2
  _supported_proto generic
@@ -52,32 +53,58 @@ echo "=== Job still present at shutdown ==="
  echo
  # Just make sure that this doesn't crash
-$QSD --chardev stdio,id=stdio --monitor chardev=stdio \
+qsd=y _launch_qemu \


sounds a bit strange.. Like we are starting qemu.


Yeah, well.  Yeah.  We could have a

_launch_qsd()
{
qsd=y _launch_qemu
}

But this would still make it weird for all the other commands from 
common.qemu, and I don’t think it makes much sense to introduce aliases 
for all of them.  So I think it’d be best to live with that bit of 
weirdness.


Max

Re: [PATCH] iotests/qsd-jobs: Filter events in the first test

2021-04-01 Thread Max Reitz


On 01.04.21 16:37, Vladimir Sementsov-Ogievskiy wrote:

01.04.2021 17:37, Vladimir Sementsov-Ogievskiy wrote:

01.04.2021 16:28, Max Reitz wrote:

The job may or may not be ready before the 'quit' is issued.  Whether it
is is irrelevant; for the purpose of the test, it only needs to still be
there.  Filter the job status change and READY events from the output so
it becomes reliable.

Reported-by: Peter Maydell
Suggested-by: Vladimir Sementsov-Ogievskiy
Signed-off-by: Max Reitz


Reviewed-by: Vladimir Sementsov-Ogievskiy



Hmm, in your mail (and in copypasted by r-b) there is no space between 
name and "<"..


Are you sure?  I can see the spaces in my original mail, and on
https://lists.nongnu.org/archive/html/qemu-devel/2021-04/msg00182.html

But OTOH I don’t see a space between your name and the < in your R-b...

Max

Re: [PATCH 2/2] iotests/qsd-jobs: Use common.qemu for the QSD

2021-04-01 Thread Vladimir Sementsov-Ogievskiy


01.04.2021 16:28, Max Reitz wrote:

Using common.qemu allows us to wait for specific replies, so we can for
example wait for events.  This allows starting the active commit job and
then wait for it to be ready before quitting the QSD, so we the output
is always the same.

(Strictly speaking, this is only necessary for the first test in
qsd-jobs, but we might as well make the second one use common.qemu's
infrastructure, too.)



Reviewed-by: Vladimir Sementsov-Ogievskiy 


Reported-by: Peter Maydell 
Signed-off-by: Max Reitz 
---
  tests/qemu-iotests/tests/qsd-jobs | 55 ---
  tests/qemu-iotests/tests/qsd-jobs.out | 10 -
  2 files changed, 49 insertions(+), 16 deletions(-)

diff --git a/tests/qemu-iotests/tests/qsd-jobs 
b/tests/qemu-iotests/tests/qsd-jobs
index 972b6b3898..af7f886f15 100755
--- a/tests/qemu-iotests/tests/qsd-jobs
+++ b/tests/qemu-iotests/tests/qsd-jobs
@@ -38,6 +38,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
  cd ..
  . ./common.rc
  . ./common.filter
+. ./common.qemu
  
  _supported_fmt qcow2

  _supported_proto generic
@@ -52,32 +53,58 @@ echo "=== Job still present at shutdown ==="
  echo
  
  # Just make sure that this doesn't crash

-$QSD --chardev stdio,id=stdio --monitor chardev=stdio \
+qsd=y _launch_qemu \


sounds a bit strange.. Like we are starting qemu.


  --blockdev node-name=file0,driver=file,filename="$TEST_IMG" \
---blockdev node-name=fmt0,driver=qcow2,file=file0 <  
  echo

  echo "=== Streaming can't get permission on base node ==="
  echo
  
  # Just make sure that this doesn't crash

-$QSD --chardev stdio,id=stdio --monitor chardev=stdio \
+qsd=y _launch_qemu \
  --blockdev node-name=file_base,driver=file,filename="$TEST_IMG.base" \
  --blockdev node-name=fmt_base,driver=qcow2,file=file_base \
  --blockdev node-name=file_overlay,driver=file,filename="$TEST_IMG" \
  --blockdev 
node-name=fmt_overlay,driver=qcow2,file=file_overlay,backing=fmt_base \
  --nbd-server addr.type=unix,addr.path="$SOCK_DIR/nbd.sock" \
---export type=nbd,id=export1,node-name=fmt_base,writable=on,name=export1 \
-<  
  # success, all done

  echo "*** done"
diff --git a/tests/qemu-iotests/tests/qsd-jobs.out 
b/tests/qemu-iotests/tests/qsd-jobs.out
index 05e1165e80..5a14668939 100644
--- a/tests/qemu-iotests/tests/qsd-jobs.out
+++ b/tests/qemu-iotests/tests/qsd-jobs.out
@@ -4,13 +4,16 @@ Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728 
backing_file=TEST_DIR/
  
  === Job still present at shutdown ===
  
-QMP_VERSION

+{"execute":"qmp_capabilities"}
  {"return": {}}
+{"execute": "block-commit",
+  "arguments": {"device": "fmt0", "job-id": "job0"}}
  {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": 
{"status": "created", "id": "job0"}}
  {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": 
{"status": "running", "id": "job0"}}
  {"return": {}}
  {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": 
{"status": "ready", "id": "job0"}}
  {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "BLOCK_JOB_READY", "data": {"device": "job0", 
"len": 0, "offset": 0, "speed": 0, "type": "commit"}}
+{"execute": "quit"}
  {"return": {}}
  {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": 
{"status": "standby", "id": "job0"}}
  {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": 
{"status": "ready", "id": "job0"}}
@@ -22,11 +25,14 @@ QMP_VERSION
  
  === Streaming can't get permission on base node ===
  
-QMP_VERSION

+{"execute": "qmp_capabilities"}
  {"return": {}}
+{"execute": "block-stream",
+  "arguments": {"device": "fmt_overlay", "job-id": "job0"}}
  {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": 
{"status": "created", "id": "job0"}}
  {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": 
{"status": "null", "id": "job0"}}
  {"error": {"class": "GenericError", "desc": "Conflicts with use by a block device 
as 'root', which uses 'write' on fmt_base"}}
+{"execute": "quit"}
  {"return": {}}
  {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "BLOCK_EXPORT_DELETED", 
"data": {"id": "export1"}}
  *** done




--
Best regards,
Vladimir

Re: [PATCH] iotests/qsd-jobs: Filter events in the first test

2021-04-01 Thread Vladimir Sementsov-Ogievskiy


01.04.2021 17:37, Vladimir Sementsov-Ogievskiy wrote:

01.04.2021 16:28, Max Reitz wrote:

The job may or may not be ready before the 'quit' is issued.  Whether it
is is irrelevant; for the purpose of the test, it only needs to still be
there.  Filter the job status change and READY events from the output so
it becomes reliable.

Reported-by: Peter Maydell
Suggested-by: Vladimir Sementsov-Ogievskiy
Signed-off-by: Max Reitz


Reviewed-by: Vladimir Sementsov-Ogievskiy



Hmm, in your mail (and in copypasted by r-b) there is no space between name and 
"<"..

--
Best regards,
Vladimir

Re: [PATCH] iotests/qsd-jobs: Filter events in the first test

2021-04-01 Thread Vladimir Sementsov-Ogievskiy


01.04.2021 16:28, Max Reitz wrote:

The job may or may not be ready before the 'quit' is issued.  Whether it
is is irrelevant; for the purpose of the test, it only needs to still be
there.  Filter the job status change and READY events from the output so
it becomes reliable.

Reported-by: Peter Maydell
Suggested-by: Vladimir Sementsov-Ogievskiy
Signed-off-by: Max Reitz


Reviewed-by: Vladimir Sementsov-Ogievskiy

--
Best regards,
Vladimir

[RFC PATCH 2/3] qemu-iotests: add option to attach gdbserver

2021-04-01 Thread Emanuele Giuseppe Esposito

Add -gdb flag and GDB_QEMU environmental variable
to python tests to attach a gdbserver to each qemu_system instance.
The GDB_QEMU will just contain the options to provide to gdbserver,
if empty and -gdb is set it will default to "localhost:12345".

The gdb option is provided to python/qemu/machine.py through
the "wrapper" parameter, currently unused by qemu-iotests.

Attaching a gdbserver implies that the qmp socket
should wait indefinitely for an answer from QEMU, otherwise
the connection will drop after 15 seconds.

if -gdb is not provided but $GDB_QEMU is set, the script will ignore the
environmental variable.

Signed-off-by: Emanuele Giuseppe Esposito 
---
 python/qemu/machine.py|  4 +++-
 python/qemu/qtest.py  |  4 +++-
 tests/qemu-iotests/check  |  5 -
 tests/qemu-iotests/iotests.py |  7 ++-
 tests/qemu-iotests/testenv.py | 15 +--
 5 files changed, 29 insertions(+), 6 deletions(-)

diff --git a/python/qemu/machine.py b/python/qemu/machine.py
index c721e07d63..d7fca6b3c1 100644
--- a/python/qemu/machine.py
+++ b/python/qemu/machine.py
@@ -408,7 +408,9 @@ def _launch(self) -> None:
stderr=subprocess.STDOUT,
shell=False,
close_fds=False)
-self._post_launch()
+
+timer = None if 'gdbserver' in self._wrapper else 15.0
+self._post_launch(timer)
 
 def _early_cleanup(self) -> None:
 """
diff --git a/python/qemu/qtest.py b/python/qemu/qtest.py
index 0d01715086..4c90daf430 100644
--- a/python/qemu/qtest.py
+++ b/python/qemu/qtest.py
@@ -111,6 +111,7 @@ class QEMUQtestMachine(QEMUMachine):
 def __init__(self,
  binary: str,
  args: Sequence[str] = (),
+ wrapper: Sequence[str] = (),
  name: Optional[str] = None,
  test_dir: str = "/var/tmp",
  socket_scm_helper: Optional[str] = None,
@@ -119,7 +120,8 @@ def __init__(self,
 name = "qemu-%d" % os.getpid()
 if sock_dir is None:
 sock_dir = test_dir
-super().__init__(binary, args, name=name, test_dir=test_dir,
+super().__init__(binary, args, wrapper=wrapper, name=name,
+ test_dir=test_dir,
  socket_scm_helper=socket_scm_helper,
  sock_dir=sock_dir)
 self._qtest: Optional[QEMUQtestProtocol] = None
diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
index df9fd733ff..5a1d0518f1 100755
--- a/tests/qemu-iotests/check
+++ b/tests/qemu-iotests/check
@@ -36,6 +36,8 @@ def make_argparser() -> argparse.ArgumentParser:
help='pretty print output for make check')
 
 p.add_argument('-d', dest='debug', action='store_true', help='debug')
+p.add_argument('-gdb', action='store_true',
+   help="start gdb server on localhost. Default port is 12345")
 p.add_argument('-misalign', action='store_true',
help='misalign memory allocations')
 p.add_argument('--color', choices=['on', 'off', 'auto'],
@@ -115,7 +117,8 @@ if __name__ == '__main__':
 env = TestEnv(imgfmt=args.imgfmt, imgproto=args.imgproto,
   aiomode=args.aiomode, cachemode=args.cachemode,
   imgopts=args.imgopts, misalign=args.misalign,
-  debug=args.debug, valgrind=args.valgrind)
+  debug=args.debug, valgrind=args.valgrind,
+  gdb=args.gdb)
 
 if len(sys.argv) > 1 and sys.argv[-len(args.tests)-1] == '--':
 if not args.tests:
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 2a34eef601..138cf135ea 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -74,6 +74,10 @@
 qemu_prog = os.environ.get('QEMU_PROG', 'qemu')
 qemu_opts = os.environ.get('QEMU_OPTIONS', '').strip().split(' ')
 
+qemu_gdb = []
+if os.environ.get('GDB_QEMU'):
+qemu_gdb = ['gdbserver'] + os.environ.get('GDB_QEMU').strip().split(' ')
+
 imgfmt = os.environ.get('IMGFMT', 'raw')
 imgproto = os.environ.get('IMGPROTO', 'file')
 output_dir = os.environ.get('OUTPUT_DIR', '.')
@@ -574,7 +578,8 @@ class VM(qtest.QEMUQtestMachine):
 
 def __init__(self, path_suffix=''):
 name = "qemu%s-%d" % (path_suffix, os.getpid())
-super().__init__(qemu_prog, qemu_opts, name=name,
+super().__init__(qemu_prog, qemu_opts, wrapper=qemu_gdb,
+ name=name,
  test_dir=test_dir,
  socket_scm_helper=socket_scm_helper,
  sock_dir=sock_dir)
diff --git a/tests/qemu-iotests/testenv.py b/tests/qemu-iotests/testenv.py
index 169268f61a..62749becbd 100644
--- a/tests/qemu-iotests/testenv.py
+++ b/tests/qemu-iotests/testenv.py
@@ -72,7 +72,8 @@ class TestEnv(ContextManager['TestEnv']):

[RFC PATCH 3/3] qemu-iotests: add gdbserver option to script tests too

2021-04-01 Thread Emanuele Giuseppe Esposito

The only limitation here is that running a script with gdbserver
will make the test output mismatch with the expected
results, making the test fail.

Signed-off-by: Emanuele Giuseppe Esposito 
---
 tests/qemu-iotests/common.rc | 8 +++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc
index 65cdba5723..53a3310fee 100644
--- a/tests/qemu-iotests/common.rc
+++ b/tests/qemu-iotests/common.rc
@@ -166,8 +166,14 @@ _qemu_wrapper()
 if [ -n "${QEMU_NEED_PID}" ]; then
 echo $BASHPID > "${QEMU_TEST_DIR}/qemu-${_QEMU_HANDLE}.pid"
 fi
+
+GDB="${QEMU_PROG}"
+if [ ! -z ${GDB_QEMU} ]; then
+GDB="gdbserver ${GDB_QEMU} ${GDB}"
+fi
+
 VALGRIND_QEMU="${VALGRIND_QEMU_VM}" _qemu_proc_exec 
"${VALGRIND_LOGFILE}" \
-"$QEMU_PROG" $QEMU_OPTIONS "$@"
+   $GDB $QEMU_OPTIONS "$@"
 )
 RETVAL=$?
 _qemu_proc_valgrind_log "${VALGRIND_LOGFILE}" $RETVAL
-- 
2.30.2

[RFC PATCH 0/3] qemu-iotests: attach gdbserver to qemu instance

2021-04-01 Thread Emanuele Giuseppe Esposito

This series adds the option to attach gdbserver to the qemu_system_*
instance running in the qemu-iotests. 

Patch 1 adds the possibility to set a custom timer for the QMP socket,
necessary otherwise the connection will just drop after 15 seconds,
making the use of gdb very hard. Patch 2 adds the flag in the python
tests (or in general when a test is invoked with the check script),
patch 3 adds the same option for the tests done in bash.

This series is tested on the previous serie
"qemu-iotests: quality of life improvements"
but independent from it, so it can be applied separately.

Emanuele Giuseppe Esposito (3):
  python: qemu: add timer parameter for qmp.accept socket
  qemu-iotests: add option to attach gdbserver
  qemu-iotests: add gdbserver option to script tests too

 python/qemu/machine.py|  8 +---
 python/qemu/qtest.py  |  8 +---
 tests/qemu-iotests/check  |  5 -
 tests/qemu-iotests/common.rc  |  8 +++-
 tests/qemu-iotests/iotests.py |  7 ++-
 tests/qemu-iotests/testenv.py | 15 +--
 6 files changed, 40 insertions(+), 11 deletions(-)

-- 
2.30.2

[RFC PATCH 1/3] python: qemu: add timer parameter for qmp.accept socket

2021-04-01 Thread Emanuele Giuseppe Esposito

Extend the _post_launch function to include the timer as
parameter instead of defaulting to 15 sec.

Signed-off-by: Emanuele Giuseppe Esposito 
---
 python/qemu/machine.py | 4 ++--
 python/qemu/qtest.py   | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/python/qemu/machine.py b/python/qemu/machine.py
index 6e44bda337..c721e07d63 100644
--- a/python/qemu/machine.py
+++ b/python/qemu/machine.py
@@ -321,9 +321,9 @@ def _pre_launch(self) -> None:
 nickname=self._name
 )
 
-def _post_launch(self) -> None:
+def _post_launch(self, timer) -> None:
 if self._qmp_connection:
-self._qmp.accept()
+self._qmp.accept(timer)
 
 def _post_shutdown(self) -> None:
 """
diff --git a/python/qemu/qtest.py b/python/qemu/qtest.py
index 39a0cf62fe..0d01715086 100644
--- a/python/qemu/qtest.py
+++ b/python/qemu/qtest.py
@@ -138,9 +138,9 @@ def _pre_launch(self) -> None:
 super()._pre_launch()
 self._qtest = QEMUQtestProtocol(self._qtest_path, server=True)
 
-def _post_launch(self) -> None:
+def _post_launch(self, timer) -> None:
 assert self._qtest is not None
-super()._post_launch()
+super()._post_launch(timer)
 self._qtest.accept()
 
 def _post_shutdown(self) -> None:
-- 
2.30.2

[PATCH 2/2] iotests/qsd-jobs: Use common.qemu for the QSD

2021-04-01 Thread Max Reitz

Using common.qemu allows us to wait for specific replies, so we can for
example wait for events.  This allows starting the active commit job and
then wait for it to be ready before quitting the QSD, so we the output
is always the same.

(Strictly speaking, this is only necessary for the first test in
qsd-jobs, but we might as well make the second one use common.qemu's
infrastructure, too.)

Reported-by: Peter Maydell 
Signed-off-by: Max Reitz 
---
 tests/qemu-iotests/tests/qsd-jobs | 55 ---
 tests/qemu-iotests/tests/qsd-jobs.out | 10 -
 2 files changed, 49 insertions(+), 16 deletions(-)

diff --git a/tests/qemu-iotests/tests/qsd-jobs 
b/tests/qemu-iotests/tests/qsd-jobs
index 972b6b3898..af7f886f15 100755
--- a/tests/qemu-iotests/tests/qsd-jobs
+++ b/tests/qemu-iotests/tests/qsd-jobs
@@ -38,6 +38,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
 cd ..
 . ./common.rc
 . ./common.filter
+. ./common.qemu
 
 _supported_fmt qcow2
 _supported_proto generic
@@ -52,32 +53,58 @@ echo "=== Job still present at shutdown ==="
 echo
 
 # Just make sure that this doesn't crash
-$QSD --chardev stdio,id=stdio --monitor chardev=stdio \
+qsd=y _launch_qemu \
 --blockdev node-name=file0,driver=file,filename="$TEST_IMG" \
---blockdev node-name=fmt0,driver=qcow2,file=file0 <

[PATCH] iotests/qsd-jobs: Filter events in the first test

2021-04-01 Thread Max Reitz

The job may or may not be ready before the 'quit' is issued.  Whether it
is is irrelevant; for the purpose of the test, it only needs to still be
there.  Filter the job status change and READY events from the output so
it becomes reliable.

Reported-by: Peter Maydell 
Suggested-by: Vladimir Sementsov-Ogievskiy 
Signed-off-by: Max Reitz 
---
This is an alternative to "iotests/qsd-jobs: Use common.qemu for the
QSD".  I can't disagree with Vladimir that perhaps this test just should
not care about the job status events, because all that matters is that
the job is still running when 'quit' is issued (which we can see by the
(not filtered) COMPLETED event coming afterwards), but it seems strange
just not to care about what state the job is actually in when we quit
the QSD.
It’s definitely much simpler than the common.qemu variant.
---
 tests/qemu-iotests/tests/qsd-jobs |  5 -
 tests/qemu-iotests/tests/qsd-jobs.out | 10 --
 2 files changed, 4 insertions(+), 11 deletions(-)

diff --git a/tests/qemu-iotests/tests/qsd-jobs 
b/tests/qemu-iotests/tests/qsd-jobs
index 972b6b3898..510bf0a9dc 100755
--- a/tests/qemu-iotests/tests/qsd-jobs
+++ b/tests/qemu-iotests/tests/qsd-jobs
@@ -52,9 +52,12 @@ echo "=== Job still present at shutdown ==="
 echo
 
 # Just make sure that this doesn't crash
+# (Filter job status and READY events, because their order may differ
+# between runs, particularly around when 'quit' is issued)
 $QSD --chardev stdio,id=stdio --monitor chardev=stdio \
 --blockdev node-name=file0,driver=file,filename="$TEST_IMG" \
---blockdev node-name=fmt0,driver=qcow2,file=file0 <

[PATCH 1/2] iotests/common.qemu: Allow using the QSD

2021-04-01 Thread Max Reitz

For block things, we often do not need to run all of qemu, so allow
using the qemu-storage-daemon instead.

Signed-off-by: Max Reitz 
---
 tests/qemu-iotests/common.qemu | 53 +++---
 1 file changed, 43 insertions(+), 10 deletions(-)

diff --git a/tests/qemu-iotests/common.qemu b/tests/qemu-iotests/common.qemu
index 0fc52d20d7..ca8bb43c63 100644
--- a/tests/qemu-iotests/common.qemu
+++ b/tests/qemu-iotests/common.qemu
@@ -291,6 +291,8 @@ _wait_event()
 # $qmp_pretty: Set this variable to 'y' to enable QMP pretty printing.
 # $keep_stderr: Set this variable to 'y' to keep QEMU's stderr output on 
stderr.
 #   If this variable is empty, stderr will be redirected to stdout.
+# $qsd: Set this variable to 'y' to use the QSD instead of QEMU.
+#   stdout and stderr are never redirected when using the QSD.
 # Returns:
 # $QEMU_HANDLE: set to a handle value to communicate with this QEMU instance.
 #
@@ -300,18 +302,31 @@ _launch_qemu()
 local fifo_out=
 local fifo_in=
 
+if [[ $qsd = 'y' ]]; then
+mon_arg='--monitor'
+else
+mon_arg='-mon'
+fi
+
 if (shopt -s nocasematch; [[ "${qemu_comm_method}" == "monitor" ]])
 then
-comm="-monitor stdio"
+comm=(--chardev stdio,id=pipe
+  $mon_arg pipe,mode=readline)
 else
 local qemu_comm_method="qmp"
 if [ "$qmp_pretty" = "y" ]; then
-comm="-monitor none -qmp-pretty stdio"
+comm=(--chardev stdio,id=pipe
+  $mon_arg pipe,mode=control,pretty=on)
 else
-comm="-monitor none -qmp stdio"
+comm=(--chardev stdio,id=pipe
+  $mon_arg pipe,mode=control,pretty=off)
 fi
 fi
 
+if [[ $qsd != 'y' ]]; then
+comm=(-monitor none "${comm[@]}")
+fi
+
 fifo_out=${QEMU_FIFO_OUT}_${_QEMU_HANDLE}
 fifo_in=${QEMU_FIFO_IN}_${_QEMU_HANDLE}
 mkfifo "${fifo_out}"
@@ -322,15 +337,23 @@ _launch_qemu()
 object_options="--object secret,id=keysec0,data=$IMGKEYSECRET"
 fi
 
+if [[ $qsd = 'y' ]]; then
+cmd=$QSD
+args=()
+else
+cmd=$QEMU
+args=(-nographic -serial none)
+fi
+args+=(${object_options} "${comm[@]}")
+args+=("$@")
+
+# Just set both QEMU_NEED_PID and QSD_NEED_PID, it can't hurt.
 if [ -z "$keep_stderr" ]; then
-QEMU_NEED_PID='y'\
-${QEMU} ${object_options} -nographic -serial none ${comm} "${@}" 
>"${fifo_out}" \
-   2>&1 \
-   <"${fifo_in}" &
+QEMU_NEED_PID='y' QSD_NEED_PID='y' $cmd "${args[@]}" \
+>"$fifo_out" 2>&1 <"$fifo_in" &
 elif [ "$keep_stderr" = "y" ]; then
-QEMU_NEED_PID='y'\
-${QEMU} ${object_options} -nographic -serial none ${comm} "${@}" 
>"${fifo_out}" \
-   <"${fifo_in}" &
+QEMU_NEED_PID='y' QSD_NEED_PID='y' $cmd "${args[@]}" \
+>"$fifo_out" <"$fifo_in" &
 else
 exit 1
 fi
@@ -360,6 +383,16 @@ _launch_qemu()
 silent=yes _timed_wait_for ${_QEMU_HANDLE} "^}"
 fi
 fi
+
+if [[ $qsd = 'y' ]]; then
+# Wait for PID file, then move it to where qemu would put it
+pidfile="$QEMU_TEST_DIR/qemu-storage-daemon.pid"
+while [[ ! -f $pidfile ]]; do
+sleep 0.5
+done
+mv "$pidfile" "$QEMU_TEST_DIR/qemu-${_QEMU_HANDLE}.pid"
+fi
+
 QEMU_HANDLE=${_QEMU_HANDLE}
 let _QEMU_HANDLE++
 }
-- 
2.29.2

[PATCH 0/2] iotests/qsd-jobs: Use common.qemu for the QSD

2021-04-01 Thread Max Reitz

(Alternative to: “iotests/qsd-jobs: Filter events in the first test”)

Hi,

The qsd-jobs test has kind of unreliable output, because sometimes the
job is ready before ‘quit’, and sometimes it is not.  This series
presents one approach to fix that, which is to extend common.qemu to
allow running the storage daemon instead of qemu, and then to use that
in qsd-jobs to wait for the BLOCK_JOB_READY event before issuing the
‘quit’ command.

I took patch 1 from my “qcow2: Improve refcount structure rebuilding”
series.
(https://lists.nongnu.org/archive/html/qemu-block/2021-03/msg00654.html)

As noted above, this series is an alternative to “iotests/qsd-jobs:
Filter events in the first test”.  I like this series here better
because I’d prefer it if tests that do QMP actually check the output so
they control what’s really happening.
On the other hand, this may be too complicated for 6.0, and we might
want to fix qsd-jobs in 6.0.


Max Reitz (2):
  iotests/common.qemu: Allow using the QSD
  iotests/qsd-jobs: Use common.qemu for the QSD

 tests/qemu-iotests/common.qemu| 53 +-
 tests/qemu-iotests/tests/qsd-jobs | 55 ---
 tests/qemu-iotests/tests/qsd-jobs.out | 10 -
 3 files changed, 92 insertions(+), 26 deletions(-)

-- 
2.29.2

Re: [PULL for-6.0 0/6] Block patches

2021-04-01 Thread Peter Maydell

On Wed, 31 Mar 2021 at 10:51, Stefan Hajnoczi  wrote:
>
> The following changes since commit 6d40ce00c1166c317e298ad82ecf10e650c4f87d:
>
>   Update version for v6.0.0-rc1 release (2021-03-30 18:19:07 +0100)
>
> are available in the Git repository at:
>
>   https://gitlab.com/stefanha/qemu.git tags/block-pull-request
>
> for you to fetch changes up to b6489ac06695e257ea0a9841364577e247fdee30:
>
>   test-coroutine: Add rwlock downgrade test (2021-03-31 10:44:21 +0100)
>
> 
> Pull request
>
> A fix for VDI image files and more generally for CoRwlock.
>
> 


Applied, thanks.

Please update the changelog at https://wiki.qemu.org/ChangeLog/6.0
for any user-visible changes.

-- PMM

Re: [PATCH] iotests: Test mirror-top filter permissions

2021-04-01 Thread Max Reitz


On 01.04.21 10:32, Vladimir Sementsov-Ogievskiy wrote:

31.03.2021 15:28, Max Reitz wrote:

Add a test accompanying commit 53431b9086b2832ca1aeff0c55e186e9ed79bd11
("block/mirror: Fix mirror_top's permissions").

Signed-off-by: Max Reitz 
---
  tests/qemu-iotests/tests/mirror-top-perms | 121 ++
  tests/qemu-iotests/tests/mirror-top-perms.out |   5 +
  2 files changed, 126 insertions(+)
  create mode 100755 tests/qemu-iotests/tests/mirror-top-perms
  create mode 100644 tests/qemu-iotests/tests/mirror-top-perms.out

diff --git a/tests/qemu-iotests/tests/mirror-top-perms 
b/tests/qemu-iotests/tests/mirror-top-perms

new file mode 100755
index 00..451a0666f8
--- /dev/null
+++ b/tests/qemu-iotests/tests/mirror-top-perms
@@ -0,0 +1,121 @@
+#!/usr/bin/env python3
+# group: rw
+#
+# Test permissions taken by the mirror-top filter
+#
+# Copyright (C) 2021 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see .
+#
+
+import os
+import iotests
+from iotests import qemu_img
+
+# Import qemu after iotests.py has amended sys.path
+# pylint: disable=wrong-import-order
+import qemu
+
+
+image_size = 1 * 1024 * 1024
+source = os.path.join(iotests.test_dir, 'source.img')
+
+
+class TestMirrorTopPerms(iotests.QMPTestCase):
+    def setUp(self):
+    assert qemu_img('create', '-f', iotests.imgfmt, source,
+    str(image_size)) == 0
+    self.vm = iotests.VM()
+    self.vm.add_drive(source)
+
self.vm.add_blockdev(f'null-co,node-name=null,size={image_size}')

+    self.vm.launch()
+
+    # Will be created by the test function itself
+    self.vm_b = None
+
+    def tearDown(self):
+    try:
+    self.vm.shutdown()
+    except qemu.machine.AbnormalShutdown:
+    pass
+
+    if self.vm_b is not None:
+    self.vm_b.shutdown()
+
+    os.remove(source)
+
+    def test_cancel(self):
+    """
+    Before commit 53431b9086b28, mirror-top used to not take any
+    permissions but WRITE and share all permissions.  Because it
+    is inserted between the source's original parents and the
+    source, there generally was no parent that would have taken or
+    unshared any permissions on the source, which means that an
+    external process could access the image unhindered by locks.
+    (Unless there was a parent above the protocol node that would
+    take its own locks, e.g. a format driver.)
+    This is bad enough, but if the mirror job is then cancelled,
+    the mirroring VM tries to take back the image, restores the
+    original permissions taken and unshared, and assumes this must
+    just work.  But it will not, and so the VM aborts.
+
+    Commit 53431b9086b28 made mirror keep the original permissions
+    and so no other process can "steal" the image.
+
+    (Note that you cannot really do the same with the target image
+    and then completing the job, because the mirror job always
+    took/unshared the correct permissions on the target.  For
+    example, it does not share READ_CONSISTENT, which makes it
+    difficult to let some other qemu process open the image.)
+    """
+
+    result = self.vm.qmp('blockdev-mirror',
+ job_id='mirror',
+ device='drive0',
+ target='null',
+ sync='full')
+    self.assert_qmp(result, 'return', {})
+
+    self.vm.event_wait('BLOCK_JOB_READY')
+
+    # We want this to fail because the image cannot be locked.
+    # If it does not fail, continue still and see what happens.


This comment is about vm_b.launch(), not about creating vm object. 
Probably better to move it down


I kind of meant this as a general comment on what this VM is for.  I 
think I kind of like any comment here, and I don’t see a practical 
advantage in having this comment above the try-except, because the whole 
“launch VM, see it fail” block is kind of one monolithic concept.



+    self.vm_b = iotests.VM(path_suffix='b')
+    # Must use -blockdev -device so we can use share-rw.
+    # (And we need share-rw=on because mirror-top was always
+    # forced to take the WRITE permission so it can write to the
+    # source image.)
+

[BUG FIX][PATCH v3 0/3] vhost-user-blk: fix bug on device disconnection during initialization

2021-04-01 Thread Denis Plotnikov


This is a series fixing a bug in host-user-blk.
Is there any chance for it to be considered for the next rc?

Thanks!

Denis

On 29.03.2021 16:44, Denis Plotnikov wrote:


ping!

On 25.03.2021 18:12, Denis Plotnikov wrote:

v3:
   * 0003: a new patch added fixing the problem on vm shutdown
 I stumbled on this bug after v2 sending.
   * 0001: gramma fixing (Raphael)
   * 0002: commit message fixing (Raphael)

v2:
   * split the initial patch into two (Raphael)
   * rename init to realized (Raphael)
   * remove unrelated comment (Raphael)

When the vhost-user-blk device lose the connection to the daemon during
the initialization phase it kills qemu because of the assert in the code.
The series fixes the bug.

0001 is preparation for the fix
0002 fixes the bug, patch description has the full motivation for the series
0003 (added in v3) fix bug on vm shutdown

Denis Plotnikov (3):
   vhost-user-blk: use different event handlers on initialization
   vhost-user-blk: perform immediate cleanup if disconnect on
 initialization
   vhost-user-blk: add immediate cleanup on shutdown

  hw/block/vhost-user-blk.c | 79 ---
  1 file changed, 48 insertions(+), 31 deletions(-)

Re: [PATCH v3 2/4] block: check for sys/disk.h

2021-04-01 Thread Paolo Bonzini


On 01/04/21 10:03, Philippe Mathieu-Daudé wrote:

On 4/1/21 7:08 AM, Joelle van Dyne wrote:

On Mon, Mar 15, 2021 at 11:03 AM Joelle van Dyne  wrote:


Some BSD platforms do not have this header.

Reviewed-by: Peter Maydell 
Reviewed-by: Philippe Mathieu-Daudé 
Signed-off-by: Joelle van Dyne 


Please bear with me as I am still new to this, but what happens to the
three patches that are reviewed if the last patch does not get
reviewed? Do the reviewed patches still get to make it into 6.0? I am
willing to drop the unreviewed patch if there are issues. Thanks.


I guess this is bad timing, as this time in year various maintainers
are on vacations. Cc'ing Paolo as he sometimes take generic/block
patches.


I didn't notice this series before, and it was posted a bit late (one 
day before feature freeze).  I have queued it, so Joelle need not do 
anything else, but I won't include it in my pull request for 6.0.


Paolo

Re: [PATCH] iotests: Test mirror-top filter permissions

2021-04-01 Thread Vladimir Sementsov-Ogievskiy


31.03.2021 15:28, Max Reitz wrote:

Add a test accompanying commit 53431b9086b2832ca1aeff0c55e186e9ed79bd11
("block/mirror: Fix mirror_top's permissions").

Signed-off-by: Max Reitz 
---
  tests/qemu-iotests/tests/mirror-top-perms | 121 ++
  tests/qemu-iotests/tests/mirror-top-perms.out |   5 +
  2 files changed, 126 insertions(+)
  create mode 100755 tests/qemu-iotests/tests/mirror-top-perms
  create mode 100644 tests/qemu-iotests/tests/mirror-top-perms.out

diff --git a/tests/qemu-iotests/tests/mirror-top-perms 
b/tests/qemu-iotests/tests/mirror-top-perms
new file mode 100755
index 00..451a0666f8
--- /dev/null
+++ b/tests/qemu-iotests/tests/mirror-top-perms
@@ -0,0 +1,121 @@
+#!/usr/bin/env python3
+# group: rw
+#
+# Test permissions taken by the mirror-top filter
+#
+# Copyright (C) 2021 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see .
+#
+
+import os
+import iotests
+from iotests import qemu_img
+
+# Import qemu after iotests.py has amended sys.path
+# pylint: disable=wrong-import-order
+import qemu
+
+
+image_size = 1 * 1024 * 1024
+source = os.path.join(iotests.test_dir, 'source.img')
+
+
+class TestMirrorTopPerms(iotests.QMPTestCase):
+def setUp(self):
+assert qemu_img('create', '-f', iotests.imgfmt, source,
+str(image_size)) == 0
+self.vm = iotests.VM()
+self.vm.add_drive(source)
+self.vm.add_blockdev(f'null-co,node-name=null,size={image_size}')
+self.vm.launch()
+
+# Will be created by the test function itself
+self.vm_b = None
+
+def tearDown(self):
+try:
+self.vm.shutdown()
+except qemu.machine.AbnormalShutdown:
+pass
+
+if self.vm_b is not None:
+self.vm_b.shutdown()
+
+os.remove(source)
+
+def test_cancel(self):
+"""
+Before commit 53431b9086b28, mirror-top used to not take any
+permissions but WRITE and share all permissions.  Because it
+is inserted between the source's original parents and the
+source, there generally was no parent that would have taken or
+unshared any permissions on the source, which means that an
+external process could access the image unhindered by locks.
+(Unless there was a parent above the protocol node that would
+take its own locks, e.g. a format driver.)
+This is bad enough, but if the mirror job is then cancelled,
+the mirroring VM tries to take back the image, restores the
+original permissions taken and unshared, and assumes this must
+just work.  But it will not, and so the VM aborts.
+
+Commit 53431b9086b28 made mirror keep the original permissions
+and so no other process can "steal" the image.
+
+(Note that you cannot really do the same with the target image
+and then completing the job, because the mirror job always
+took/unshared the correct permissions on the target.  For
+example, it does not share READ_CONSISTENT, which makes it
+difficult to let some other qemu process open the image.)
+"""
+
+result = self.vm.qmp('blockdev-mirror',
+ job_id='mirror',
+ device='drive0',
+ target='null',
+ sync='full')
+self.assert_qmp(result, 'return', {})
+
+self.vm.event_wait('BLOCK_JOB_READY')
+
+# We want this to fail because the image cannot be locked.
+# If it does not fail, continue still and see what happens.


This comment is about vm_b.launch(), not about creating vm object. Probably 
better to move it down


+self.vm_b = iotests.VM(path_suffix='b')
+# Must use -blockdev -device so we can use share-rw.
+# (And we need share-rw=on because mirror-top was always
+# forced to take the WRITE permission so it can write to the
+# source image.)
+self.vm_b.add_blockdev(f'file,node-name=drive0,filename={source}')
+self.vm_b.add_device('virtio-blk,drive=drive0,share-rw=on')
+try:
+self.vm_b.launch()
+print('ERROR: VM B launched successfully, this should not have '
+  'happened')


probably iotests.log() is better here.


+except qemu.qmp.QMPConnectError:

Re: [PATCH v3 2/4] block: check for sys/disk.h

2021-04-01 Thread Philippe Mathieu-Daudé

On 4/1/21 7:08 AM, Joelle van Dyne wrote:
> On Mon, Mar 15, 2021 at 11:03 AM Joelle van Dyne  wrote:
>>
>> Some BSD platforms do not have this header.
>>
>> Reviewed-by: Peter Maydell 
>> Reviewed-by: Philippe Mathieu-Daudé 
>> Signed-off-by: Joelle van Dyne 
> 
> Please bear with me as I am still new to this, but what happens to the
> three patches that are reviewed if the last patch does not get
> reviewed? Do the reviewed patches still get to make it into 6.0? I am
> willing to drop the unreviewed patch if there are issues. Thanks.

I guess this is bad timing, as this time in year various maintainers
are on vacations. Cc'ing Paolo as he sometimes take generic/block
patches.

Regards,

Phil.

42 matches

Mail list logo