Qcow2 support #1

AnthoineB · 2025-01-23T14:21:28Z

This PR contains different parts:

Tapdisk adaptation to gcc-11. Few warnings/errors are fixed with commit f03fb5f.
Tapdisk cleanup: 3100fcc, 4b00977, ed5b8d7, 8187679, 576172f, 63fb9a9.
Tapdisk use-after-free fix: 4a20003.
Qcow2 sources:

import: 0fb137a, c0d5c11,
modification for tapdisk: 2000fba, an explanation of qcow2 library modification for tapdisk can be found here Qcow2 library portage,
modification for xcp-ng toolchain: eb18c7a.

Tapdisk multithread protection: bf0f129, 1fc1d3c, f2b8b40
Tapdisk's Qcow2 driver: d784290

Errors come from -Wstringop-truncation with gcc-8 and above. Signed-off-by: Anthoine Bourgeois <[email protected]>

Signed-off-by: Anthoine Bourgeois <[email protected]>

Function tapdisk_xenblkif_make_vbd_request will release tapreq->msg and reinsert it in the reqs_free array if the request is a BLKIF_OP_WRITE_BARRIER without any segment. But tapdisk_xenblkif_queue_request will use it to check if it must send the request to VBD layer. This bug triggers a crash when DEBUG is defined in td-reqs.c because DEBUG will poison the msg memory. Without DEBUG, the code reads freed memory that still contains unchanged data. Signed-off-by: Anthoine Bourgeois <[email protected]>

fail label already set vreq->error with err, so remove the first assignment in both paths that lead to fail label. Signed-off-by: Anthoine Bourgeois <[email protected]>

Signed-off-by: Anthoine Bourgeois <[email protected]>

This commit includes a list of files imported from qemu to build the qcow2 library for tapdisk. A small script helps to import files and diff them to ease the sources management. Signed-off-by: Anthoine Bourgeois <[email protected]>

Signed-off-by: Anthoine Bourgeois <[email protected]>

This commit modify sources from qemu to disable some features unneeded for tapdisk. Signed-off-by: Anthoine Bourgeois <[email protected]>

XCP-ng-8.3 doesn't have components recent enough for qemu sources. Fix a few things to support old tools. Signed-off-by: Anthoine Bourgeois <[email protected]>

Add a mutex to protect vbd structure from qcow2 concurrency. Signed-off-by: Anthoine Bourgeois <[email protected]>

Add a mutex to protect blktap structure from qcow2 concurrency. Signed-off-by: Anthoine Bourgeois <[email protected]>

Add a mutex to protect blkif structure from qcow2 concurrency. Signed-off-by: Anthoine Bourgeois <[email protected]>

Signed-off-by: Anthoine Bourgeois <[email protected]>

TSnake41

Did a quick review of this PR if I see things to improve.

Aside this review, I am not completely convinced of directly picking a large part of QEMU code.
I think it would be better to maintain a set of patches for QEMU code and fetching a specific (working) QEMU version on what we apply the patches. It will make maintenance easier in the long term.

TSnake41 · 2025-01-28T13:20:17Z

drivers/block-qcow2.c

+#define QCOW2_OP_READ             1
+#define QCOW2_OP_WRITE            2
+
+#define QCOW2_FLAG_OPEN_RDONLY         1


Using 1 << 0, 1 << 1, ... style would be preferable than raw decimals.

For reference, Xen headers uses a variant of it with a intermediate macro
e.g

/* xen_pci_sharedinfo flags */ #define _XEN_PCIF_active (0) #define XEN_PCIF_active (1<<_XEN_PCIF_active) #define _XEN_PCIB_AERHANDLER (1) #define XEN_PCIB_AERHANDLER (1<<_XEN_PCIB_AERHANDLER) #define _XEN_PCIB_active (2) #define XEN_PCIB_active (1<<_XEN_PCIB_active)

TSnake41 · 2025-01-28T13:22:14Z

drivers/block-qcow2.c

