From: Connor Kuehl <ckuehl@redhat.com>

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 <ckuehl@redhat.com>

Reviewed-by: Max Reitz <mreitz@redhat.com>

Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>

Message-Id: <20210421212343.85524-2-ckuehl@redhat.com>

Signed-off-by: Max Reitz <mreitz@redhat.com>

---

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 XXXXXXX..XXXXXXX 100644

--- a/tests/qemu-iotests/231.out

+++ b/tests/qemu-iotests/231.out

@@ -XXX,XX +XXX,XX @@

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.31.1

From: Connor Kuehl <ckuehl@redhat.com>

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. Furthermore, this code is identical to how

qemu_rbd_next_tok() seeks its next token, so incorporate this custom

strchr into the body of that function to reduce duplication.

Reported-by: Han Han <hhan@redhat.com>

Fixes: https://bugzilla.redhat.com/1873913

Signed-off-by: Connor Kuehl <ckuehl@redhat.com>

Message-Id: <20210421212343.85524-3-ckuehl@redhat.com>

Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>

Signed-off-by: Max Reitz <mreitz@redhat.com>

---

block/rbd.c | 32 +++++++++++++++++++++-----------

tests/qemu-iotests/231 | 4 ++++

tests/qemu-iotests/231.out | 3 +++

3 files changed, 28 insertions(+), 11 deletions(-)

diff --git a/block/rbd.c b/block/rbd.c

index XXXXXXX..XXXXXXX 100644

--- a/block/rbd.c

+++ b/block/rbd.c

@@ -XXX,XX +XXX,XX @@ static int qemu_rbd_connect(rados_t *cluster, rados_ioctx_t *io_ctx,

const char *keypairs, const char *secretid,

Error **errp);

+static char *qemu_rbd_strchr(char *src, char delim)

+{

+ char *p;

+

+ for (p = src; *p; ++p) {

+ if (*p == delim) {

+ return p;

+ }

+ if (*p == '\\' && p[1] != '\0') {

+ ++p;

+ }

+ }

+

+ return NULL;

+}

+

+

static char *qemu_rbd_next_tok(char *src, char delim, char **p)

