* [Qemu-devel] [PULL 01/14] file-posix: Use error API properly
2018-11-12 17:05 [Qemu-devel] [PULL 00/14] Block layer patches Kevin Wolf
@ 2018-11-12 17:05 ` Kevin Wolf
2018-11-12 17:05 ` [Qemu-devel] [PULL 02/14] blockdev: handle error on block latency histogram set error Kevin Wolf
` (13 subsequent siblings)
14 siblings, 0 replies; 16+ messages in thread
From: Kevin Wolf @ 2018-11-12 17:05 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, peter.maydell, qemu-devel
From: Fam Zheng <famz@redhat.com>
Use error_report for situations that affect user operation (i.e. we're
actually returning error), and warn_report/warn_report_err when some
less critical error happened but the user operation can still carry on.
For raw_normalize_devicepath, add Error parameter to propagate to
its callers.
Suggested-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Fam Zheng <famz@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
block/file-posix.c | 39 ++++++++++++++++-----------------------
1 file changed, 16 insertions(+), 23 deletions(-)
diff --git a/block/file-posix.c b/block/file-posix.c
index 0c1b81ce4b..074f0ce2b3 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -205,7 +205,7 @@ static int cdrom_reopen(BlockDriverState *bs);
#endif
#if defined(__NetBSD__)
-static int raw_normalize_devicepath(const char **filename)
+static int raw_normalize_devicepath(const char **filename, Error **errp)
{
static char namebuf[PATH_MAX];
const char *dp, *fname;
@@ -214,8 +214,7 @@ static int raw_normalize_devicepath(const char **filename)
fname = *filename;
dp = strrchr(fname, '/');
if (lstat(fname, &sb) < 0) {
- fprintf(stderr, "%s: stat failed: %s\n",
- fname, strerror(errno));
+ error_setg_errno(errp, errno, "%s: stat failed", fname);
return -errno;
}
@@ -229,14 +228,13 @@ static int raw_normalize_devicepath(const char **filename)
snprintf(namebuf, PATH_MAX, "%.*s/r%s",
(int)(dp - fname), fname, dp + 1);
}
- fprintf(stderr, "%s is a block device", fname);
*filename = namebuf;
- fprintf(stderr, ", using %s\n", *filename);
+ warn_report("%s is a block device, using %s", fname, *filename);
return 0;
}
#else
-static int raw_normalize_devicepath(const char **filename)
+static int raw_normalize_devicepath(const char **filename, Error **errp)
{
return 0;
}
@@ -461,9 +459,8 @@ static int raw_open_common(BlockDriverState *bs, QDict *options,
filename = qemu_opt_get(opts, "filename");
- ret = raw_normalize_devicepath(&filename);
+ ret = raw_normalize_devicepath(&filename, errp);
if (ret != 0) {
- error_setg_errno(errp, -ret, "Could not normalize device path");
goto fail;
}
@@ -492,11 +489,10 @@ static int raw_open_common(BlockDriverState *bs, QDict *options,
case ON_OFF_AUTO_ON:
s->use_lock = true;
if (!qemu_has_ofd_lock()) {
- fprintf(stderr,
- "File lock requested but OFD locking syscall is "
- "unavailable, falling back to POSIX file locks.\n"
- "Due to the implementation, locks can be lost "
- "unexpectedly.\n");
+ warn_report("File lock requested but OFD locking syscall is "
+ "unavailable, falling back to POSIX file locks");
+ error_printf("Due to the implementation, locks can be lost "
+ "unexpectedly.\n");
}
break;
case ON_OFF_AUTO_OFF:
@@ -818,7 +814,7 @@ static int raw_handle_perm_lock(BlockDriverState *bs,
/* Theoretically the above call only unlocks bytes and it cannot
* fail. Something weird happened, report it.
*/
- error_report_err(local_err);
+ warn_report_err(local_err);
}
break;
case RAW_PL_COMMIT:
@@ -828,7 +824,7 @@ static int raw_handle_perm_lock(BlockDriverState *bs,
/* Theoretically the above call only unlocks bytes and it cannot
* fail. Something weird happened, report it.
*/
- error_report_err(local_err);
+ warn_report_err(local_err);
}
break;
}
@@ -905,10 +901,8 @@ static int raw_reopen_prepare(BDRVReopenState *state,
/* If we cannot use fcntl, or fcntl failed, fall back to qemu_open() */
if (rs->fd == -1) {
const char *normalized_filename = state->bs->filename;
- ret = raw_normalize_devicepath(&normalized_filename);
- if (ret < 0) {
- error_setg_errno(errp, -ret, "Could not normalize device path");
- } else {
+ ret = raw_normalize_devicepath(&normalized_filename, errp);
+ if (ret >= 0) {
assert(!(rs->open_flags & O_CREAT));
rs->fd = qemu_open(normalized_filename, rs->open_flags);
if (rs->fd == -1) {
@@ -1788,7 +1782,7 @@ static int aio_worker(void *arg)
ret = handle_aiocb_truncate(aiocb);
break;
default:
- fprintf(stderr, "invalid aio request (0x%x)\n", aiocb->aio_type);
+ error_report("invalid aio request (0x%x)", aiocb->aio_type);
ret = -EINVAL;
break;
}
@@ -2276,7 +2270,7 @@ out_unlock:
* not mean the whole creation operation has failed. So
* report it the user for their convenience, but do not report
* it to the caller. */
- error_report_err(local_err);
+ warn_report_err(local_err);
}
out_close:
@@ -3141,9 +3135,8 @@ static int coroutine_fn hdev_co_create_opts(const char *filename, QemuOpts *opts
(void)has_prefix;
- ret = raw_normalize_devicepath(&filename);
+ ret = raw_normalize_devicepath(&filename, errp);
if (ret < 0) {
- error_setg_errno(errp, -ret, "Could not normalize device path");
return ret;
}
--
2.19.1
^ permalink raw reply related [flat|nested] 16+ messages in thread* [Qemu-devel] [PULL 02/14] blockdev: handle error on block latency histogram set error
2018-11-12 17:05 [Qemu-devel] [PULL 00/14] Block layer patches Kevin Wolf
2018-11-12 17:05 ` [Qemu-devel] [PULL 01/14] file-posix: Use error API properly Kevin Wolf
@ 2018-11-12 17:05 ` Kevin Wolf
2018-11-12 17:05 ` [Qemu-devel] [PULL 03/14] blockdev: Consistently use snapshot_node_name in external_snapshot_prepare() Kevin Wolf
` (12 subsequent siblings)
14 siblings, 0 replies; 16+ messages in thread
From: Kevin Wolf @ 2018-11-12 17:05 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, peter.maydell, qemu-devel
From: zhenwei pi <pizhenwei@bytedance.com>
Function block_latency_histogram_set may return error, but qapi ignore this.
This can be reproduced easily by qmp command:
virsh qemu-monitor-command INSTANCE '{"execute":"x-block-latency-histogram-set",
"arguments":{"device":"drive-virtio-disk1","boundaries":[10,200,40]}}'
In fact this command does not work, but we still get success result.
qmp_x_block_latency_histogram_set is a batch setting API, report error ASAP.
Signed-off-by: zhenwei pi <pizhenwei@bytedance.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
blockdev.c | 19 ++++++++++++++++---
1 file changed, 16 insertions(+), 3 deletions(-)
diff --git a/blockdev.c b/blockdev.c
index e5b5eb46e2..9310ff3e7c 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -4413,6 +4413,7 @@ void qmp_x_block_latency_histogram_set(
{
BlockBackend *blk = blk_by_name(device);
BlockAcctStats *stats;
+ int ret;
if (!blk) {
error_setg(errp, "Device '%s' not found", device);
@@ -4428,21 +4429,33 @@ void qmp_x_block_latency_histogram_set(
}
if (has_boundaries || has_boundaries_read) {
- block_latency_histogram_set(
+ ret = block_latency_histogram_set(
stats, BLOCK_ACCT_READ,
has_boundaries_read ? boundaries_read : boundaries);
+ if (ret) {
+ error_setg(errp, "Device '%s' set read boundaries fail", device);
+ return;
+ }
}
if (has_boundaries || has_boundaries_write) {
- block_latency_histogram_set(
+ ret = block_latency_histogram_set(
stats, BLOCK_ACCT_WRITE,
has_boundaries_write ? boundaries_write : boundaries);
+ if (ret) {
+ error_setg(errp, "Device '%s' set write boundaries fail", device);
+ return;
+ }
}
if (has_boundaries || has_boundaries_flush) {
- block_latency_histogram_set(
+ ret = block_latency_histogram_set(
stats, BLOCK_ACCT_FLUSH,
has_boundaries_flush ? boundaries_flush : boundaries);
+ if (ret) {
+ error_setg(errp, "Device '%s' set flush boundaries fail", device);
+ return;
+ }
}
}
--
2.19.1
^ permalink raw reply related [flat|nested] 16+ messages in thread* [Qemu-devel] [PULL 03/14] blockdev: Consistently use snapshot_node_name in external_snapshot_prepare()
2018-11-12 17:05 [Qemu-devel] [PULL 00/14] Block layer patches Kevin Wolf
2018-11-12 17:05 ` [Qemu-devel] [PULL 01/14] file-posix: Use error API properly Kevin Wolf
2018-11-12 17:05 ` [Qemu-devel] [PULL 02/14] blockdev: handle error on block latency histogram set error Kevin Wolf
@ 2018-11-12 17:05 ` Kevin Wolf
2018-11-12 17:05 ` [Qemu-devel] [PULL 04/14] nvme: don't unref ctrl_mem when device unrealized Kevin Wolf
` (11 subsequent siblings)
14 siblings, 0 replies; 16+ messages in thread
From: Kevin Wolf @ 2018-11-12 17:05 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, peter.maydell, qemu-devel
From: Peter Maydell <peter.maydell@linaro.org>
In the function external_snapshot_prepare() we have a
BlockdevSnapshotSync struct, which has the usual combination
of has_snapshot_node_name and snapshot_node_name fields for an
optional field. We set up a local variable
const char *snapshot_node_name =
s->has_snapshot_node_name ? s->snapshot_node_name : NULL;
and then mostly use "if (!snapshot_node_name)" for checking
whether we have a snapshot node name. The exception is that in
one place we check s->has_snapshot_node_name instead. This
confuses Coverity (CID 1396473), which thinks it might be
possible to get here with s->has_snapshot_node_name true but
snapshot_node_name NULL, and warns that the call to
qdict_put_str() will segfault in that case.
Make the code consistent and unconfuse Coverity by using
the same check for this conditional that we do in the rest
of the surrounding code.
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Alberto Garcia <berto@igalia.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
blockdev.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/blockdev.c b/blockdev.c
index 9310ff3e7c..81f95d920b 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1640,7 +1640,7 @@ static void external_snapshot_prepare(BlkActionState *common,
}
options = qdict_new();
- if (s->has_snapshot_node_name) {
+ if (snapshot_node_name) {
qdict_put_str(options, "node-name", snapshot_node_name);
}
qdict_put_str(options, "driver", format);
--
2.19.1
^ permalink raw reply related [flat|nested] 16+ messages in thread* [Qemu-devel] [PULL 04/14] nvme: don't unref ctrl_mem when device unrealized
2018-11-12 17:05 [Qemu-devel] [PULL 00/14] Block layer patches Kevin Wolf
` (2 preceding siblings ...)
2018-11-12 17:05 ` [Qemu-devel] [PULL 03/14] blockdev: Consistently use snapshot_node_name in external_snapshot_prepare() Kevin Wolf
@ 2018-11-12 17:05 ` Kevin Wolf
2018-11-12 17:05 ` [Qemu-devel] [PULL 05/14] nvme: free cmbuf in nvme_exit Kevin Wolf
` (10 subsequent siblings)
14 siblings, 0 replies; 16+ messages in thread
From: Kevin Wolf @ 2018-11-12 17:05 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, peter.maydell, qemu-devel
From: Li Qiang <liq3ea@gmail.com>
Currently, when hotplug/unhotplug nvme device, it will cause an
assert in object.c. Following is the backtrack:
ERROR:qom/object.c:981:object_unref: assertion failed: (obj->ref > 0)
Thread 2 "qemu-system-x86" received signal SIGABRT, Aborted.
[Switching to Thread 0x7fffcbd32700 (LWP 18844)]
0x00007fffdb9e4fff in raise () from /lib/x86_64-linux-gnu/libc.so.6
(gdb) bt
/lib/x86_64-linux-gnu/libglib-2.0.so.0
/lib/x86_64-linux-gnu/libglib-2.0.so.0
qom/object.c:981
/home/liqiang02/qemu-upstream/qemu/memory.c:1732
/home/liqiang02/qemu-upstream/qemu/memory.c:285
util/qemu-thread-posix.c:504
/lib/x86_64-linux-gnu/libpthread.so.0
This is caused by memory_region_unref in nvme_exit.
Remove it to make the PCIdevice refcount correct.
Signed-off-by: Li Qiang <liq3ea@gmail.com>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
hw/block/nvme.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index fc7dacb816..359a06d0ad 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -1331,9 +1331,6 @@ static void nvme_exit(PCIDevice *pci_dev)
g_free(n->namespaces);
g_free(n->cq);
g_free(n->sq);
- if (n->cmbsz) {
- memory_region_unref(&n->ctrl_mem);
- }
msix_uninit_exclusive_bar(pci_dev);
}
--
2.19.1
^ permalink raw reply related [flat|nested] 16+ messages in thread* [Qemu-devel] [PULL 05/14] nvme: free cmbuf in nvme_exit
2018-11-12 17:05 [Qemu-devel] [PULL 00/14] Block layer patches Kevin Wolf
` (3 preceding siblings ...)
2018-11-12 17:05 ` [Qemu-devel] [PULL 04/14] nvme: don't unref ctrl_mem when device unrealized Kevin Wolf
@ 2018-11-12 17:05 ` Kevin Wolf
2018-11-12 17:05 ` [Qemu-devel] [PULL 06/14] file-posix: Skip effectiveless OFD lock operations Kevin Wolf
` (9 subsequent siblings)
14 siblings, 0 replies; 16+ messages in thread
From: Kevin Wolf @ 2018-11-12 17:05 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, peter.maydell, qemu-devel
From: Li Qiang <liq3ea@gmail.com>
This avoid a memory leak in unhotplug nvme device.
Signed-off-by: Li Qiang <liq3ea@gmail.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
hw/block/nvme.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 359a06d0ad..09d7c90259 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -1332,6 +1332,9 @@ static void nvme_exit(PCIDevice *pci_dev)
g_free(n->cq);
g_free(n->sq);
+ if (n->cmb_size_mb) {
+ g_free(n->cmbuf);
+ }
msix_uninit_exclusive_bar(pci_dev);
}
--
2.19.1
^ permalink raw reply related [flat|nested] 16+ messages in thread* [Qemu-devel] [PULL 06/14] file-posix: Skip effectiveless OFD lock operations
2018-11-12 17:05 [Qemu-devel] [PULL 00/14] Block layer patches Kevin Wolf
` (4 preceding siblings ...)
2018-11-12 17:05 ` [Qemu-devel] [PULL 05/14] nvme: free cmbuf in nvme_exit Kevin Wolf
@ 2018-11-12 17:05 ` Kevin Wolf
2018-11-12 17:05 ` [Qemu-devel] [PULL 07/14] file-posix: Drop s->lock_fd Kevin Wolf
` (8 subsequent siblings)
14 siblings, 0 replies; 16+ messages in thread
From: Kevin Wolf @ 2018-11-12 17:05 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, peter.maydell, qemu-devel
From: Fam Zheng <famz@redhat.com>
If we know we've already locked the bytes, don't do it again; similarly
don't unlock a byte if we haven't locked it. This doesn't change the
behavior, but fixes a corner case explained below.
Libvirt had an error handling bug that an image can get its (ownership,
file mode, SELinux) permissions changed (RHBZ 1584982) by mistake behind
QEMU. Specifically, an image in use by Libvirt VM has:
$ ls -lhZ b.img
-rw-r--r--. qemu qemu system_u:object_r:svirt_image_t:s0:c600,c690 b.img
Trying to attach it a second time won't work because of image locking.
And after the error, it becomes:
$ ls -lhZ b.img
-rw-r--r--. root root system_u:object_r:virt_image_t:s0 b.img
Then, we won't be able to do OFD lock operations with the existing fd.
In other words, the code such as in blk_detach_dev:
blk_set_perm(blk, 0, BLK_PERM_ALL, &error_abort);
can abort() QEMU, out of environmental changes.
This patch is an easy fix to this and the change is regardlessly
reasonable, so do it.
Signed-off-by: Fam Zheng <famz@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
block/file-posix.c | 54 +++++++++++++++++++++++++++++++++++++---------
1 file changed, 44 insertions(+), 10 deletions(-)
diff --git a/block/file-posix.c b/block/file-posix.c
index 074f0ce2b3..1b4ff8de00 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -152,6 +152,11 @@ typedef struct BDRVRawState {
uint64_t perm;
uint64_t shared_perm;
+ /* The perms bits whose corresponding bytes are already locked in
+ * s->lock_fd. */
+ uint64_t locked_perm;
+ uint64_t locked_shared_perm;
+
#ifdef CONFIG_XFS
bool is_xfs:1;
#endif
@@ -689,43 +694,72 @@ typedef enum {
* file; if @unlock == true, also unlock the unneeded bytes.
* @shared_perm_lock_bits is the mask of all permissions that are NOT shared.
*/
-static int raw_apply_lock_bytes(int fd,
+static int raw_apply_lock_bytes(BDRVRawState *s, int fd,
uint64_t perm_lock_bits,
uint64_t shared_perm_lock_bits,
bool unlock, Error **errp)
{
int ret;
int i;
+ uint64_t locked_perm, locked_shared_perm;
+
+ if (s) {
+ locked_perm = s->locked_perm;
+ locked_shared_perm = s->locked_shared_perm;
+ } else {
+ /*
+ * We don't have the previous bits, just lock/unlock for each of the
+ * requested bits.
+ */
+ if (unlock) {
+ locked_perm = BLK_PERM_ALL;
+ locked_shared_perm = BLK_PERM_ALL;
+ } else {
+ locked_perm = 0;
+ locked_shared_perm = 0;
+ }
+ }
PERM_FOREACH(i) {
int off = RAW_LOCK_PERM_BASE + i;
- if (perm_lock_bits & (1ULL << i)) {
+ uint64_t bit = (1ULL << i);
+ if ((perm_lock_bits & bit) && !(locked_perm & bit)) {
ret = qemu_lock_fd(fd, off, 1, false);
if (ret) {
error_setg(errp, "Failed to lock byte %d", off);
return ret;
+ } else if (s) {
+ s->locked_perm |= bit;
}
- } else if (unlock) {
+ } else if (unlock && (locked_perm & bit) && !(perm_lock_bits & bit)) {
ret = qemu_unlock_fd(fd, off, 1);
if (ret) {
error_setg(errp, "Failed to unlock byte %d", off);
return ret;
+ } else if (s) {
+ s->locked_perm &= ~bit;
}
}
}
PERM_FOREACH(i) {
int off = RAW_LOCK_SHARED_BASE + i;
- if (shared_perm_lock_bits & (1ULL << i)) {
+ uint64_t bit = (1ULL << i);
+ if ((shared_perm_lock_bits & bit) && !(locked_shared_perm & bit)) {
ret = qemu_lock_fd(fd, off, 1, false);
if (ret) {
error_setg(errp, "Failed to lock byte %d", off);
return ret;
+ } else if (s) {
+ s->locked_shared_perm |= bit;
}
- } else if (unlock) {
+ } else if (unlock && (locked_shared_perm & bit) &&
+ !(shared_perm_lock_bits & bit)) {
ret = qemu_unlock_fd(fd, off, 1);
if (ret) {
error_setg(errp, "Failed to unlock byte %d", off);
return ret;
+ } else if (s) {
+ s->locked_shared_perm &= ~bit;
}
}
}
@@ -793,7 +827,7 @@ static int raw_handle_perm_lock(BlockDriverState *bs,
switch (op) {
case RAW_PL_PREPARE:
- ret = raw_apply_lock_bytes(s->lock_fd, s->perm | new_perm,
+ ret = raw_apply_lock_bytes(s, s->lock_fd, s->perm | new_perm,
~s->shared_perm | ~new_shared,
false, errp);
if (!ret) {
@@ -808,7 +842,7 @@ static int raw_handle_perm_lock(BlockDriverState *bs,
op = RAW_PL_ABORT;
/* fall through to unlock bytes. */
case RAW_PL_ABORT:
- raw_apply_lock_bytes(s->lock_fd, s->perm, ~s->shared_perm,
+ raw_apply_lock_bytes(s, s->lock_fd, s->perm, ~s->shared_perm,
true, &local_err);
if (local_err) {
/* Theoretically the above call only unlocks bytes and it cannot
@@ -818,7 +852,7 @@ static int raw_handle_perm_lock(BlockDriverState *bs,
}
break;
case RAW_PL_COMMIT:
- raw_apply_lock_bytes(s->lock_fd, new_perm, ~new_shared,
+ raw_apply_lock_bytes(s, s->lock_fd, new_perm, ~new_shared,
true, &local_err);
if (local_err) {
/* Theoretically the above call only unlocks bytes and it cannot
@@ -2220,7 +2254,7 @@ raw_co_create(BlockdevCreateOptions *options, Error **errp)
shared = BLK_PERM_ALL & ~BLK_PERM_RESIZE;
/* Step one: Take locks */
- result = raw_apply_lock_bytes(fd, perm, ~shared, false, errp);
+ result = raw_apply_lock_bytes(NULL, fd, perm, ~shared, false, errp);
if (result < 0) {
goto out_close;
}
@@ -2264,7 +2298,7 @@ raw_co_create(BlockdevCreateOptions *options, Error **errp)
}
out_unlock:
- raw_apply_lock_bytes(fd, 0, 0, true, &local_err);
+ raw_apply_lock_bytes(NULL, fd, 0, 0, true, &local_err);
if (local_err) {
/* The above call should not fail, and if it does, that does
* not mean the whole creation operation has failed. So
--
2.19.1
^ permalink raw reply related [flat|nested] 16+ messages in thread* [Qemu-devel] [PULL 07/14] file-posix: Drop s->lock_fd
2018-11-12 17:05 [Qemu-devel] [PULL 00/14] Block layer patches Kevin Wolf
` (5 preceding siblings ...)
2018-11-12 17:05 ` [Qemu-devel] [PULL 06/14] file-posix: Skip effectiveless OFD lock operations Kevin Wolf
@ 2018-11-12 17:05 ` Kevin Wolf
2018-11-12 17:05 ` [Qemu-devel] [PULL 08/14] tests: Add unit tests for image locking Kevin Wolf
` (7 subsequent siblings)
14 siblings, 0 replies; 16+ messages in thread
From: Kevin Wolf @ 2018-11-12 17:05 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, peter.maydell, qemu-devel
From: Fam Zheng <famz@redhat.com>
The lock_fd field is not strictly necessary because transferring locked
bytes from old fd to the new one shouldn't fail anyway. This spares the
user one fd per image.
Signed-off-by: Fam Zheng <famz@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
block/file-posix.c | 37 +++++++++++++------------------------
1 file changed, 13 insertions(+), 24 deletions(-)
diff --git a/block/file-posix.c b/block/file-posix.c
index 1b4ff8de00..58c86a01ea 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -142,7 +142,6 @@ do { \
typedef struct BDRVRawState {
int fd;
- int lock_fd;
bool use_lock;
int type;
int open_flags;
@@ -153,7 +152,7 @@ typedef struct BDRVRawState {
uint64_t shared_perm;
/* The perms bits whose corresponding bytes are already locked in
- * s->lock_fd. */
+ * s->fd. */
uint64_t locked_perm;
uint64_t locked_shared_perm;
@@ -551,18 +550,6 @@ static int raw_open_common(BlockDriverState *bs, QDict *options,
}
s->fd = fd;
- s->lock_fd = -1;
- if (s->use_lock) {
- fd = qemu_open(filename, s->open_flags);
- if (fd < 0) {
- ret = -errno;
- error_setg_errno(errp, errno, "Could not open '%s' for locking",
- filename);
- qemu_close(s->fd);
- goto fail;
- }
- s->lock_fd = fd;
- }
s->perm = 0;
s->shared_perm = BLK_PERM_ALL;
@@ -823,15 +810,13 @@ static int raw_handle_perm_lock(BlockDriverState *bs,
return 0;
}
- assert(s->lock_fd > 0);
-
switch (op) {
case RAW_PL_PREPARE:
- ret = raw_apply_lock_bytes(s, s->lock_fd, s->perm | new_perm,
+ ret = raw_apply_lock_bytes(s, s->fd, s->perm | new_perm,
~s->shared_perm | ~new_shared,
false, errp);
if (!ret) {
- ret = raw_check_lock_bytes(s->lock_fd, new_perm, new_shared, errp);
+ ret = raw_check_lock_bytes(s->fd, new_perm, new_shared, errp);
if (!ret) {
return 0;
}
@@ -842,7 +827,7 @@ static int raw_handle_perm_lock(BlockDriverState *bs,
op = RAW_PL_ABORT;
/* fall through to unlock bytes. */
case RAW_PL_ABORT:
- raw_apply_lock_bytes(s, s->lock_fd, s->perm, ~s->shared_perm,
+ raw_apply_lock_bytes(s, s->fd, s->perm, ~s->shared_perm,
true, &local_err);
if (local_err) {
/* Theoretically the above call only unlocks bytes and it cannot
@@ -852,7 +837,7 @@ static int raw_handle_perm_lock(BlockDriverState *bs,
}
break;
case RAW_PL_COMMIT:
- raw_apply_lock_bytes(s, s->lock_fd, new_perm, ~new_shared,
+ raw_apply_lock_bytes(s, s->fd, new_perm, ~new_shared,
true, &local_err);
if (local_err) {
/* Theoretically the above call only unlocks bytes and it cannot
@@ -967,10 +952,18 @@ static void raw_reopen_commit(BDRVReopenState *state)
{
BDRVRawReopenState *rs = state->opaque;
BDRVRawState *s = state->bs->opaque;
+ Error *local_err = NULL;
s->check_cache_dropped = rs->check_cache_dropped;
s->open_flags = rs->open_flags;
+ /* Copy locks to the new fd before closing the old one. */
+ raw_apply_lock_bytes(NULL, rs->fd, s->locked_perm,
+ ~s->locked_shared_perm, false, &local_err);
+ if (local_err) {
+ /* shouldn't fail in a sane host, but report it just in case. */
+ error_report_err(local_err);
+ }
qemu_close(s->fd);
s->fd = rs->fd;
@@ -1963,10 +1956,6 @@ static void raw_close(BlockDriverState *bs)
qemu_close(s->fd);
s->fd = -1;
}
- if (s->lock_fd >= 0) {
- qemu_close(s->lock_fd);
- s->lock_fd = -1;
- }
}
/**
--
2.19.1
^ permalink raw reply related [flat|nested] 16+ messages in thread* [Qemu-devel] [PULL 08/14] tests: Add unit tests for image locking
2018-11-12 17:05 [Qemu-devel] [PULL 00/14] Block layer patches Kevin Wolf
` (6 preceding siblings ...)
2018-11-12 17:05 ` [Qemu-devel] [PULL 07/14] file-posix: Drop s->lock_fd Kevin Wolf
@ 2018-11-12 17:05 ` Kevin Wolf
2018-11-12 17:05 ` [Qemu-devel] [PULL 09/14] block: Make more block drivers compile-time configurable Kevin Wolf
` (6 subsequent siblings)
14 siblings, 0 replies; 16+ messages in thread
From: Kevin Wolf @ 2018-11-12 17:05 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, peter.maydell, qemu-devel
From: Fam Zheng <famz@redhat.com>
Signed-off-by: Fam Zheng <famz@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
tests/test-image-locking.c | 157 +++++++++++++++++++++++++++++++++++++
tests/Makefile.include | 2 +
2 files changed, 159 insertions(+)
create mode 100644 tests/test-image-locking.c
diff --git a/tests/test-image-locking.c b/tests/test-image-locking.c
new file mode 100644
index 0000000000..7614cbf90c
--- /dev/null
+++ b/tests/test-image-locking.c
@@ -0,0 +1,157 @@
+/*
+ * Image locking tests
+ *
+ * Copyright (c) 2018 Red Hat Inc.
+ *
+ * Author: Fam Zheng <famz@redhat.com>
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#include "qemu/osdep.h"
+#include "block/block.h"
+#include "sysemu/block-backend.h"
+#include "qapi/error.h"
+#include "qapi/qmp/qdict.h"
+
+static BlockBackend *open_image(const char *path,
+ uint64_t perm, uint64_t shared_perm,
+ Error **errp)
+{
+ Error *local_err = NULL;
+ BlockBackend *blk;
+ QDict *options = qdict_new();
+
+ qdict_put_str(options, "driver", "raw");
+ blk = blk_new_open(path, NULL, options, BDRV_O_RDWR, &local_err);
+ if (blk) {
+ g_assert_null(local_err);
+ if (blk_set_perm(blk, perm, shared_perm, errp)) {
+ blk_unref(blk);
+ blk = NULL;
+ }
+ } else {
+ error_propagate(errp, local_err);
+ }
+ return blk;
+}
+
+static void check_locked_bytes(int fd, uint64_t perm_locks,
+ uint64_t shared_perm_locks)
+{
+ int i;
+
+ if (!perm_locks && !shared_perm_locks) {
+ g_assert(!qemu_lock_fd_test(fd, 0, 0, true));
+ return;
+ }
+ for (i = 0; (1ULL << i) <= BLK_PERM_ALL; i++) {
+ uint64_t bit = (1ULL << i);
+ bool perm_expected = !!(bit & perm_locks);
+ bool shared_perm_expected = !!(bit & shared_perm_locks);
+ g_assert_cmpint(perm_expected, ==,
+ !!qemu_lock_fd_test(fd, 100 + i, 1, true));
+ g_assert_cmpint(shared_perm_expected, ==,
+ !!qemu_lock_fd_test(fd, 200 + i, 1, true));
+ }
+}
+
+static void test_image_locking_basic(void)
+{
+ BlockBackend *blk1, *blk2, *blk3;
+ char img_path[] = "/tmp/qtest.XXXXXX";
+ uint64_t perm, shared_perm;
+
+ int fd = mkstemp(img_path);
+ assert(fd >= 0);
+
+ perm = BLK_PERM_WRITE | BLK_PERM_CONSISTENT_READ;
+ shared_perm = BLK_PERM_ALL;
+ blk1 = open_image(img_path, perm, shared_perm, &error_abort);
+ g_assert(blk1);
+
+ check_locked_bytes(fd, perm, ~shared_perm);
+
+ /* compatible perm between blk1 and blk2 */
+ blk2 = open_image(img_path, perm | BLK_PERM_RESIZE, shared_perm, NULL);
+ g_assert(blk2);
+ check_locked_bytes(fd, perm | BLK_PERM_RESIZE, ~shared_perm);
+
+ /* incompatible perm with already open blk1 and blk2 */
+ blk3 = open_image(img_path, perm, BLK_PERM_WRITE_UNCHANGED, NULL);
+ g_assert_null(blk3);
+
+ blk_unref(blk2);
+
+ /* Check that extra bytes in blk2 are correctly unlocked */
+ check_locked_bytes(fd, perm, ~shared_perm);
+
+ blk_unref(blk1);
+
+ /* Image is unused, no lock there */
+ check_locked_bytes(fd, 0, 0);
+ blk3 = open_image(img_path, perm, BLK_PERM_WRITE_UNCHANGED, &error_abort);
+ g_assert(blk3);
+ blk_unref(blk3);
+ close(fd);
+ unlink(img_path);
+}
+
+static void test_set_perm_abort(void)
+{
+ BlockBackend *blk1, *blk2;
+ char img_path[] = "/tmp/qtest.XXXXXX";
+ uint64_t perm, shared_perm;
+ int r;
+ int fd = mkstemp(img_path);
+ assert(fd >= 0);
+
+ perm = BLK_PERM_WRITE | BLK_PERM_CONSISTENT_READ;
+ shared_perm = BLK_PERM_ALL;
+ blk1 = open_image(img_path, perm, shared_perm, &error_abort);
+ g_assert(blk1);
+
+ blk2 = open_image(img_path, perm, shared_perm, &error_abort);
+ g_assert(blk2);
+
+ check_locked_bytes(fd, perm, ~shared_perm);
+
+ /* A failed blk_set_perm mustn't change perm status (locked bytes) */
+ r = blk_set_perm(blk2, perm | BLK_PERM_RESIZE, BLK_PERM_WRITE_UNCHANGED,
+ NULL);
+ g_assert_cmpint(r, !=, 0);
+ check_locked_bytes(fd, perm, ~shared_perm);
+ blk_unref(blk1);
+ blk_unref(blk2);
+}
+
+int main(int argc, char **argv)
+{
+ bdrv_init();
+ qemu_init_main_loop(&error_abort);
+
+ g_test_init(&argc, &argv, NULL);
+
+ if (qemu_has_ofd_lock()) {
+ g_test_add_func("/image-locking/basic", test_image_locking_basic);
+ g_test_add_func("/image-locking/set-perm-abort", test_set_perm_abort);
+ }
+
+ return g_test_run();
+}
diff --git a/tests/Makefile.include b/tests/Makefile.include
index 074eece558..613242bc6e 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -70,6 +70,7 @@ check-unit-y += tests/test-bdrv-drain$(EXESUF)
check-unit-y += tests/test-blockjob$(EXESUF)
check-unit-y += tests/test-blockjob-txn$(EXESUF)
check-unit-y += tests/test-block-backend$(EXESUF)
+check-unit-y += tests/test-image-locking$(EXESUF)
check-unit-y += tests/test-x86-cpuid$(EXESUF)
# all code tested by test-x86-cpuid is inside topology.h
ifeq ($(CONFIG_SOFTMMU),y)
@@ -537,6 +538,7 @@ tests/test-bdrv-drain$(EXESUF): tests/test-bdrv-drain.o $(test-block-obj-y) $(te
tests/test-blockjob$(EXESUF): tests/test-blockjob.o $(test-block-obj-y) $(test-util-obj-y)
tests/test-blockjob-txn$(EXESUF): tests/test-blockjob-txn.o $(test-block-obj-y) $(test-util-obj-y)
tests/test-block-backend$(EXESUF): tests/test-block-backend.o $(test-block-obj-y) $(test-util-obj-y)
+tests/test-image-locking$(EXESUF): tests/test-image-locking.o $(test-block-obj-y) $(test-util-obj-y)
tests/test-thread-pool$(EXESUF): tests/test-thread-pool.o $(test-block-obj-y)
tests/test-iov$(EXESUF): tests/test-iov.o $(test-util-obj-y)
tests/test-hbitmap$(EXESUF): tests/test-hbitmap.o $(test-util-obj-y) $(test-crypto-obj-y)
--
2.19.1
^ permalink raw reply related [flat|nested] 16+ messages in thread* [Qemu-devel] [PULL 09/14] block: Make more block drivers compile-time configurable
2018-11-12 17:05 [Qemu-devel] [PULL 00/14] Block layer patches Kevin Wolf
` (7 preceding siblings ...)
2018-11-12 17:05 ` [Qemu-devel] [PULL 08/14] tests: Add unit tests for image locking Kevin Wolf
@ 2018-11-12 17:05 ` Kevin Wolf
2018-11-12 17:05 ` [Qemu-devel] [PULL 10/14] job: Fix off-by-one assert checks for JobSTT and JobVerbTable Kevin Wolf
` (5 subsequent siblings)
14 siblings, 0 replies; 16+ messages in thread
From: Kevin Wolf @ 2018-11-12 17:05 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, peter.maydell, qemu-devel
From: Jeff Cody <jcody@redhat.com>
This adds configure options to control the following block drivers:
* Bochs
* Cloop
* Dmg
* Qcow (V1)
* Vdi
* Vvfat
* qed
* parallels
* sheepdog
Each of these defaults to being enabled.
Signed-off-by: Jeff Cody <jcody@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Message-id: 20181107063644.2254-1-armbru@redhat.com
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
configure | 91 +++++++++++++++++++++++++++++++++++++++++++++
block/Makefile.objs | 22 ++++++++---
2 files changed, 107 insertions(+), 6 deletions(-)
diff --git a/configure b/configure
index 74e313a810..5b1d83ea26 100755
--- a/configure
+++ b/configure
@@ -470,6 +470,15 @@ tcmalloc="no"
jemalloc="no"
replication="yes"
vxhs=""
+bochs="yes"
+cloop="yes"
+dmg="yes"
+qcow1="yes"
+vdi="yes"
+vvfat="yes"
+qed="yes"
+parallels="yes"
+sheepdog="yes"
libxml2=""
docker="no"
debug_mutex="no"
@@ -1416,6 +1425,42 @@ for opt do
;;
--enable-vxhs) vxhs="yes"
;;
+ --disable-bochs) bochs="no"
+ ;;
+ --enable-bochs) bochs="yes"
+ ;;
+ --disable-cloop) cloop="no"
+ ;;
+ --enable-cloop) cloop="yes"
+ ;;
+ --disable-dmg) dmg="no"
+ ;;
+ --enable-dmg) dmg="yes"
+ ;;
+ --disable-qcow1) qcow1="no"
+ ;;
+ --enable-qcow1) qcow1="yes"
+ ;;
+ --disable-vdi) vdi="no"
+ ;;
+ --enable-vdi) vdi="yes"
+ ;;
+ --disable-vvfat) vvfat="no"
+ ;;
+ --enable-vvfat) vvfat="yes"
+ ;;
+ --disable-qed) qed="no"
+ ;;
+ --enable-qed) qed="yes"
+ ;;
+ --disable-parallels) parallels="no"
+ ;;
+ --enable-parallels) parallels="yes"
+ ;;
+ --disable-sheepdog) sheepdog="no"
+ ;;
+ --enable-sheepdog) sheepdog="yes"
+ ;;
--disable-vhost-user) vhost_user="no"
;;
--enable-vhost-user)
@@ -1718,6 +1763,15 @@ disabled with --disable-FEATURE, default is enabled if available:
qom-cast-debug cast debugging support
tools build qemu-io, qemu-nbd and qemu-image tools
vxhs Veritas HyperScale vDisk backend support
+ bochs bochs image format support
+ cloop cloop image format support
+ dmg dmg image format support
+ qcow1 qcow v1 image format support
+ vdi vdi image format support
+ vvfat vvfat image format support
+ qed qed image format support
+ parallels parallels image format support
+ sheepdog sheepdog block driver support
crypto-afalg Linux AF_ALG crypto backend driver
vhost-user vhost-user support
capstone capstone disassembler support
@@ -6043,6 +6097,15 @@ echo "jemalloc support $jemalloc"
echo "avx2 optimization $avx2_opt"
echo "replication support $replication"
echo "VxHS block device $vxhs"
+echo "bochs support $bochs"
+echo "cloop support $cloop"
+echo "dmg support $dmg"
+echo "qcow v1 support $qcow1"
+echo "vdi support $vdi"
+echo "vvfat support $vvfat"
+echo "qed support $qed"
+echo "parallels support $parallels"
+echo "sheepdog support $sheepdog"
echo "capstone $capstone"
echo "docker $docker"
echo "libpmem support $libpmem"
@@ -6799,6 +6862,34 @@ if test "$libpmem" = "yes" ; then
echo "CONFIG_LIBPMEM=y" >> $config_host_mak
fi
+if test "$bochs" = "yes" ; then
+ echo "CONFIG_BOCHS=y" >> $config_host_mak
+fi
+if test "$cloop" = "yes" ; then
+ echo "CONFIG_CLOOP=y" >> $config_host_mak
+fi
+if test "$dmg" = "yes" ; then
+ echo "CONFIG_DMG=y" >> $config_host_mak
+fi
+if test "$qcow1" = "yes" ; then
+ echo "CONFIG_QCOW1=y" >> $config_host_mak
+fi
+if test "$vdi" = "yes" ; then
+ echo "CONFIG_VDI=y" >> $config_host_mak
+fi
+if test "$vvfat" = "yes" ; then
+ echo "CONFIG_VVFAT=y" >> $config_host_mak
+fi
+if test "$qed" = "yes" ; then
+ echo "CONFIG_QED=y" >> $config_host_mak
+fi
+if test "$parallels" = "yes" ; then
+ echo "CONFIG_PARALLELS=y" >> $config_host_mak
+fi
+if test "$sheepdog" = "yes" ; then
+ echo "CONFIG_SHEEPDOG=y" >> $config_host_mak
+fi
+
if test "$tcg_interpreter" = "yes"; then
QEMU_INCLUDES="-iquote \$(SRC_PATH)/tcg/tci $QEMU_INCLUDES"
elif test "$ARCH" = "sparc64" ; then
diff --git a/block/Makefile.objs b/block/Makefile.objs
index c8337bf186..46d585cfd0 100644
--- a/block/Makefile.objs
+++ b/block/Makefile.objs
@@ -1,10 +1,18 @@
-block-obj-y += raw-format.o qcow.o vdi.o vmdk.o cloop.o bochs.o vpc.o vvfat.o dmg.o
+block-obj-y += raw-format.o vmdk.o vpc.o
+block-obj-$(CONFIG_QCOW1) += qcow.o
+block-obj-$(CONFIG_VDI) += vdi.o
+block-obj-$(CONFIG_CLOOP) += cloop.o
+block-obj-$(CONFIG_BOCHS) += bochs.o
+block-obj-$(CONFIG_VVFAT) += vvfat.o
+block-obj-$(CONFIG_DMG) += dmg.o
+
block-obj-y += qcow2.o qcow2-refcount.o qcow2-cluster.o qcow2-snapshot.o qcow2-cache.o qcow2-bitmap.o
-block-obj-y += qed.o qed-l2-cache.o qed-table.o qed-cluster.o
-block-obj-y += qed-check.o
+block-obj-$(CONFIG_QED) += qed.o qed-l2-cache.o qed-table.o qed-cluster.o
+block-obj-$(CONFIG_QED) += qed-check.o
block-obj-y += vhdx.o vhdx-endian.o vhdx-log.o
block-obj-y += quorum.o
-block-obj-y += parallels.o blkdebug.o blkverify.o blkreplay.o
+block-obj-y += blkdebug.o blkverify.o blkreplay.o
+block-obj-$(CONFIG_PARALLELS) += parallels.o
block-obj-y += blklogwrites.o
block-obj-y += block-backend.o snapshot.o qapi.o
block-obj-$(CONFIG_WIN32) += file-win32.o win32-aio.o
@@ -14,7 +22,8 @@ block-obj-y += null.o mirror.o commit.o io.o create.o
block-obj-y += throttle-groups.o
block-obj-$(CONFIG_LINUX) += nvme.o
-block-obj-y += nbd.o nbd-client.o sheepdog.o
+block-obj-y += nbd.o nbd-client.o
+block-obj-$(CONFIG_SHEEPDOG) += sheepdog.o
block-obj-$(CONFIG_LIBISCSI) += iscsi.o
block-obj-$(if $(CONFIG_LIBISCSI),y,n) += iscsi-opts.o
block-obj-$(CONFIG_LIBNFS) += nfs.o
@@ -45,7 +54,8 @@ gluster.o-libs := $(GLUSTERFS_LIBS)
vxhs.o-libs := $(VXHS_LIBS)
ssh.o-cflags := $(LIBSSH2_CFLAGS)
ssh.o-libs := $(LIBSSH2_LIBS)
-block-obj-$(if $(CONFIG_BZIP2),m,n) += dmg-bz2.o
+block-obj-dmg-bz2-$(CONFIG_BZIP2) += dmg-bz2.o
+block-obj-$(if $(CONFIG_DMG),m,n) += $(block-obj-dmg-bz2-y)
dmg-bz2.o-libs := $(BZIP2_LIBS)
qcow.o-libs := -lz
linux-aio.o-libs := -laio
--
2.19.1
^ permalink raw reply related [flat|nested] 16+ messages in thread* [Qemu-devel] [PULL 10/14] job: Fix off-by-one assert checks for JobSTT and JobVerbTable
2018-11-12 17:05 [Qemu-devel] [PULL 00/14] Block layer patches Kevin Wolf
` (8 preceding siblings ...)
2018-11-12 17:05 ` [Qemu-devel] [PULL 09/14] block: Make more block drivers compile-time configurable Kevin Wolf
@ 2018-11-12 17:05 ` Kevin Wolf
2018-11-12 17:06 ` [Qemu-devel] [PULL 11/14] block: Null pointer dereference in blk_root_get_parent_desc() Kevin Wolf
` (4 subsequent siblings)
14 siblings, 0 replies; 16+ messages in thread
From: Kevin Wolf @ 2018-11-12 17:05 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, peter.maydell, qemu-devel
From: Liam Merwick <Liam.Merwick@oracle.com>
In the assert checking the array dereference of JobVerbTable[verb]
in job_apply_verb() the check of the index, verb, allows an overrun
because an index equal to the array size is permitted.
Similarly, in the assert check of JobSTT[s0][s1] with index s1
in job_state_transition(), an off-by-one overrun is not flagged
either.
This is not a run-time issue as there are no callers actually
passing in the max value.
Signed-off-by: Liam Merwick <Liam.Merwick@oracle.com>
Reviewed-by: Darren Kenny <Darren.Kenny@oracle.com>
Reviewed-by: Mark Kanda <Mark.Kanda@oracle.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: John Snow <jsnow@redhat.com>
Message-id: 1541453919-25973-2-git-send-email-Liam.Merwick@oracle.com
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
job.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/job.c b/job.c
index c65e01bbfa..da8e4b7bf2 100644
--- a/job.c
+++ b/job.c
@@ -159,7 +159,7 @@ bool job_is_internal(Job *job)
static void job_state_transition(Job *job, JobStatus s1)
{
JobStatus s0 = job->status;
- assert(s1 >= 0 && s1 <= JOB_STATUS__MAX);
+ assert(s1 >= 0 && s1 < JOB_STATUS__MAX);
trace_job_state_transition(job, job->ret,
JobSTT[s0][s1] ? "allowed" : "disallowed",
JobStatus_str(s0), JobStatus_str(s1));
@@ -174,7 +174,7 @@ static void job_state_transition(Job *job, JobStatus s1)
int job_apply_verb(Job *job, JobVerb verb, Error **errp)
{
JobStatus s0 = job->status;
- assert(verb >= 0 && verb <= JOB_VERB__MAX);
+ assert(verb >= 0 && verb < JOB_VERB__MAX);
trace_job_apply_verb(job, JobStatus_str(s0), JobVerb_str(verb),
JobVerbTable[verb][s0] ? "allowed" : "prohibited");
if (JobVerbTable[verb][s0]) {
--
2.19.1
^ permalink raw reply related [flat|nested] 16+ messages in thread* [Qemu-devel] [PULL 11/14] block: Null pointer dereference in blk_root_get_parent_desc()
2018-11-12 17:05 [Qemu-devel] [PULL 00/14] Block layer patches Kevin Wolf
` (9 preceding siblings ...)
2018-11-12 17:05 ` [Qemu-devel] [PULL 10/14] job: Fix off-by-one assert checks for JobSTT and JobVerbTable Kevin Wolf
@ 2018-11-12 17:06 ` Kevin Wolf
2018-11-12 17:06 ` [Qemu-devel] [PULL 12/14] qemu-img: assert block_job_get() does not return NULL in img_commit() Kevin Wolf
` (3 subsequent siblings)
14 siblings, 0 replies; 16+ messages in thread
From: Kevin Wolf @ 2018-11-12 17:06 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, peter.maydell, qemu-devel
From: Liam Merwick <Liam.Merwick@oracle.com>
The dev_id returned by the call to blk_get_attached_dev_id() in
blk_root_get_parent_desc() can be NULL (an internal call to
object_get_canonical_path may have returned NULL).
Instead of just checking this case before before dereferencing,
adjust blk_get_attached_dev_id() to return the empty string if no
object path can be found (similar to the case when blk->dev is NULL
and an empty string is returned).
Signed-off-by: Liam Merwick <Liam.Merwick@oracle.com>
Message-id: 1541453919-25973-3-git-send-email-Liam.Merwick@oracle.com
Reviewed-by: Max Reitz <mreitz@redhat.com>
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
block/block-backend.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/block/block-backend.c b/block/block-backend.c
index 2a8f3b55f8..60d37a0c3d 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -918,7 +918,8 @@ char *blk_get_attached_dev_id(BlockBackend *blk)
} else if (dev->id) {
return g_strdup(dev->id);
}
- return object_get_canonical_path(OBJECT(dev));
+
+ return object_get_canonical_path(OBJECT(dev)) ?: g_strdup("");
}
/*
--
2.19.1
^ permalink raw reply related [flat|nested] 16+ messages in thread* [Qemu-devel] [PULL 12/14] qemu-img: assert block_job_get() does not return NULL in img_commit()
2018-11-12 17:05 [Qemu-devel] [PULL 00/14] Block layer patches Kevin Wolf
` (10 preceding siblings ...)
2018-11-12 17:06 ` [Qemu-devel] [PULL 11/14] block: Null pointer dereference in blk_root_get_parent_desc() Kevin Wolf
@ 2018-11-12 17:06 ` Kevin Wolf
2018-11-12 17:06 ` [Qemu-devel] [PULL 13/14] block: Fix potential Null pointer dereferences in vvfat.c Kevin Wolf
` (2 subsequent siblings)
14 siblings, 0 replies; 16+ messages in thread
From: Kevin Wolf @ 2018-11-12 17:06 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, peter.maydell, qemu-devel
From: Liam Merwick <Liam.Merwick@oracle.com>
Although the function block_job_get() can return NULL, it would be a
serious bug if it did so (because the job yields before executing anything
(if it started successfully); but otherwise, commit_active_start() would
have returned an error). However, as a precaution, before dereferencing
the 'job' pointer in img_commit() assert it is not NULL.
Signed-off-by: Liam Merwick <Liam.Merwick@oracle.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
Message-id: 1541453919-25973-4-git-send-email-Liam.Merwick@oracle.com
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
qemu-img.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/qemu-img.c b/qemu-img.c
index 4c96db7ba4..13a6ca31b4 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1029,6 +1029,7 @@ static int img_commit(int argc, char **argv)
}
job = block_job_get("commit");
+ assert(job);
run_block_job(job, &local_err);
if (local_err) {
goto unref_backing;
--
2.19.1
^ permalink raw reply related [flat|nested] 16+ messages in thread* [Qemu-devel] [PULL 13/14] block: Fix potential Null pointer dereferences in vvfat.c
2018-11-12 17:05 [Qemu-devel] [PULL 00/14] Block layer patches Kevin Wolf
` (11 preceding siblings ...)
2018-11-12 17:06 ` [Qemu-devel] [PULL 12/14] qemu-img: assert block_job_get() does not return NULL in img_commit() Kevin Wolf
@ 2018-11-12 17:06 ` Kevin Wolf
2018-11-12 17:06 ` [Qemu-devel] [PULL 14/14] qcow2: Read outside array bounds in qcow2_pre_write_overlap_check() Kevin Wolf
2018-11-13 10:14 ` [Qemu-devel] [PULL 00/14] Block layer patches Peter Maydell
14 siblings, 0 replies; 16+ messages in thread
From: Kevin Wolf @ 2018-11-12 17:06 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, peter.maydell, qemu-devel
From: Liam Merwick <Liam.Merwick@oracle.com>
The calls to find_mapping_for_cluster() may return NULL but it
isn't always checked for before dereferencing the value returned.
Additionally, add some asserts to cover cases where NULL can't
be returned but which might not be obvious at first glance.
Signed-off-by: Liam Merwick <Liam.Merwick@oracle.com>
Message-id: 1541453919-25973-5-git-send-email-Liam.Merwick@oracle.com
[mreitz: Dropped superfluous check of "mapping" following an assertion
that it is not NULL, and fixed some indentation]
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
block/vvfat.c | 46 ++++++++++++++++++++++++++++++----------------
1 file changed, 30 insertions(+), 16 deletions(-)
diff --git a/block/vvfat.c b/block/vvfat.c
index e4df255d58..1de5de1db4 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -100,30 +100,26 @@ static inline void array_free(array_t* array)
/* does not automatically grow */
static inline void* array_get(array_t* array,unsigned int index) {
assert(index < array->next);
+ assert(array->pointer);
return array->pointer + index * array->item_size;
}
-static inline int array_ensure_allocated(array_t* array, int index)
+static inline void array_ensure_allocated(array_t *array, int index)
{
if((index + 1) * array->item_size > array->size) {
int new_size = (index + 32) * array->item_size;
array->pointer = g_realloc(array->pointer, new_size);
- if (!array->pointer)
- return -1;
+ assert(array->pointer);
memset(array->pointer + array->size, 0, new_size - array->size);
array->size = new_size;
array->next = index + 1;
}
-
- return 0;
}
static inline void* array_get_next(array_t* array) {
unsigned int next = array->next;
- if (array_ensure_allocated(array, next) < 0)
- return NULL;
-
+ array_ensure_allocated(array, next);
array->next = next + 1;
return array_get(array, next);
}
@@ -2422,16 +2418,13 @@ static int commit_direntries(BDRVVVFATState* s,
direntry_t* direntry = array_get(&(s->directory), dir_index);
uint32_t first_cluster = dir_index == 0 ? 0 : begin_of_direntry(direntry);
mapping_t* mapping = find_mapping_for_cluster(s, first_cluster);
-
int factor = 0x10 * s->sectors_per_cluster;
int old_cluster_count, new_cluster_count;
- int current_dir_index = mapping->info.dir.first_dir_index;
- int first_dir_index = current_dir_index;
+ int current_dir_index;
+ int first_dir_index;
int ret, i;
uint32_t c;
-DLOG(fprintf(stderr, "commit_direntries for %s, parent_mapping_index %d\n", mapping->path, parent_mapping_index));
-
assert(direntry);
assert(mapping);
assert(mapping->begin == first_cluster);
@@ -2439,6 +2432,11 @@ DLOG(fprintf(stderr, "commit_direntries for %s, parent_mapping_index %d\n", mapp
assert(mapping->mode & MODE_DIRECTORY);
assert(dir_index == 0 || is_directory(direntry));
+ DLOG(fprintf(stderr, "commit_direntries for %s, parent_mapping_index %d\n",
+ mapping->path, parent_mapping_index));
+
+ current_dir_index = mapping->info.dir.first_dir_index;
+ first_dir_index = current_dir_index;
mapping->info.dir.parent_mapping_index = parent_mapping_index;
if (first_cluster == 0) {
@@ -2488,6 +2486,9 @@ DLOG(fprintf(stderr, "commit_direntries for %s, parent_mapping_index %d\n", mapp
direntry = array_get(&(s->directory), first_dir_index + i);
if (is_directory(direntry) && !is_dot(direntry)) {
mapping = find_mapping_for_cluster(s, first_cluster);
+ if (mapping == NULL) {
+ return -1;
+ }
assert(mapping->mode & MODE_DIRECTORY);
ret = commit_direntries(s, first_dir_index + i,
array_index(&(s->mapping), mapping));
@@ -2516,6 +2517,10 @@ static int commit_one_file(BDRVVVFATState* s,
assert(offset < size);
assert((offset % s->cluster_size) == 0);
+ if (mapping == NULL) {
+ return -1;
+ }
+
for (i = s->cluster_size; i < offset; i += s->cluster_size)
c = modified_fat_get(s, c);
@@ -2662,8 +2667,12 @@ static int handle_renames_and_mkdirs(BDRVVVFATState* s)
if (commit->action == ACTION_RENAME) {
mapping_t* mapping = find_mapping_for_cluster(s,
commit->param.rename.cluster);
- char* old_path = mapping->path;
+ char *old_path;
+ if (mapping == NULL) {
+ return -1;
+ }
+ old_path = mapping->path;
assert(commit->path);
mapping->path = commit->path;
if (rename(old_path, mapping->path))
@@ -2684,10 +2693,15 @@ static int handle_renames_and_mkdirs(BDRVVVFATState* s)
direntry_t* d = direntry + i;
if (is_file(d) || (is_directory(d) && !is_dot(d))) {
+ int l;
+ char *new_path;
mapping_t* m = find_mapping_for_cluster(s,
begin_of_direntry(d));
- int l = strlen(m->path);
- char* new_path = g_malloc(l + diff + 1);
+ if (m == NULL) {
+ return -1;
+ }
+ l = strlen(m->path);
+ new_path = g_malloc(l + diff + 1);
assert(!strncmp(m->path, mapping->path, l2));
--
2.19.1
^ permalink raw reply related [flat|nested] 16+ messages in thread* [Qemu-devel] [PULL 14/14] qcow2: Read outside array bounds in qcow2_pre_write_overlap_check()
2018-11-12 17:05 [Qemu-devel] [PULL 00/14] Block layer patches Kevin Wolf
` (12 preceding siblings ...)
2018-11-12 17:06 ` [Qemu-devel] [PULL 13/14] block: Fix potential Null pointer dereferences in vvfat.c Kevin Wolf
@ 2018-11-12 17:06 ` Kevin Wolf
2018-11-13 10:14 ` [Qemu-devel] [PULL 00/14] Block layer patches Peter Maydell
14 siblings, 0 replies; 16+ messages in thread
From: Kevin Wolf @ 2018-11-12 17:06 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, peter.maydell, qemu-devel
From: Liam Merwick <Liam.Merwick@oracle.com>
The commit for 0e4e4318eaa5 increments QCOW2_OL_MAX_BITNR but does not
add an array entry for QCOW2_OL_BITMAP_DIRECTORY_BITNR to metadata_ol_names[].
As a result, an array dereference of metadata_ol_names[8] in
qcow2_pre_write_overlap_check() could result in a read outside of the array bounds.
Fixes: 0e4e4318eaa5 ('qcow2: add overlap check for bitmap directory')
Cc: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Signed-off-by: Liam Merwick <Liam.Merwick@oracle.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
Message-id: 1541453919-25973-6-git-send-email-Liam.Merwick@oracle.com
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
block/qcow2-refcount.c | 18 ++++++++++--------
1 file changed, 10 insertions(+), 8 deletions(-)
diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index 3c539f02e5..46082aeac1 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -2719,15 +2719,17 @@ int qcow2_check_metadata_overlap(BlockDriverState *bs, int ign, int64_t offset,
}
static const char *metadata_ol_names[] = {
- [QCOW2_OL_MAIN_HEADER_BITNR] = "qcow2_header",
- [QCOW2_OL_ACTIVE_L1_BITNR] = "active L1 table",
- [QCOW2_OL_ACTIVE_L2_BITNR] = "active L2 table",
- [QCOW2_OL_REFCOUNT_TABLE_BITNR] = "refcount table",
- [QCOW2_OL_REFCOUNT_BLOCK_BITNR] = "refcount block",
- [QCOW2_OL_SNAPSHOT_TABLE_BITNR] = "snapshot table",
- [QCOW2_OL_INACTIVE_L1_BITNR] = "inactive L1 table",
- [QCOW2_OL_INACTIVE_L2_BITNR] = "inactive L2 table",
+ [QCOW2_OL_MAIN_HEADER_BITNR] = "qcow2_header",
+ [QCOW2_OL_ACTIVE_L1_BITNR] = "active L1 table",
+ [QCOW2_OL_ACTIVE_L2_BITNR] = "active L2 table",
+ [QCOW2_OL_REFCOUNT_TABLE_BITNR] = "refcount table",
+ [QCOW2_OL_REFCOUNT_BLOCK_BITNR] = "refcount block",
+ [QCOW2_OL_SNAPSHOT_TABLE_BITNR] = "snapshot table",
+ [QCOW2_OL_INACTIVE_L1_BITNR] = "inactive L1 table",
+ [QCOW2_OL_INACTIVE_L2_BITNR] = "inactive L2 table",
+ [QCOW2_OL_BITMAP_DIRECTORY_BITNR] = "bitmap directory",
};
+QEMU_BUILD_BUG_ON(QCOW2_OL_MAX_BITNR != ARRAY_SIZE(metadata_ol_names));
/*
* First performs a check for metadata overlaps (through
--
2.19.1
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [Qemu-devel] [PULL 00/14] Block layer patches
2018-11-12 17:05 [Qemu-devel] [PULL 00/14] Block layer patches Kevin Wolf
` (13 preceding siblings ...)
2018-11-12 17:06 ` [Qemu-devel] [PULL 14/14] qcow2: Read outside array bounds in qcow2_pre_write_overlap_check() Kevin Wolf
@ 2018-11-13 10:14 ` Peter Maydell
14 siblings, 0 replies; 16+ messages in thread
From: Peter Maydell @ 2018-11-13 10:14 UTC (permalink / raw)
To: Kevin Wolf; +Cc: Qemu-block, QEMU Developers
On 12 November 2018 at 17:05, Kevin Wolf <kwolf@redhat.com> wrote:
> The following changes since commit 5704c36d25ee84e7129722cb0db53df9faefe943:
>
> Merge remote-tracking branch 'remotes/kraxel/tags/fixes-31-20181112-pull-request' into staging (2018-11-12 15:55:40 +0000)
>
> are available in the Git repository at:
>
> git://repo.or.cz/qemu/kevin.git tags/for-upstream
>
> for you to fetch changes up to 1a42e5d8298d1b0f90d2254e7d559391dd3a45ca:
>
> Merge remote-tracking branch 'mreitz/tags/pull-block-2018-11-12' into queue-block (2018-11-12 17:57:32 +0100)
>
> ----------------------------------------------------------------
> Block layer patches:
>
> - file-posix: Don't waste a file descriptor for locking, don't lock the
> same bit multiple times
> - nvme: Fix double free and memory leak
> - Misc error handling fixes
> - Added NULL checks found by static analysis
> - Allow more block drivers to not be included in the qemu build
>
Applied, thanks.
-- PMM
^ permalink raw reply [flat|nested] 16+ messages in thread