+
+struct qcow2_request {
+    int                     error;
+    uint8_t                 op;


Is there a reason to use uint8_t here specifically ?
I think unsigned int would fit better to make the structure layout less complex and potentially more efficient.

I don't see anything objectionable about using a signed integer of explicit size here. Generally it's even good for storing constants and operations. And regarding performance, I doubt it will help anything regarding buffer reads/writes or writing data to disk.

TSnake41 · 2025-01-28T13:24:31Z

drivers/block-qcow2.c

+    Error *local_err = NULL;
+    BlockConf *conf = NULL;
+    bool writethrough;
+#define AIO_NATIVE


These macros will always be defined (is it temporary ?).
(#else part will never be compiled)

TSnake41 · 2025-01-28T13:31:21Z

drivers/block-qcow2.c

+	.disk_type          = "tapdisk_qcow2",
+	.flags              = 0,
+	.private_data_size  = sizeof(struct qcow2_state),
+	.td_open            = _qcow2_open,


assuming that this can be called multiples times, static variables are going to be overwritten, causing potentially various tricky issues

What can be called multiple times? And what could overwrite these fields? They are normally set only one time.
I don't see any other changes to these fields in the blktap code.

TSnake41 · 2025-01-28T13:31:46Z

drivers/block-qcow2.c

+        bql_unlock();
+}
+
+static pthread_t thread;


You probably want to ensure that these variables don't get overwritten (see .td_open = _qcow2_open) comment

Same questions here. What's the issue?

AnthoineB added 17 commits January 23, 2025 15:28

Fix string copy errors (safe_strncpy)

abb0711

Errors come from -Wstringop-truncation with gcc-8 and above. Signed-off-by: Anthoine Bourgeois <[email protected]>

tapdisk: remove useless code

4d620d2

Signed-off-by: Anthoine Bourgeois <[email protected]>

blktap: libaio: fix a typo

75c2591

Signed-off-by: Anthoine Bourgeois <[email protected]>

tapdisk: doc final param in __tapdisk_xenblkif_request_cb

9b8df99

Signed-off-by: Anthoine Bourgeois <[email protected]>

tapdisk: use defined abstraction

02c4526

Signed-off-by: Anthoine Bourgeois <[email protected]>

tapdisk-vbd: remove double assignment of error variable

66802b7

fail label already set vreq->error with err, so remove the first assignment in both paths that lead to fail label. Signed-off-by: Anthoine Bourgeois <[email protected]>

tapdisk: replace flag number by its name

3346203

Signed-off-by: Anthoine Bourgeois <[email protected]>

qcow2: manage qcow2 sources

adfbc81

This commit includes a list of files imported from qemu to build the qcow2 library for tapdisk. A small script helps to import files and diff them to ease the sources management. Signed-off-by: Anthoine Bourgeois <[email protected]>

qcow2: import vanilla sources from qemu 9.1.1

2ba189b

Signed-off-by: Anthoine Bourgeois <[email protected]>

qcow2: build qcow2 support in tapdisk

ad83e87

This commit modify sources from qemu to disable some features unneeded for tapdisk. Signed-off-by: Anthoine Bourgeois <[email protected]>

qcow2: fix support for old components (gcc, glibc, glib, gnutls)

69a7f9b

XCP-ng-8.3 doesn't have components recent enough for qemu sources. Fix a few things to support old tools. Signed-off-by: Anthoine Bourgeois <[email protected]>

tapdisk: protect td_vbd_t structure

c47bf4e

Add a mutex to protect vbd structure from qcow2 concurrency. Signed-off-by: Anthoine Bourgeois <[email protected]>

tapdisk: protect td_blktap_t structure

bd537c6

Add a mutex to protect blktap structure from qcow2 concurrency. Signed-off-by: Anthoine Bourgeois <[email protected]>

tapdisk: protect td_xenblkif structure

49db0b5

Add a mutex to protect blkif structure from qcow2 concurrency. Signed-off-by: Anthoine Bourgeois <[email protected]>

qcow2: driver: support qcow2 files in tapdisk

2d570a8

Signed-off-by: Anthoine Bourgeois <[email protected]>

blktap.spec: add qcow2 dependencies

e32fd54

Signed-off-by: Anthoine Bourgeois <[email protected]>

AnthoineB force-pushed the qcow2 branch from 1db488d to e32fd54 Compare January 23, 2025 14:29

TSnake41 suggested changes Jan 28, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Qcow2 support #1

Qcow2 support #1

AnthoineB commented Jan 23, 2025

TSnake41 left a comment •

edited

Loading

TSnake41 Jan 28, 2025 •

edited

Loading

TSnake41 Jan 28, 2025

Wescoeur Jan 30, 2025

TSnake41 Jan 28, 2025

TSnake41 Jan 28, 2025 •

edited

Loading

Wescoeur Jan 30, 2025

TSnake41 Jan 28, 2025 •

edited

Loading

Wescoeur Jan 30, 2025 •

edited

Loading

Qcow2 support #1

Are you sure you want to change the base?

Qcow2 support #1

Conversation

AnthoineB commented Jan 23, 2025

TSnake41 left a comment • edited Loading

Choose a reason for hiding this comment

TSnake41 Jan 28, 2025 • edited Loading

Choose a reason for hiding this comment

TSnake41 Jan 28, 2025

Choose a reason for hiding this comment

Wescoeur Jan 30, 2025

Choose a reason for hiding this comment

TSnake41 Jan 28, 2025

Choose a reason for hiding this comment

TSnake41 Jan 28, 2025 • edited Loading

Choose a reason for hiding this comment

Wescoeur Jan 30, 2025

Choose a reason for hiding this comment

TSnake41 Jan 28, 2025 • edited Loading

Choose a reason for hiding this comment

Wescoeur Jan 30, 2025 • edited Loading

Choose a reason for hiding this comment

TSnake41 left a comment •

edited

Loading

TSnake41 Jan 28, 2025 •

edited

Loading

TSnake41 Jan 28, 2025 •

edited

Loading

TSnake41 Jan 28, 2025 •

edited

Loading

Wescoeur Jan 30, 2025 •

edited

Loading