qemu-nbd already has the ability to wrap a read-write layer on top of
the export of a read-only image (-s) and the ability to export an
internal snapshot rather than the main image in a qcow2 file (-l
SNAPID).  But it gave a confusing error message when trying to mix the
two, because the snapshot overlay created first no longer has access
to the internal snapshots of the image being wrapped.

WIth a bit of finesse, this useful scenario is now supported.

https://gitlab.com/qemu-project/qemu/-/work_items/159

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 docs/tools/qemu-nbd.rst                       |  7 +-
 qemu-nbd.c                                    | 20 ++++-
 tests/qemu-iotests/tests/qemu-nbd-snapshots   | 79 +++++++++++++++++++
 .../qemu-iotests/tests/qemu-nbd-snapshots.out | 34 ++++++++
 4 files changed, 134 insertions(+), 6 deletions(-)
 create mode 100755 tests/qemu-iotests/tests/qemu-nbd-snapshots
 create mode 100644 tests/qemu-iotests/tests/qemu-nbd-snapshots.out

diff --git a/docs/tools/qemu-nbd.rst b/docs/tools/qemu-nbd.rst
index f82ea5fd77b..336b4aa46e7 100644
--- a/docs/tools/qemu-nbd.rst
+++ b/docs/tools/qemu-nbd.rst
@@ -95,9 +95,10 @@ driver options if :option:`--image-opts` is specified.

 .. option:: -l, --load-snapshot=SNAPSHOT_PARAM

-  Load an internal snapshot inside *filename* and export it
-  as an read-only device, SNAPSHOT_PARAM format is
-  ``snapshot.id=[ID],snapshot.name=[NAME]`` or ``[ID_OR_NAME]``
+  Load an internal snapshot inside *filename* and export it.
+  SNAPSHOT_PARAM format is ``snapshot.id=[ID],snapshot.name=[NAME]``
+  or ``[ID_OR_NAME]``. The export is read-only unless ``-s`` is also
+  in use.

 .. option:: --cache=CACHE

diff --git a/qemu-nbd.c b/qemu-nbd.c
index ed5895861bb..128486a0ec2 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -534,6 +534,7 @@ int main(int argc, char **argv)
 {
     BlockBackend *blk;
     BlockDriverState *bs;
+    BlockDriverState *orig_bs;
     uint64_t dev_offset = 0;
     bool readonly = false;
     bool disconnect = false;
@@ -716,7 +717,7 @@ int main(int argc, char **argv)
             } else {
                 sn_id_or_name = optarg;
             }
-            /* fall through */
+            break;
         case 'r':
             readonly = true;
             flags &= ~BDRV_O_RDWR;
@@ -1110,6 +1111,15 @@ int main(int argc, char **argv)
     bdrv_init();
     atexit(qemu_nbd_shutdown);