{

char *end;

*p = NULL;

- for (end = src; *end; ++end) {

- if (*end == delim) {

- break;

- }

- if (*end == '\\' && end[1] != '\0') {

- end++;

- }

- }

- if (*end == delim) {

+ end = qemu_rbd_strchr(src, delim);

+ if (end) {

*p = end + 1;

*end = '\0';

}

@@ -XXX,XX +XXX,XX @@ 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, '@', &p);

found_str = qemu_rbd_next_tok(p, ':', &p);

@@ -XXX,XX +XXX,XX @@ static void qemu_rbd_parse_filename(const char *filename, QDict *options,

image_name = qemu_rbd_next_tok(p, ':', &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, '/', &image_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 XXXXXXX..XXXXXXX 100755

--- a/tests/qemu-iotests/231

+++ b/tests/qemu-iotests/231

@@ -XXX,XX +XXX,XX @@ _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 XXXXXXX..XXXXXXX 100644

--- a/tests/qemu-iotests/231.out

+++ b/tests/qemu-iotests/231.out

@@ -XXX,XX +XXX,XX @@ 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.31.1

From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

Max reported the following bug:

$ ./qemu-img create -f raw src.img 1G

$ ./qemu-img create -f raw dst.img 1G

$ (echo '

{"execute":"qmp_capabilities"}

{"execute":"blockdev-mirror",

"arguments":{"job-id":"mirror",

"device":"source",

"target":"target",

"sync":"full",

"filter-node-name":"mirror-top"}}

'; sleep 3; echo '

{"execute":"human-monitor-command",

"arguments":{"command-line":

"qemu-io mirror-top \"write 0 1G\""}}') \

| x86_64-softmmu/qemu-system-x86_64 \

-qmp stdio \

-blockdev file,node-name=source,filename=src.img \

-blockdev file,node-name=target,filename=dst.img \

-object iothread,id=iothr0 \

-device virtio-blk,drive=source,iothread=iothr0

crashes:

0 raise () at /usr/lib/libc.so.6

1 abort () at /usr/lib/libc.so.6

2 error_exit

(err=<optimized out>,

msg=msg@entry=0x55fbb1634790 <__func__.27> "qemu_mutex_unlock_impl")

at ../util/qemu-thread-posix.c:37

3 qemu_mutex_unlock_impl

(mutex=mutex@entry=0x55fbb25ab6e0,

file=file@entry=0x55fbb1636957 "../util/async.c",

line=line@entry=650)

at ../util/qemu-thread-posix.c:109

4 aio_context_release (ctx=ctx@entry=0x55fbb25ab680) at ../util/async.c:650

5 bdrv_do_drained_begin

(bs=bs@entry=0x55fbb3a87000, recursive=recursive@entry=false,

parent=parent@entry=0x0,

ignore_bds_parents=ignore_bds_parents@entry=false,

poll=poll@entry=true) at ../block/io.c:441

6 bdrv_do_drained_begin

(poll=true, ignore_bds_parents=false, parent=0x0, recursive=false,

bs=0x55fbb3a87000) at ../block/io.c:448

7 blk_drain (blk=0x55fbb26c5a00) at ../block/block-backend.c:1718

8 blk_unref (blk=0x55fbb26c5a00) at ../block/block-backend.c:498

9 blk_unref (blk=0x55fbb26c5a00) at ../block/block-backend.c:491

10 hmp_qemu_io (mon=0x7fffaf3fc7d0, qdict=<optimized out>)

at ../block/monitor/block-hmp-cmds.c:628

man pthread_mutex_unlock

...

EPERM The mutex type is PTHREAD_MUTEX_ERRORCHECK or

PTHREAD_MUTEX_RECURSIVE, or the mutex is a robust mutex, and the

current thread does not own the mutex.

So, thread doesn't own the mutex. And we have iothread here.

Next, note that AIO_WAIT_WHILE() documents that ctx must be acquired

exactly once by caller. But where is it acquired in the call stack?

Seems nowhere.

qemuio_command do acquire aio context.. But we need context acquired

around blk_unref() as well and actually around blk_insert_bs() too.

Let's refactor qemuio_command so that it doesn't acquire aio context

but callers do that instead. This way we can cleanly acquire aio

context in hmp_qemu_io() around all three calls.

Reported-by: Max Reitz <mreitz@redhat.com>

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

Message-Id: <20210423134233.51495-1-vsementsov@virtuozzo.com>

[mreitz: Fixed comment]

Signed-off-by: Max Reitz <mreitz@redhat.com>

---

block/monitor/block-hmp-cmds.c | 31 +++++++++++++++++++++----------

qemu-io-cmds.c | 8 ++++----

qemu-io.c | 17 +++++++++++++++--

3 files changed, 40 insertions(+), 16 deletions(-)

diff --git a/block/monitor/block-hmp-cmds.c b/block/monitor/block-hmp-cmds.c

index XXXXXXX..XXXXXXX 100644

--- a/block/monitor/block-hmp-cmds.c

+++ b/block/monitor/block-hmp-cmds.c

@@ -XXX,XX +XXX,XX @@ void hmp_eject(Monitor *mon, const QDict *qdict)

void hmp_qemu_io(Monitor *mon, const QDict *qdict)

{

- BlockBackend *blk;

+ BlockBackend *blk = NULL;

+ BlockDriverState *bs = NULL;

BlockBackend *local_blk = NULL;

+ AioContext *ctx = NULL;

bool qdev = qdict_get_try_bool(qdict, "qdev", false);

const char *device = qdict_get_str(qdict, "device");

const char *command = qdict_get_str(qdict, "command");

@@ -XXX,XX +XXX,XX @@ void hmp_qemu_io(Monitor *mon, const QDict *qdict)

} else {

blk = blk_by_name(device);

if (!blk) {

- BlockDriverState *bs = bdrv_lookup_bs(NULL, device, &err);

- if (bs) {

- blk = local_blk = blk_new(bdrv_get_aio_context(bs),

- 0, BLK_PERM_ALL);

- ret = blk_insert_bs(blk, bs, &err);

- if (ret < 0) {

- goto fail;

- }

- } else {

+ bs = bdrv_lookup_bs(NULL, device, &err);

+ if (!bs) {

goto fail;

}

}

}

+ ctx = blk ? blk_get_aio_context(blk) : bdrv_get_aio_context(bs);

+ aio_context_acquire(ctx);

+

+ if (bs) {

+ blk = local_blk = blk_new(bdrv_get_aio_context(bs), 0, BLK_PERM_ALL);

+ ret = blk_insert_bs(blk, bs, &err);

+ if (ret < 0) {

+ goto fail;

+ }

+ }

+

/*

* Notably absent: Proper permission management. This is sad, but it seems

* almost impossible to achieve without changing the semantics and thereby

@@ -XXX,XX +XXX,XX @@ void hmp_qemu_io(Monitor *mon, const QDict *qdict)

fail:

blk_unref(local_blk);

+

+ if (ctx) {

+ aio_context_release(ctx);

+ }

+

hmp_handle_error(mon, err);

}

diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c

index XXXXXXX..XXXXXXX 100644

--- a/qemu-io-cmds.c

+++ b/qemu-io-cmds.c

@@ -XXX,XX +XXX,XX @@ static const cmdinfo_t help_cmd = {

.oneline = "help for one or all commands",

};

+/*

+ * Called with aio context of blk acquired. Or with qemu_get_aio_context()

+ * context acquired if blk is NULL.

+ */

int qemuio_command(BlockBackend *blk, const char *cmd)

{

- AioContext *ctx;

char *input;

const cmdinfo_t *ct;

char **v;

@@ -XXX,XX +XXX,XX @@ int qemuio_command(BlockBackend *blk, const char *cmd)

if (c) {

ct = find_command(v[0]);

if (ct) {

- ctx = blk ? blk_get_aio_context(blk) : qemu_get_aio_context();

- aio_context_acquire(ctx);

ret = command(blk, ct, c, v);

- aio_context_release(ctx);

} else {

fprintf(stderr, "command \"%s\" not found\n", v[0]);

ret = -EINVAL;

diff --git a/qemu-io.c b/qemu-io.c

index XXXXXXX..XXXXXXX 100644

--- a/qemu-io.c

+++ b/qemu-io.c

@@ -XXX,XX +XXX,XX @@ static void prep_fetchline(void *opaque)

*fetchable= 1;

}

+static int do_qemuio_command(const char *cmd)

+{

+ int ret;

+ AioContext *ctx =

+ qemuio_blk ? blk_get_aio_context(qemuio_blk) : qemu_get_aio_context();

+

+ aio_context_acquire(ctx);

+ ret = qemuio_command(qemuio_blk, cmd);

+ aio_context_release(ctx);

+

+ return ret;

+}

+

static int command_loop(void)

{

int i, fetchable = 0, prompted = 0;

@@ -XXX,XX +XXX,XX @@ static int command_loop(void)

char *input;

for (i = 0; !quit_qemu_io && i < ncmdline; i++) {

- ret = qemuio_command(qemuio_blk, cmdline[i]);

+ ret = do_qemuio_command(cmdline[i]);

if (ret < 0) {

last_error = ret;

}

@@ -XXX,XX +XXX,XX @@ static int command_loop(void)

if (input == NULL) {

break;

}

- ret = qemuio_command(qemuio_blk, input);

+ ret = do_qemuio_command(input);

g_free(input);

if (ret < 0) {

--

2.31.1

From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

If mirror is READY than cancel operation is not discarding the whole

result of the operation, but instead it's a documented way get a

point-in-time snapshot of source disk.

So, we should not cancel any requests if mirror is READ and

force=false. Let's fix that case.

Note, that bug that we have before this commit is not critical, as the

only .bdrv_cancel_in_flight implementation is nbd_cancel_in_flight()

and it cancels only requests waiting for reconnection, so it should be

rare case.

Fixes: 521ff8b779b11c394dbdc43f02e158dd99df308a

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

Message-Id: <20210421075858.40197-1-vsementsov@virtuozzo.com>

Signed-off-by: Max Reitz <mreitz@redhat.com>

---

include/block/block_int.h | 2 +-

include/qemu/job.h | 2 +-

block/backup.c | 2 +-

block/mirror.c | 6 ++++--

job.c | 2 +-

tests/qemu-iotests/264 | 2 +-

6 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/include/block/block_int.h b/include/block/block_int.h

index XXXXXXX..XXXXXXX 100644

--- a/include/block/block_int.h

+++ b/include/block/block_int.h

@@ -XXX,XX +XXX,XX @@ struct BlockDriver {

* of in-flight requests, so don't waste the time if possible.

*

* One example usage is to avoid waiting for an nbd target node reconnect

- * timeout during job-cancel.

+ * timeout during job-cancel with force=true.

*/

void (*bdrv_cancel_in_flight)(BlockDriverState *bs);

diff --git a/include/qemu/job.h b/include/qemu/job.h

index XXXXXXX..XXXXXXX 100644

--- a/include/qemu/job.h

+++ b/include/qemu/job.h

@@ -XXX,XX +XXX,XX @@ struct JobDriver {

/**

* If the callback is not NULL, it will be invoked in job_cancel_async

*/

- void (*cancel)(Job *job);

+ void (*cancel)(Job *job, bool force);

/** Called when the job is freed */

diff --git a/block/backup.c b/block/backup.c

index XXXXXXX..XXXXXXX 100644

--- a/block/backup.c

+++ b/block/backup.c

@@ -XXX,XX +XXX,XX @@ static void coroutine_fn backup_set_speed(BlockJob *job, int64_t speed)

}

}

-static void backup_cancel(Job *job)

+static void backup_cancel(Job *job, bool force)

{

BackupBlockJob *s = container_of(job, BackupBlockJob, common.job);

diff --git a/block/mirror.c b/block/mirror.c

index XXXXXXX..XXXXXXX 100644

--- a/block/mirror.c

+++ b/block/mirror.c

@@ -XXX,XX +XXX,XX @@ static bool mirror_drained_poll(BlockJob *job)

return !!s->in_flight;

}

-static void mirror_cancel(Job *job)

+static void mirror_cancel(Job *job, bool force)

{

MirrorBlockJob *s = container_of(job, MirrorBlockJob, common.job);

BlockDriverState *target = blk_bs(s->target);

- bdrv_cancel_in_flight(target);

+ if (force || !job_is_ready(job)) {

+ bdrv_cancel_in_flight(target);

+ }

}

static const BlockJobDriver mirror_job_driver = {

diff --git a/job.c b/job.c

index XXXXXXX..XXXXXXX 100644

--- a/job.c

+++ b/job.c

@@ -XXX,XX +XXX,XX @@ static int job_finalize_single(Job *job)

static void job_cancel_async(Job *job, bool force)

{

if (job->driver->cancel) {

- job->driver->cancel(job);

+ job->driver->cancel(job, force);

}

if (job->user_paused) {

/* Do not call job_enter here, the caller will handle it. */

diff --git a/tests/qemu-iotests/264 b/tests/qemu-iotests/264

index XXXXXXX..XXXXXXX 100755

--- a/tests/qemu-iotests/264

+++ b/tests/qemu-iotests/264

@@ -XXX,XX +XXX,XX @@ class TestNbdReconnect(iotests.QMPTestCase):

self.assert_qmp(result, 'return', {})

def cancel_job(self):

- result = self.vm.qmp('block-job-cancel', device='drive0')

+ result = self.vm.qmp('block-job-cancel', device='drive0', force=True)

self.assert_qmp(result, 'return', {})

start_t = time.time()

--

2.31.1