fbdea3d6 | 13-Nov-2024 |
Jakub Jelen <jjelen@redhat.com> |
ssh: Do not switch session to non-blocking mode
The libssh does not handle non-blocking mode in SFTP correctly. The driver code already changes the mode to blocking for the SFTP initialization, but
ssh: Do not switch session to non-blocking mode
The libssh does not handle non-blocking mode in SFTP correctly. The driver code already changes the mode to blocking for the SFTP initialization, but for some reason changes to non-blocking mode. This used to work accidentally until libssh in 0.11 branch merged the patch to avoid infinite looping in case of network errors:
https://gitlab.com/libssh/libssh-mirror/-/merge_requests/498
Since then, the ssh driver in qemu fails to read files over SFTP as the first SFTP messages exchanged after switching the session to non-blocking mode return SSH_AGAIN, but that message is lost int the SFTP internals and interpretted as SSH_ERROR, which is returned to the caller:
https://gitlab.com/libssh/libssh-mirror/-/issues/280
This is indeed an issue in libssh that we should address in the long term, but it will require more work on the internals. For now, the SFTP is not supported in non-blocking mode.
Fixes: https://gitlab.com/libssh/libssh-mirror/-/issues/280 Signed-off-by: Jakub Jelen <jjelen@redhat.com> Signed-off-by: Richard W.M. Jones <rjones@redhat.com> Message-ID: <20241113125526.2495731-1-rjones@redhat.com> Reviewed-by: Kevin Wolf <kwolf@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
show more ...
|
04bbc3ee | 29-Aug-2024 |
Kevin Wolf <kwolf@redhat.com> |
raw-format: Fix error message for invalid offset/size
s->offset and s->size are only set at the end of the function and still contain the old values when formatting the error message. Print the para
raw-format: Fix error message for invalid offset/size
s->offset and s->size are only set at the end of the function and still contain the old values when formatting the error message. Print the parameters with the new values that we actually checked instead.
Fixes: 500e2434207d ('raw-format: Split raw_read_options()') Signed-off-by: Kevin Wolf <kwolf@redhat.com> Message-ID: <20240829185527.47152-1-kwolf@redhat.com> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Reviewed-by: Hanna Czenczek <hreitz@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
show more ...
|
7452162a | 02-Oct-2024 |
Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> |
qapi: add qom-path to BLOCK_IO_ERROR event
We need something more reliable than "device" (which absent in modern interfaces) and "node-name" (which may absent, and actually don't specify the device,
qapi: add qom-path to BLOCK_IO_ERROR event
We need something more reliable than "device" (which absent in modern interfaces) and "node-name" (which may absent, and actually don't specify the device, which is a source of error) to make a per-device throttling for the event in the following commit.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> Message-ID: <20241002151806.592469-2-vsementsov@yandex-team.ru> Reviewed-by: Kevin Wolf <kwolf@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
show more ...
|
4d7c5f83 | 08-Oct-2024 |
Peter Maydell <peter.maydell@linaro.org> |
block/vdi.c: Make SECTOR_SIZE constant 64-bits
Make the VDI SECTOR_SIZE define be a 64-bit constant; this matches how we define BDRV_SECTOR_SIZE. The benefit is that it means that we don't need to
block/vdi.c: Make SECTOR_SIZE constant 64-bits
Make the VDI SECTOR_SIZE define be a 64-bit constant; this matches how we define BDRV_SECTOR_SIZE. The benefit is that it means that we don't need to carefully cast to 64-bits when doing operations like "n_sectors * SECTOR_SIZE" to avoid doing a 32x32->32 multiply, which might overflow, and which Coverity and other static analysers tend to warn about.
The specific potential overflow Coverity is highlighting is the one at the end of vdi_co_pwritev() where we write out n_sectors sectors to the block map. This is very unlikely to actually overflow, since the block map has 4 bytes per block and the maximum number of blocks in the image must fit into a 32-bit integer. So this commit is not fixing a real-world bug.
An inspection of all the places currently using SECTOR_SIZE in the file shows none which care about the change in its type, except for one call to error_setg() which needs the format string adjusting.
Resolves: Coverity CID 1508076 Suggested-by: Kevin Wolf <kwolf@redhat.com> Message-ID: <20241008164708.2966400-5-peter.maydell@linaro.org> Reviewed-by: Kevin Wolf <kwolf@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
show more ...
|
19c1e441 | 08-Oct-2024 |
Peter Maydell <peter.maydell@linaro.org> |
block/ssh.c: Don't double-check that characters are hex digits
In compare_fingerprint() we effectively check whether the characters in the fingerprint are valid hex digits twice: first we do so with
block/ssh.c: Don't double-check that characters are hex digits
In compare_fingerprint() we effectively check whether the characters in the fingerprint are valid hex digits twice: first we do so with qemu_isxdigit(), but then the hex2decimal() function also has a code path where it effectively detects an invalid digit and returns -1. This causes Coverity to complain because it thinks that we might use that -1 value in an expression where it would be an integer overflow.
Avoid the double-check of hex digit validity by testing the return values from hex2decimal() rather than doing separate calls to qemu_isxdigit().
Since this means we now use the illegal-character return value from hex2decimal(), rewrite it from "-1" to "UINT_MAX", which has the same effect since the return type is "unsigned" but looks less confusing at the callsites when we detect it with "c0 > 0xf".
Resolves: Coverity CID 1547813 Signed-off-by: Peter Maydell <peter.maydell@linaro.org> Reviewed-by: Kevin Wolf <kwolf@redhat.com> Message-ID: <20241008164708.2966400-3-peter.maydell@linaro.org> Reviewed-by: Richard Henderson <richard.henderson@linaro.org> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
show more ...
|
ce2a0ef6 | 28-Mar-2024 |
Marc-André Lureau <marcandre.lureau@redhat.com> |
block/stream: fix -Werror=maybe-uninitialized false-positives
../block/stream.c:193:19: error: ‘unfiltered_bs’ may be used uninitialized [-Werror=maybe-uninitialized] ../block/stream.c:176:5: error:
block/stream: fix -Werror=maybe-uninitialized false-positives
../block/stream.c:193:19: error: ‘unfiltered_bs’ may be used uninitialized [-Werror=maybe-uninitialized] ../block/stream.c:176:5: error: ‘len’ may be used uninitialized [-Werror=maybe-uninitialized] trace/trace-block.h:906:9: error: ‘ret’ may be used uninitialized [-Werror=maybe-uninitialized]
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> Acked-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> Reviewed-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
show more ...
|
5791ba52 | 09-Apr-2024 |
Marc-André Lureau <marcandre.lureau@redhat.com> |
block/mirror: fix -Werror=maybe-uninitialized false-positive
../block/mirror.c:404:5: error: ‘ret’ may be used uninitialized [-Werror=maybe-uninitialized] ../block/mirror.c:895:12: error: ‘ret’ may
block/mirror: fix -Werror=maybe-uninitialized false-positive
../block/mirror.c:404:5: error: ‘ret’ may be used uninitialized [-Werror=maybe-uninitialized] ../block/mirror.c:895:12: error: ‘ret’ may be used uninitialized [-Werror=maybe-uninitialized] ../block/mirror.c:578:12: error: ‘ret’ may be used uninitialized [-Werror=maybe-uninitialized]
Change a variable to int, as suggested by Manos: "bdrv_co_preadv() which is int and is passed as an int argument to mirror_read_complete()"
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
show more ...
|
e84af3eb | 16-Sep-2024 |
Dr. David Alan Gilbert <dave@treblig.org> |
block: Remove unused aio_task_pool_empty
aio_task_pool_empty has been unused since it was added in 6e9b225f73 ("block: introduce aio task pool")
Remove it.
Signed-off-by: Dr. David Alan Gilbert
block: Remove unused aio_task_pool_empty
aio_task_pool_empty has been unused since it was added in 6e9b225f73 ("block: introduce aio task pool")
Remove it.
Signed-off-by: Dr. David Alan Gilbert <dave@treblig.org> Message-Id: <20240917002007.330689-1-dave@treblig.org> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
show more ...
|
6475155d | 12-Jul-2024 |
Fiona Ebner <f.ebner@proxmox.com> |
block/reqlist: allow adding overlapping requests
Allow overlapping request by removing the assert that made it impossible. There are only two callers:
1. block_copy_task_create()
It already assert
block/reqlist: allow adding overlapping requests
Allow overlapping request by removing the assert that made it impossible. There are only two callers:
1. block_copy_task_create()
It already asserts the very same condition before calling reqlist_init_req().
2. cbw_snapshot_read_lock()
There is no need to have read requests be non-overlapping in copy-before-write when used for snapshot-access. In fact, there was no protection against two callers of cbw_snapshot_read_lock() calling reqlist_init_req() with overlapping ranges and this could lead to an assertion failure [1].
In particular, with the reproducer script below [0], two cbw_co_snapshot_block_status() callers could race, with the second calling reqlist_init_req() before the first one finishes and removes its conflicting request.
[0]:
> #!/bin/bash -e > dd if=/dev/urandom of=/tmp/disk.raw bs=1M count=1024 > ./qemu-img create /tmp/fleecing.raw -f raw 1G > ( > ./qemu-system-x86_64 --qmp stdio \ > --blockdev raw,node-name=node0,file.driver=file,file.filename=/tmp/disk.raw \ > --blockdev raw,node-name=node1,file.driver=file,file.filename=/tmp/fleecing.raw \ > <<EOF > {"execute": "qmp_capabilities"} > {"execute": "blockdev-add", "arguments": { "driver": "copy-before-write", "file": "node0", "target": "node1", "node-name": "node3" } } > {"execute": "blockdev-add", "arguments": { "driver": "snapshot-access", "file": "node3", "node-name": "snap0" } } > {"execute": "nbd-server-start", "arguments": {"addr": { "type": "unix", "data": { "path": "/tmp/nbd.socket" } } } } > {"execute": "block-export-add", "arguments": {"id": "exp0", "node-name": "snap0", "type": "nbd", "name": "exp0"}} > EOF > ) & > sleep 5 > while true; do > ./qemu-nbd -d /dev/nbd0 > ./qemu-nbd -c /dev/nbd0 nbd:unix:/tmp/nbd.socket:exportname=exp0 -f raw -r > nbdinfo --map 'nbd+unix:///exp0?socket=/tmp/nbd.socket' > done
[1]:
> #5 0x000071e5f0088eb2 in __GI___assert_fail (...) at ./assert/assert.c:101 > #6 0x0000615285438017 in reqlist_init_req (...) at ../block/reqlist.c:23 > #7 0x00006152853e2d98 in cbw_snapshot_read_lock (...) at ../block/copy-before-write.c:237 > #8 0x00006152853e3068 in cbw_co_snapshot_block_status (...) at ../block/copy-before-write.c:304 > #9 0x00006152853f4d22 in bdrv_co_snapshot_block_status (...) at ../block/io.c:3726 > #10 0x000061528543a63e in snapshot_access_co_block_status (...) at ../block/snapshot-access.c:48 > #11 0x00006152853f1a0a in bdrv_co_do_block_status (...) at ../block/io.c:2474 > #12 0x00006152853f2016 in bdrv_co_common_block_status_above (...) at ../block/io.c:2652 > #13 0x00006152853f22cf in bdrv_co_block_status_above (...) at ../block/io.c:2732 > #14 0x00006152853d9a86 in blk_co_block_status_above (...) at ../block/block-backend.c:1473 > #15 0x000061528538da6c in blockstatus_to_extents (...) at ../nbd/server.c:2374 > #16 0x000061528538deb1 in nbd_co_send_block_status (...) at ../nbd/server.c:2481 > #17 0x000061528538f424 in nbd_handle_request (...) at ../nbd/server.c:2978 > #18 0x000061528538f906 in nbd_trip (...) at ../nbd/server.c:3121 > #19 0x00006152855a7caf in coroutine_trampoline (...) at ../util/coroutine-ucontext.c:175
Cc: qemu-stable@nongnu.org Suggested-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com> Message-Id: <20240712140716.517911-1-f.ebner@proxmox.com> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
show more ...
|
6252deb2 | 11-Jul-2024 |
Fiona Ebner <f.ebner@proxmox.com> |
backup: add minimum cluster size to performance options
In the context of backup fleecing, discarding the source will not work when the fleecing image has a larger granularity than the one used for
backup: add minimum cluster size to performance options
In the context of backup fleecing, discarding the source will not work when the fleecing image has a larger granularity than the one used for block-copy operations (can happen if the backup target has smaller cluster size), because cbw_co_pdiscard_snapshot() will align down the discard requests and thus effectively ignore then.
To make @discard-source work in such a scenario, allow specifying the minimum cluster size used for block-copy operations and thus in particular also the granularity for discard requests to the source.
Suggested-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> Acked-by: Markus Armbruster <armbru@redhat.com> (QAPI schema) Signed-off-by: Fiona Ebner <f.ebner@proxmox.com> Message-Id: <20240711120915.310243-3-f.ebner@proxmox.com> [vsementsov: switch version to 9.2 in QAPI doc] Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
show more ...
|
9484ad6c | 11-Jul-2024 |
Fiona Ebner <f.ebner@proxmox.com> |
copy-before-write: allow specifying minimum cluster size
In the context of backup fleecing, discarding the source will not work when the fleecing image has a larger granularity than the one used for
copy-before-write: allow specifying minimum cluster size
In the context of backup fleecing, discarding the source will not work when the fleecing image has a larger granularity than the one used for block-copy operations (can happen if the backup target has smaller cluster size), because cbw_co_pdiscard_snapshot() will align down the discard requests and thus effectively ignore then.
To make @discard-source work in such a scenario, allow specifying the minimum cluster size used for block-copy operations and thus in particular also the granularity for discard requests to the source.
The type 'size' (corresponding to uint64_t in C) is used in QAPI to rule out negative inputs and for consistency with already existing @cluster-size parameters. Since block_copy_calculate_cluster_size() uses int64_t for its result, a check that the input is not too large is added in block_copy_state_new() before calling it. The calculation in block_copy_calculate_cluster_size() is done in the target int64_t type.
Suggested-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> Acked-by: Markus Armbruster <armbru@redhat.com> (QAPI schema) Signed-off-by: Fiona Ebner <f.ebner@proxmox.com> Message-Id: <20240711120915.310243-2-f.ebner@proxmox.com> [vsementsov: switch version to 9.2 in QAPI doc] Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
show more ...
|
a092c513 | 04-Sep-2024 |
Markus Armbruster <armbru@redhat.com> |
qapi/crypto: Rename QCryptoCipherAlgorithm to *Algo, and drop prefix
QAPI's 'prefix' feature can make the connection between enumeration type and its constants less than obvious. It's best used wit
qapi/crypto: Rename QCryptoCipherAlgorithm to *Algo, and drop prefix
QAPI's 'prefix' feature can make the connection between enumeration type and its constants less than obvious. It's best used with restraint.
QCryptoCipherAlgorithm has a 'prefix' that overrides the generated enumeration constants' prefix to QCRYPTO_CIPHER_ALG.
We could simply drop 'prefix', but then the prefix becomes QCRYPTO_CIPHER_ALGORITHM, which is rather long.
We could additionally rename the type to QCryptoCipherAlg, but I think the abbreviation "alg" is less than clear.
Rename the type to QCryptoCipherAlgo instead. The prefix becomes QCRYPTO_CIPHER_ALGO.
Signed-off-by: Markus Armbruster <armbru@redhat.com> Acked-by: Daniel P. Berrangé <berrange@redhat.com> Message-ID: <20240904111836.3273842-13-armbru@redhat.com>
show more ...
|