+    /*
+     * -s and -l can be used together to create read-write wrapper
+     * around an internal snapshot. Otherwise, -l in isolation implies
+     * read-only.
+     */
+    if ((sn_opts || sn_id_or_name) && (flags & BDRV_O_SNAPSHOT) == 0) {
+        readonly = true;
+        flags &= ~BDRV_O_RDWR;
+    }
     opts.srcpath = argv[optind];
     if (imageOpts) {
         QemuOpts *o;
@@ -1155,13 +1165,17 @@ int main(int argc, char **argv)

     blk_set_enable_write_cache(blk, !writethrough);

+    orig_bs = bs;
+    if (flags & BDRV_O_SNAPSHOT) {
+        orig_bs = bs->backing->bs;
+    }
     if (sn_opts) {
-        ret = bdrv_snapshot_load_tmp(bs,
+        ret = bdrv_snapshot_load_tmp(orig_bs,
                                      qemu_opt_get(sn_opts, SNAPSHOT_OPT_ID),
                                      qemu_opt_get(sn_opts, SNAPSHOT_OPT_NAME),
                                      &local_err);
     } else if (sn_id_or_name) {
-        ret = bdrv_snapshot_load_tmp_by_id_or_name(bs, sn_id_or_name,
+        ret = bdrv_snapshot_load_tmp_by_id_or_name(orig_bs, sn_id_or_name,
                                                    &local_err);
     }
     if (ret < 0) {
diff --git a/tests/qemu-iotests/tests/qemu-nbd-snapshots b/tests/qemu-iotests/tests/qemu-nbd-snapshots
new file mode 100755
index 00000000000..de405e43720
--- /dev/null
+++ b/tests/qemu-iotests/tests/qemu-nbd-snapshots
@@ -0,0 +1,79 @@
+#!/usr/bin/env bash
+# group: rw quick
+#
+# Test qemu-nbd snapshot handling
+#
+# Copyright (C) Red Hat, Inc.
+#
+# SPDX-License-Identifier: GPL-2.0-or-later
+
+seq="$(basename $0)"
+echo "QA output created by $seq"
+
+status=1 # failure is the default!
+
+_cleanup()
+{
+    _cleanup_test_img
+    nbd_server_stop
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+cd ..
+. ./common.rc
+. ./common.filter
+. ./common.nbd
+
+_supported_fmt qcow2
+_supported_proto file
+_supported_os Linux
+_require_command QEMU_NBD
+
+echo
+echo "=== Initial image setup ==="
+echo
+
+# Create qcow2 image with one internal snapshot
+_make_test_img 10M
+$QEMU_IO -c 'w -P 1 3M 1M' -f $IMGFMT "$TEST_IMG" | _filter_qemu_io
+$QEMU_IMG snapshot -c snap -f $IMGFMT "$TEST_IMG" | _filter_qemu_img
+$QEMU_IO -c 'w -P 2 3M 1M' -f $IMGFMT "$TEST_IMG" | _filter_qemu_io
+
+echo
+echo "=== Main image in snapshot mode ==="
+echo
+
+nbd_server_start_unix_socket -s -f qcow2 "$TEST_IMG"
+$QEMU_IO -c 'r -P 2 3M 1M' -c 'w -P 3 3M 1M' -f raw \
+  nbd:unix:$nbd_unix_socket | _filter_qemu_io
+nbd_server_stop
+
+echo
+echo "=== Read-only snapshot access ==="
+echo
+
+nbd_server_start_unix_socket -l snap -f qcow2 "$TEST_IMG"
+$QEMU_IO -r -c 'r -P 1 3M 1M' -f raw \
+  nbd:unix:$nbd_unix_socket | _filter_qemu_io
+nbd_server_stop
+
+echo
+echo "=== Combine -s and -l for read-write internal snapshot access ==="
+echo
+
+nbd_server_start_unix_socket -s -l snap -f qcow2 "$TEST_IMG"
+$QEMU_IO -c 'r -P 1 3M 1M' -c 'w -P 3 3M 1M' -f raw \
+  nbd:unix:$nbd_unix_socket | _filter_qemu_io
+nbd_server_stop
+
+echo
+echo "=== Check that original image is unchanged ==="
+echo
+
+$QEMU_IO -c 'r -P 2 3M 1M' -f $IMGFMT "$TEST_IMG" | _filter_qemu_io
+
+# success, all done
+echo '*** done'
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/tests/qemu-nbd-snapshots.out b/tests/qemu-iotests/tests/qemu-nbd-snapshots.out
new file mode 100644
index 00000000000..6bce9d90253
--- /dev/null
+++ b/tests/qemu-iotests/tests/qemu-nbd-snapshots.out
@@ -0,0 +1,34 @@
+QA output created by qemu-nbd-snapshots
+
+=== Initial image setup ===
+
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=10485760
+wrote 1048576/1048576 bytes at offset 3145728
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+wrote 1048576/1048576 bytes at offset 3145728
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+=== Main image in snapshot mode ===
+
+read 1048576/1048576 bytes at offset 3145728
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+wrote 1048576/1048576 bytes at offset 3145728
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+=== Read-only snapshot access ===
+
+read 1048576/1048576 bytes at offset 3145728
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+=== Combine -s and -l for read-write internal snapshot access ===
+
+read 1048576/1048576 bytes at offset 3145728
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+wrote 1048576/1048576 bytes at offset 3145728
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+=== Check that original image is unchanged ===
+
+read 1048576/1048576 bytes at offset 3145728
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+*** done
-- 
2.53.0

On 08.04.26 05:16, Eric Blake wrote:
> qemu-nbd already has the ability to wrap a read-write layer on top of
> the export of a read-only image (-s) and the ability to export an
> internal snapshot rather than the main image in a qcow2 file (-l
> SNAPID).  But it gave a confusing error message when trying to mix the
> two, because the snapshot overlay created first no longer has access
> to the internal snapshots of the image being wrapped.
> 
> WIth a bit of finesse, this useful scenario is now supported.
> 
> https://gitlab.com/qemu-project/qemu/-/work_items/159
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>

Some optional suggestions below:

> +
> +echo
> +echo "=== Main image in snapshot mode ==="
> +echo
> +
> +nbd_server_start_unix_socket -s -f qcow2 "$TEST_IMG"
> +$QEMU_IO -c 'r -P 2 3M 1M' -c 'w -P 3 3M 1M' -f raw \

Maybe, add read command after write, to check write correctness. [*]

> +  nbd:unix:$nbd_unix_socket | _filter_qemu_io
> +nbd_server_stop
> +
> +echo
> +echo "=== Read-only snapshot access ==="
> +echo
> +
> +nbd_server_start_unix_socket -l snap -f qcow2 "$TEST_IMG"
> +$QEMU_IO -r -c 'r -P 1 3M 1M' -f raw \

Suggest, add write command, to check that it fails.

> +  nbd:unix:$nbd_unix_socket | _filter_qemu_io
> +nbd_server_stop
> +
> +echo
> +echo "=== Combine -s and -l for read-write internal snapshot access ==="
> +echo
> +
> +nbd_server_start_unix_socket -s -l snap -f qcow2 "$TEST_IMG"
> +$QEMU_IO -c 'r -P 1 3M 1M' -c 'w -P 3 3M 1M' -f raw \

[*] and here

> +  nbd:unix:$nbd_unix_socket | _filter_qemu_io
> +nbd_server_stop
> +
> +echo
> +echo "=== Check that original image is unchanged ==="
> +echo
> +



-- 
Best regards,
Vladimir