* [Qemu-devel] [PATCH] Add infrastructure to forbid external snapshot @ 2013-09-25 14:23 Benoît Canet 2013-09-25 14:23 ` [Qemu-devel] [PATCH] block: Add bdrv_forbid_ext_snapshots Benoît Canet 0 siblings, 1 reply; 6+ messages in thread From: Benoît Canet @ 2013-09-25 14:23 UTC (permalink / raw) To: qemu-devel; +Cc: kwolf, Benoît Canet, stefanha This paves the way for the quorum block driver to be merged. Benoît Canet (1): block: Add bdrv_forbid_ext_snapshots. block.c | 27 +++++++++++++++++++++++++++ block/blkdebug.c | 2 ++ block/blkverify.c | 2 ++ block/bochs.c | 2 ++ block/cloop.c | 2 ++ block/cow.c | 2 ++ block/curl.c | 10 ++++++++++ block/dmg.c | 2 ++ block/gluster.c | 8 ++++++++ block/iscsi.c | 2 ++ block/nbd.c | 6 ++++++ block/parallels.c | 2 ++ block/qcow.c | 2 ++ block/qcow2.c | 2 ++ block/qed.c | 2 ++ block/raw-posix.c | 10 ++++++++++ block/raw-win32.c | 4 ++++ block/raw_bsd.c | 4 +++- block/rbd.c | 2 ++ block/sheepdog.c | 6 ++++++ block/ssh.c | 2 ++ block/vdi.c | 2 ++ block/vhdx.c | 2 ++ block/vmdk.c | 2 ++ block/vpc.c | 2 ++ block/vvfat.c | 2 ++ blockdev.c | 5 +++++ include/block/block.h | 10 ++++++++++ include/block/block_int.h | 7 +++++++ 29 files changed, 132 insertions(+), 1 deletion(-) -- 1.8.1.2 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [Qemu-devel] [PATCH] block: Add bdrv_forbid_ext_snapshots. 2013-09-25 14:23 [Qemu-devel] [PATCH] Add infrastructure to forbid external snapshot Benoît Canet @ 2013-09-25 14:23 ` Benoît Canet 2013-09-26 2:01 ` Jeff Cody 0 siblings, 1 reply; 6+ messages in thread From: Benoît Canet @ 2013-09-25 14:23 UTC (permalink / raw) To: qemu-devel; +Cc: kwolf, Benoît Canet, stefanha Drivers having a bs->file where set to recurse the call to their child. Protocol and drivers designed to be on the bottom of the stack where set to allow snapshots. Future protocols like quorum where creating snapshots does not make sense without block filters will be set to forbid snapshots. Signed-off-by: Benoit Canet <benoit@irqsave.net> --- block.c | 27 +++++++++++++++++++++++++++ block/blkdebug.c | 2 ++ block/blkverify.c | 2 ++ block/bochs.c | 2 ++ block/cloop.c | 2 ++ block/cow.c | 2 ++ block/curl.c | 10 ++++++++++ block/dmg.c | 2 ++ block/gluster.c | 8 ++++++++ block/iscsi.c | 2 ++ block/nbd.c | 6 ++++++ block/parallels.c | 2 ++ block/qcow.c | 2 ++ block/qcow2.c | 2 ++ block/qed.c | 2 ++ block/raw-posix.c | 10 ++++++++++ block/raw-win32.c | 4 ++++ block/raw_bsd.c | 4 +++- block/rbd.c | 2 ++ block/sheepdog.c | 6 ++++++ block/ssh.c | 2 ++ block/vdi.c | 2 ++ block/vhdx.c | 2 ++ block/vmdk.c | 2 ++ block/vpc.c | 2 ++ block/vvfat.c | 2 ++ blockdev.c | 5 +++++ include/block/block.h | 10 ++++++++++ include/block/block_int.h | 7 +++++++ 29 files changed, 132 insertions(+), 1 deletion(-) diff --git a/block.c b/block.c index 4a98250..ff296df 100644 --- a/block.c +++ b/block.c @@ -4651,3 +4651,30 @@ int bdrv_amend_options(BlockDriverState *bs, QEMUOptionParameter *options) } return bs->drv->bdrv_amend_options(bs, options); } + +bool bdrv_is_ext_snapshot_forbidden(BlockDriverState *bs) +{ + /* allow snapshot by default */ + if (!bs->drv->bdrv_forbid_ext_snapshots) { + return false; + } + return bs->drv->bdrv_forbid_ext_snapshots(bs); +} + +bool bdrv_recurse_forbid_ext_snapshots(BlockDriverState *bs) +{ + if (!bs->file->drv) { + return false; + } + return bs->file->drv->bdrv_forbid_ext_snapshots(bs->file); +} + +bool bdrv_forbid_ext_snapshots(BlockDriverState *bs) +{ + return true; +} + +bool bdrv_allow_ext_snapshots(BlockDriverState *bs) +{ + return false; +} diff --git a/block/blkdebug.c b/block/blkdebug.c index be948b2..5acee0f 100644 --- a/block/blkdebug.c +++ b/block/blkdebug.c @@ -641,6 +641,8 @@ static BlockDriver bdrv_blkdebug = { .bdrv_debug_breakpoint = blkdebug_debug_breakpoint, .bdrv_debug_resume = blkdebug_debug_resume, .bdrv_debug_is_suspended = blkdebug_debug_is_suspended, + + .bdrv_forbid_ext_snapshots = bdrv_recurse_forbid_ext_snapshots, }; static void bdrv_blkdebug_init(void) diff --git a/block/blkverify.c b/block/blkverify.c index 2077d8a..7890adb 100644 --- a/block/blkverify.c +++ b/block/blkverify.c @@ -313,6 +313,8 @@ static BlockDriver bdrv_blkverify = { .bdrv_aio_readv = blkverify_aio_readv, .bdrv_aio_writev = blkverify_aio_writev, .bdrv_aio_flush = blkverify_aio_flush, + + .bdrv_forbid_ext_snapshots = bdrv_forbid_ext_snapshots, }; static void bdrv_blkverify_init(void) diff --git a/block/bochs.c b/block/bochs.c index 51d9a90..55c13c5 100644 --- a/block/bochs.c +++ b/block/bochs.c @@ -242,6 +242,8 @@ static BlockDriver bdrv_bochs = { .bdrv_open = bochs_open, .bdrv_read = bochs_co_read, .bdrv_close = bochs_close, + + .bdrv_forbid_ext_snapshots = bdrv_recurse_forbid_ext_snapshots, }; static void bdrv_bochs_init(void) diff --git a/block/cloop.c b/block/cloop.c index b907023..bb28758 100644 --- a/block/cloop.c +++ b/block/cloop.c @@ -195,6 +195,8 @@ static BlockDriver bdrv_cloop = { .bdrv_open = cloop_open, .bdrv_read = cloop_co_read, .bdrv_close = cloop_close, + + .bdrv_forbid_ext_snapshots = bdrv_recurse_forbid_ext_snapshots, }; static void bdrv_cloop_init(void) diff --git a/block/cow.c b/block/cow.c index 909c3e7..ceb786a 100644 --- a/block/cow.c +++ b/block/cow.c @@ -393,6 +393,8 @@ static BlockDriver bdrv_cow = { .bdrv_co_get_block_status = cow_co_get_block_status, .create_options = cow_create_options, + + .bdrv_forbid_ext_snapshots = bdrv_recurse_forbid_ext_snapshots, }; static void bdrv_cow_init(void) diff --git a/block/curl.c b/block/curl.c index 5a46f97..ac3c94a 100644 --- a/block/curl.c +++ b/block/curl.c @@ -617,6 +617,8 @@ static BlockDriver bdrv_http = { .bdrv_getlength = curl_getlength, .bdrv_aio_readv = curl_aio_readv, + + .bdrv_forbid_ext_snapshots = bdrv_allow_ext_snapshots, }; static BlockDriver bdrv_https = { @@ -630,6 +632,8 @@ static BlockDriver bdrv_https = { .bdrv_getlength = curl_getlength, .bdrv_aio_readv = curl_aio_readv, + + .bdrv_forbid_ext_snapshots = bdrv_allow_ext_snapshots, }; static BlockDriver bdrv_ftp = { @@ -643,6 +647,8 @@ static BlockDriver bdrv_ftp = { .bdrv_getlength = curl_getlength, .bdrv_aio_readv = curl_aio_readv, + + .bdrv_forbid_ext_snapshots = bdrv_allow_ext_snapshots, }; static BlockDriver bdrv_ftps = { @@ -656,6 +662,8 @@ static BlockDriver bdrv_ftps = { .bdrv_getlength = curl_getlength, .bdrv_aio_readv = curl_aio_readv, + + .bdrv_forbid_ext_snapshots = bdrv_allow_ext_snapshots, }; static BlockDriver bdrv_tftp = { @@ -669,6 +677,8 @@ static BlockDriver bdrv_tftp = { .bdrv_getlength = curl_getlength, .bdrv_aio_readv = curl_aio_readv, + + .bdrv_forbid_ext_snapshots = bdrv_allow_ext_snapshots, }; static void curl_block_init(void) diff --git a/block/dmg.c b/block/dmg.c index d5e9b1f..96e7103 100644 --- a/block/dmg.c +++ b/block/dmg.c @@ -382,6 +382,8 @@ static BlockDriver bdrv_dmg = { .bdrv_open = dmg_open, .bdrv_read = dmg_co_read, .bdrv_close = dmg_close, + + .bdrv_forbid_ext_snapshots = bdrv_recurse_forbid_ext_snapshots, }; static void bdrv_dmg_init(void) diff --git a/block/gluster.c b/block/gluster.c index 877686a..dfb7997 100644 --- a/block/gluster.c +++ b/block/gluster.c @@ -626,6 +626,8 @@ static BlockDriver bdrv_gluster = { .bdrv_aio_discard = qemu_gluster_aio_discard, #endif .create_options = qemu_gluster_create_options, + + .bdrv_forbid_ext_snapshots = bdrv_allow_ext_snapshots, }; static BlockDriver bdrv_gluster_tcp = { @@ -647,6 +649,8 @@ static BlockDriver bdrv_gluster_tcp = { .bdrv_aio_discard = qemu_gluster_aio_discard, #endif .create_options = qemu_gluster_create_options, + + .bdrv_forbid_ext_snapshots = bdrv_allow_ext_snapshots, }; static BlockDriver bdrv_gluster_unix = { @@ -668,6 +672,8 @@ static BlockDriver bdrv_gluster_unix = { .bdrv_aio_discard = qemu_gluster_aio_discard, #endif .create_options = qemu_gluster_create_options, + + .bdrv_forbid_ext_snapshots = bdrv_allow_ext_snapshots, }; static BlockDriver bdrv_gluster_rdma = { @@ -689,6 +695,8 @@ static BlockDriver bdrv_gluster_rdma = { .bdrv_aio_discard = qemu_gluster_aio_discard, #endif .create_options = qemu_gluster_create_options, + + .bdrv_forbid_ext_snapshots = bdrv_allow_ext_snapshots, }; static void bdrv_gluster_init(void) diff --git a/block/iscsi.c b/block/iscsi.c index 6152ef1..4cc7ce0 100644 --- a/block/iscsi.c +++ b/block/iscsi.c @@ -1544,6 +1544,8 @@ static BlockDriver bdrv_iscsi = { .bdrv_ioctl = iscsi_ioctl, .bdrv_aio_ioctl = iscsi_aio_ioctl, #endif + + .bdrv_forbid_ext_snapshots = bdrv_allow_ext_snapshots, }; static QemuOptsList qemu_iscsi_opts = { diff --git a/block/nbd.c b/block/nbd.c index c8deeee..7eac042 100644 --- a/block/nbd.c +++ b/block/nbd.c @@ -651,6 +651,8 @@ static BlockDriver bdrv_nbd = { .bdrv_co_flush_to_os = nbd_co_flush, .bdrv_co_discard = nbd_co_discard, .bdrv_getlength = nbd_getlength, + + .bdrv_forbid_ext_snapshots = bdrv_allow_ext_snapshots, }; static BlockDriver bdrv_nbd_tcp = { @@ -665,6 +667,8 @@ static BlockDriver bdrv_nbd_tcp = { .bdrv_co_flush_to_os = nbd_co_flush, .bdrv_co_discard = nbd_co_discard, .bdrv_getlength = nbd_getlength, + + .bdrv_forbid_ext_snapshots = bdrv_allow_ext_snapshots, }; static BlockDriver bdrv_nbd_unix = { @@ -679,6 +683,8 @@ static BlockDriver bdrv_nbd_unix = { .bdrv_co_flush_to_os = nbd_co_flush, .bdrv_co_discard = nbd_co_discard, .bdrv_getlength = nbd_getlength, + + .bdrv_forbid_ext_snapshots = bdrv_allow_ext_snapshots, }; static void bdrv_nbd_init(void) diff --git a/block/parallels.c b/block/parallels.c index 2121e43..01ee036 100644 --- a/block/parallels.c +++ b/block/parallels.c @@ -168,6 +168,8 @@ static BlockDriver bdrv_parallels = { .bdrv_open = parallels_open, .bdrv_read = parallels_co_read, .bdrv_close = parallels_close, + + .bdrv_forbid_ext_snapshots = bdrv_recurse_forbid_ext_snapshots, }; static void bdrv_parallels_init(void) diff --git a/block/qcow.c b/block/qcow.c index c470e05..47f3b44 100644 --- a/block/qcow.c +++ b/block/qcow.c @@ -918,6 +918,8 @@ static BlockDriver bdrv_qcow = { .bdrv_get_info = qcow_get_info, .create_options = qcow_create_options, + + .bdrv_forbid_ext_snapshots = bdrv_recurse_forbid_ext_snapshots, }; static void bdrv_qcow_init(void) diff --git a/block/qcow2.c b/block/qcow2.c index 318d95d..c560133b 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -2141,6 +2141,8 @@ static BlockDriver bdrv_qcow2 = { .create_options = qcow2_create_options, .bdrv_check = qcow2_check, .bdrv_amend_options = qcow2_amend_options, + + .bdrv_forbid_ext_snapshots = bdrv_recurse_forbid_ext_snapshots, }; static void bdrv_qcow2_init(void) diff --git a/block/qed.c b/block/qed.c index 6c0cba0..e7aaa98 100644 --- a/block/qed.c +++ b/block/qed.c @@ -1615,6 +1615,8 @@ static BlockDriver bdrv_qed = { .bdrv_change_backing_file = bdrv_qed_change_backing_file, .bdrv_invalidate_cache = bdrv_qed_invalidate_cache, .bdrv_check = bdrv_qed_check, + + .bdrv_forbid_ext_snapshots = bdrv_recurse_forbid_ext_snapshots, }; static void bdrv_qed_init(void) diff --git a/block/raw-posix.c b/block/raw-posix.c index f7f102d..ab2a64b 100644 --- a/block/raw-posix.c +++ b/block/raw-posix.c @@ -1222,6 +1222,8 @@ static BlockDriver bdrv_file = { = raw_get_allocated_file_size, .create_options = raw_create_options, + + .bdrv_forbid_ext_snapshots = bdrv_allow_ext_snapshots, }; /***********************************************/ @@ -1568,6 +1570,8 @@ static BlockDriver bdrv_host_device = { .bdrv_ioctl = hdev_ioctl, .bdrv_aio_ioctl = hdev_aio_ioctl, #endif + + .bdrv_forbid_ext_snapshots = bdrv_allow_ext_snapshots, }; #ifdef __linux__ @@ -1692,6 +1696,8 @@ static BlockDriver bdrv_host_floppy = { .bdrv_is_inserted = floppy_is_inserted, .bdrv_media_changed = floppy_media_changed, .bdrv_eject = floppy_eject, + + .bdrv_forbid_ext_snapshots = bdrv_allow_ext_snapshots, }; static int cdrom_open(BlockDriverState *bs, QDict *options, int flags, @@ -1799,6 +1805,8 @@ static BlockDriver bdrv_host_cdrom = { /* generic scsi device */ .bdrv_ioctl = hdev_ioctl, .bdrv_aio_ioctl = hdev_aio_ioctl, + + .bdrv_forbid_ext_snapshots = bdrv_allow_ext_snapshots, }; #endif /* __linux__ */ @@ -1917,6 +1925,8 @@ static BlockDriver bdrv_host_cdrom = { .bdrv_is_inserted = cdrom_is_inserted, .bdrv_eject = cdrom_eject, .bdrv_lock_medium = cdrom_lock_medium, + + .bdrv_forbid_ext_snapshots = bdrv_allow_ext_snapshots, }; #endif /* __FreeBSD__ */ diff --git a/block/raw-win32.c b/block/raw-win32.c index 6ef320f..3479881 100644 --- a/block/raw-win32.c +++ b/block/raw-win32.c @@ -475,6 +475,8 @@ static BlockDriver bdrv_file = { = raw_get_allocated_file_size, .create_options = raw_create_options, + + .bdrv_forbid_ext_snapshots = bdrv_allow_ext_snapshots, }; /***********************************************/ @@ -614,6 +616,8 @@ static BlockDriver bdrv_host_device = { .bdrv_getlength = raw_getlength, .bdrv_get_allocated_file_size = raw_get_allocated_file_size, + + .bdrv_forbid_ext_snapshots = bdrv_allow_ext_snapshots, }; static void bdrv_file_init(void) diff --git a/block/raw_bsd.c b/block/raw_bsd.c index d4ace60..2d1af9a 100644 --- a/block/raw_bsd.c +++ b/block/raw_bsd.c @@ -185,7 +185,9 @@ static BlockDriver bdrv_raw = { .bdrv_ioctl = &raw_ioctl, .bdrv_aio_ioctl = &raw_aio_ioctl, .create_options = &raw_create_options[0], - .bdrv_has_zero_init = &raw_has_zero_init + .bdrv_has_zero_init = &raw_has_zero_init, + + .bdrv_forbid_ext_snapshots = bdrv_allow_ext_snapshots, }; static void bdrv_raw_init(void) diff --git a/block/rbd.c b/block/rbd.c index ee1dec5..203d786 100644 --- a/block/rbd.c +++ b/block/rbd.c @@ -1030,6 +1030,8 @@ static BlockDriver bdrv_rbd = { .bdrv_snapshot_delete = qemu_rbd_snap_remove, .bdrv_snapshot_list = qemu_rbd_snap_list, .bdrv_snapshot_goto = qemu_rbd_snap_rollback, + + .bdrv_forbid_ext_snapshots = bdrv_allow_ext_snapshots, }; static void bdrv_rbd_init(void) diff --git a/block/sheepdog.c b/block/sheepdog.c index 5f81c93..2ba2e20 100644 --- a/block/sheepdog.c +++ b/block/sheepdog.c @@ -2361,6 +2361,8 @@ static BlockDriver bdrv_sheepdog = { .bdrv_load_vmstate = sd_load_vmstate, .create_options = sd_create_options, + + .bdrv_forbid_ext_snapshots = bdrv_allow_ext_snapshots, }; static BlockDriver bdrv_sheepdog_tcp = { @@ -2390,6 +2392,8 @@ static BlockDriver bdrv_sheepdog_tcp = { .bdrv_load_vmstate = sd_load_vmstate, .create_options = sd_create_options, + + .bdrv_forbid_ext_snapshots = bdrv_allow_ext_snapshots, }; static BlockDriver bdrv_sheepdog_unix = { @@ -2419,6 +2423,8 @@ static BlockDriver bdrv_sheepdog_unix = { .bdrv_load_vmstate = sd_load_vmstate, .create_options = sd_create_options, + + .bdrv_forbid_ext_snapshots = bdrv_allow_ext_snapshots, }; static void bdrv_sheepdog_init(void) diff --git a/block/ssh.c b/block/ssh.c index aa63c9d..a3c1fe5 100644 --- a/block/ssh.c +++ b/block/ssh.c @@ -1052,6 +1052,8 @@ static BlockDriver bdrv_ssh = { .bdrv_getlength = ssh_getlength, .bdrv_co_flush_to_disk = ssh_co_flush, .create_options = ssh_create_options, + + .bdrv_forbid_ext_snapshots = bdrv_allow_ext_snapshots, }; static void bdrv_ssh_init(void) diff --git a/block/vdi.c b/block/vdi.c index dcbc27c..78a74da 100644 --- a/block/vdi.c +++ b/block/vdi.c @@ -805,6 +805,8 @@ static BlockDriver bdrv_vdi = { .create_options = vdi_create_options, .bdrv_check = vdi_check, + + .bdrv_forbid_ext_snapshots = bdrv_recurse_forbid_ext_snapshots, }; static void bdrv_vdi_init(void) diff --git a/block/vhdx.c b/block/vhdx.c index b8aa49c..ddf3744 100644 --- a/block/vhdx.c +++ b/block/vhdx.c @@ -963,6 +963,8 @@ static BlockDriver bdrv_vhdx = { .bdrv_reopen_prepare = vhdx_reopen_prepare, .bdrv_co_readv = vhdx_co_readv, .bdrv_co_writev = vhdx_co_writev, + + .bdrv_forbid_ext_snapshots = bdrv_recurse_forbid_ext_snapshots, }; static void bdrv_vhdx_init(void) diff --git a/block/vmdk.c b/block/vmdk.c index 5d56e31..78719f0 100644 --- a/block/vmdk.c +++ b/block/vmdk.c @@ -1868,6 +1868,8 @@ static BlockDriver bdrv_vmdk = { .bdrv_has_zero_init = vmdk_has_zero_init, .create_options = vmdk_create_options, + + .bdrv_forbid_ext_snapshots = bdrv_recurse_forbid_ext_snapshots, }; static void bdrv_vmdk_init(void) diff --git a/block/vpc.c b/block/vpc.c index db61274..f7fef97 100644 --- a/block/vpc.c +++ b/block/vpc.c @@ -843,6 +843,8 @@ static BlockDriver bdrv_vpc = { .create_options = vpc_create_options, .bdrv_has_zero_init = vpc_has_zero_init, + + .bdrv_forbid_ext_snapshots = bdrv_recurse_forbid_ext_snapshots, }; static void bdrv_vpc_init(void) diff --git a/block/vvfat.c b/block/vvfat.c index 3ddaa0b..eef207c 100644 --- a/block/vvfat.c +++ b/block/vvfat.c @@ -2993,6 +2993,8 @@ static BlockDriver bdrv_vvfat = { .bdrv_read = vvfat_co_read, .bdrv_write = vvfat_co_write, .bdrv_co_get_block_status = vvfat_co_get_block_status, + + .bdrv_forbid_ext_snapshots = bdrv_allow_ext_snapshots, }; static void bdrv_vvfat_init(void) diff --git a/blockdev.c b/blockdev.c index 8aa66a9..12528cf 100644 --- a/blockdev.c +++ b/blockdev.c @@ -1133,6 +1133,11 @@ static void external_snapshot_prepare(BlkTransactionState *common, flags = state->old_bs->open_flags; + if (bdrv_is_ext_snapshot_forbidden(state->old_bs)) { + error_set(errp, QERR_FEATURE_DISABLED, "snapshot"); + return; + } + /* create new image w/backing file */ if (mode != NEW_IMAGE_MODE_EXISTING) { bdrv_img_create(new_image_file, format, diff --git a/include/block/block.h b/include/block/block.h index f808550..98f1cbb 100644 --- a/include/block/block.h +++ b/include/block/block.h @@ -130,6 +130,16 @@ void bdrv_info(Monitor *mon, QObject **ret_data); void bdrv_stats_print(Monitor *mon, const QObject *data); void bdrv_info_stats(Monitor *mon, QObject **ret_data); +/* used to know if an external snapshot could be taken */ +bool bdrv_is_ext_snapshot_forbidden(BlockDriverState *bs); +/* generics .bdrv_forbid_snapshots implementations */ +/* Used by drivers having a bs->file */ +bool bdrv_recurse_forbid_ext_snapshots(BlockDriverState *bs); +/* Used by drivers where snapshots does not makes sense without block filters */ +bool bdrv_forbid_ext_snapshots(BlockDriverState *bs); +/* Used by regular protocols and bottom of the stack drivers */ +bool bdrv_allow_ext_snapshots(BlockDriverState *bs); + /* disk I/O throttling */ void bdrv_io_limits_enable(BlockDriverState *bs); void bdrv_io_limits_disable(BlockDriverState *bs); diff --git a/include/block/block_int.h b/include/block/block_int.h index 211087a..c41336b 100644 --- a/include/block/block_int.h +++ b/include/block/block_int.h @@ -217,6 +217,13 @@ struct BlockDriver { */ int (*bdrv_has_zero_init)(BlockDriverState *bs); + /* This function return true in BlockDriver for which the extern snapshot + * operation does not make sense. + * Else it's recursively called from the top to the down of the + * BlockDriverState chain. + */ + bool (*bdrv_forbid_ext_snapshots)(BlockDriverState *bs); + QLIST_ENTRY(BlockDriver) list; }; -- 1.8.1.2 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] block: Add bdrv_forbid_ext_snapshots. 2013-09-25 14:23 ` [Qemu-devel] [PATCH] block: Add bdrv_forbid_ext_snapshots Benoît Canet @ 2013-09-26 2:01 ` Jeff Cody 2013-09-26 11:43 ` Kevin Wolf 0 siblings, 1 reply; 6+ messages in thread From: Jeff Cody @ 2013-09-26 2:01 UTC (permalink / raw) To: Benoît Canet; +Cc: kwolf, qemu-devel, stefanha On Wed, Sep 25, 2013 at 04:23:22PM +0200, Benoît Canet wrote: > Drivers having a bs->file where set to recurse the call to their child. > Protocol and drivers designed to be on the bottom of the stack where set to allow > snapshots. > Future protocols like quorum where creating snapshots does not make sense > without block filters will be set to forbid snapshots. > > Signed-off-by: Benoit Canet <benoit@irqsave.net> > --- > block.c | 27 +++++++++++++++++++++++++++ > block/blkdebug.c | 2 ++ > block/blkverify.c | 2 ++ > block/bochs.c | 2 ++ > block/cloop.c | 2 ++ > block/cow.c | 2 ++ > block/curl.c | 10 ++++++++++ > block/dmg.c | 2 ++ > block/gluster.c | 8 ++++++++ > block/iscsi.c | 2 ++ > block/nbd.c | 6 ++++++ > block/parallels.c | 2 ++ > block/qcow.c | 2 ++ > block/qcow2.c | 2 ++ > block/qed.c | 2 ++ > block/raw-posix.c | 10 ++++++++++ > block/raw-win32.c | 4 ++++ > block/raw_bsd.c | 4 +++- > block/rbd.c | 2 ++ > block/sheepdog.c | 6 ++++++ > block/ssh.c | 2 ++ > block/vdi.c | 2 ++ > block/vhdx.c | 2 ++ > block/vmdk.c | 2 ++ > block/vpc.c | 2 ++ > block/vvfat.c | 2 ++ > blockdev.c | 5 +++++ > include/block/block.h | 10 ++++++++++ > include/block/block_int.h | 7 +++++++ > 29 files changed, 132 insertions(+), 1 deletion(-) > > diff --git a/block.c b/block.c > index 4a98250..ff296df 100644 > --- a/block.c > +++ b/block.c > @@ -4651,3 +4651,30 @@ int bdrv_amend_options(BlockDriverState *bs, QEMUOptionParameter *options) > } > return bs->drv->bdrv_amend_options(bs, options); > } > + > +bool bdrv_is_ext_snapshot_forbidden(BlockDriverState *bs) > +{ I think either: A) Name this function bdrv_forbid_ext_snapshots(), or B) Name the BlockDriver function ptr to .bdrv_is_ext_snapshot_forbidden The idea being that this function and the BlockDriver function ptr should have the same name (e.g. bdrv_has_zero_init, and bs->drv->bdrv_has_zero_init, etc..) > + /* allow snapshot by default */ > + if (!bs->drv->bdrv_forbid_ext_snapshots) { > + return false; Rather than return false here, we could do: if (!bs->drv->bdrv_forbid_ext_snapshots) { if (bs->file) { return bdrv_is_ext_snapshot_forbidden(bs->file); } else { return false; } } That would then get rid of the need for bdrv_recurse_forbid_ext_snapshots(), if I understand correctly the goal. And then the only Block Drivers that need to populate .bdrv_is_ext_snapshot_forbidden() are the ones that explicitly forbid it themselves. I.e., the only image driver now that would need to populate this is blkverify (and later on quorum). > + } > + return bs->drv->bdrv_forbid_ext_snapshots(bs); > +} > + > +bool bdrv_recurse_forbid_ext_snapshots(BlockDriverState *bs) > +{ > + if (!bs->file->drv) { > + return false; > + } > + return bs->file->drv->bdrv_forbid_ext_snapshots(bs->file); > +} (As I said above, I don't think this function is needed...) > + > +bool bdrv_forbid_ext_snapshots(BlockDriverState *bs) > +{ > + return true; > +} > + ... and if you chose A above, then maybe just name this function something more like 'bdrv_ext_snapshots_forbidden()', etc. > +bool bdrv_allow_ext_snapshots(BlockDriverState *bs) > +{ > + return false; > +} I understand why this returns false (it is actually 'false' meaning don't forbid external snapshots, as it is mapped to .bdrv_forbid_ext_snapshots()). But it is a bit discordant to have 'bdrv_allow_ext_snapshots()' return false to mean snapshots are allowed. And again, as I said above, I don't think this is needed; if .bdrv_forbid_ext_snapshots is NULL, then the driver is not forbidding snapshots. Thanks, Jeff > diff --git a/block/blkdebug.c b/block/blkdebug.c > index be948b2..5acee0f 100644 > --- a/block/blkdebug.c > +++ b/block/blkdebug.c > @@ -641,6 +641,8 @@ static BlockDriver bdrv_blkdebug = { > .bdrv_debug_breakpoint = blkdebug_debug_breakpoint, > .bdrv_debug_resume = blkdebug_debug_resume, > .bdrv_debug_is_suspended = blkdebug_debug_is_suspended, > + > + .bdrv_forbid_ext_snapshots = bdrv_recurse_forbid_ext_snapshots, > }; > > static void bdrv_blkdebug_init(void) > diff --git a/block/blkverify.c b/block/blkverify.c > index 2077d8a..7890adb 100644 > --- a/block/blkverify.c > +++ b/block/blkverify.c > @@ -313,6 +313,8 @@ static BlockDriver bdrv_blkverify = { > .bdrv_aio_readv = blkverify_aio_readv, > .bdrv_aio_writev = blkverify_aio_writev, > .bdrv_aio_flush = blkverify_aio_flush, > + > + .bdrv_forbid_ext_snapshots = bdrv_forbid_ext_snapshots, > }; > > static void bdrv_blkverify_init(void) > diff --git a/block/bochs.c b/block/bochs.c > index 51d9a90..55c13c5 100644 > --- a/block/bochs.c > +++ b/block/bochs.c > @@ -242,6 +242,8 @@ static BlockDriver bdrv_bochs = { > .bdrv_open = bochs_open, > .bdrv_read = bochs_co_read, > .bdrv_close = bochs_close, > + > + .bdrv_forbid_ext_snapshots = bdrv_recurse_forbid_ext_snapshots, > }; > > static void bdrv_bochs_init(void) > diff --git a/block/cloop.c b/block/cloop.c > index b907023..bb28758 100644 > --- a/block/cloop.c > +++ b/block/cloop.c > @@ -195,6 +195,8 @@ static BlockDriver bdrv_cloop = { > .bdrv_open = cloop_open, > .bdrv_read = cloop_co_read, > .bdrv_close = cloop_close, > + > + .bdrv_forbid_ext_snapshots = bdrv_recurse_forbid_ext_snapshots, > }; > > static void bdrv_cloop_init(void) > diff --git a/block/cow.c b/block/cow.c > index 909c3e7..ceb786a 100644 > --- a/block/cow.c > +++ b/block/cow.c > @@ -393,6 +393,8 @@ static BlockDriver bdrv_cow = { > .bdrv_co_get_block_status = cow_co_get_block_status, > > .create_options = cow_create_options, > + > + .bdrv_forbid_ext_snapshots = bdrv_recurse_forbid_ext_snapshots, > }; > > static void bdrv_cow_init(void) > diff --git a/block/curl.c b/block/curl.c > index 5a46f97..ac3c94a 100644 > --- a/block/curl.c > +++ b/block/curl.c > @@ -617,6 +617,8 @@ static BlockDriver bdrv_http = { > .bdrv_getlength = curl_getlength, > > .bdrv_aio_readv = curl_aio_readv, > + > + .bdrv_forbid_ext_snapshots = bdrv_allow_ext_snapshots, > }; > > static BlockDriver bdrv_https = { > @@ -630,6 +632,8 @@ static BlockDriver bdrv_https = { > .bdrv_getlength = curl_getlength, > > .bdrv_aio_readv = curl_aio_readv, > + > + .bdrv_forbid_ext_snapshots = bdrv_allow_ext_snapshots, > }; > > static BlockDriver bdrv_ftp = { > @@ -643,6 +647,8 @@ static BlockDriver bdrv_ftp = { > .bdrv_getlength = curl_getlength, > > .bdrv_aio_readv = curl_aio_readv, > + > + .bdrv_forbid_ext_snapshots = bdrv_allow_ext_snapshots, > }; > > static BlockDriver bdrv_ftps = { > @@ -656,6 +662,8 @@ static BlockDriver bdrv_ftps = { > .bdrv_getlength = curl_getlength, > > .bdrv_aio_readv = curl_aio_readv, > + > + .bdrv_forbid_ext_snapshots = bdrv_allow_ext_snapshots, > }; > > static BlockDriver bdrv_tftp = { > @@ -669,6 +677,8 @@ static BlockDriver bdrv_tftp = { > .bdrv_getlength = curl_getlength, > > .bdrv_aio_readv = curl_aio_readv, > + > + .bdrv_forbid_ext_snapshots = bdrv_allow_ext_snapshots, > }; > > static void curl_block_init(void) > diff --git a/block/dmg.c b/block/dmg.c > index d5e9b1f..96e7103 100644 > --- a/block/dmg.c > +++ b/block/dmg.c > @@ -382,6 +382,8 @@ static BlockDriver bdrv_dmg = { > .bdrv_open = dmg_open, > .bdrv_read = dmg_co_read, > .bdrv_close = dmg_close, > + > + .bdrv_forbid_ext_snapshots = bdrv_recurse_forbid_ext_snapshots, > }; > > static void bdrv_dmg_init(void) > diff --git a/block/gluster.c b/block/gluster.c > index 877686a..dfb7997 100644 > --- a/block/gluster.c > +++ b/block/gluster.c > @@ -626,6 +626,8 @@ static BlockDriver bdrv_gluster = { > .bdrv_aio_discard = qemu_gluster_aio_discard, > #endif > .create_options = qemu_gluster_create_options, > + > + .bdrv_forbid_ext_snapshots = bdrv_allow_ext_snapshots, > }; > > static BlockDriver bdrv_gluster_tcp = { > @@ -647,6 +649,8 @@ static BlockDriver bdrv_gluster_tcp = { > .bdrv_aio_discard = qemu_gluster_aio_discard, > #endif > .create_options = qemu_gluster_create_options, > + > + .bdrv_forbid_ext_snapshots = bdrv_allow_ext_snapshots, > }; > > static BlockDriver bdrv_gluster_unix = { > @@ -668,6 +672,8 @@ static BlockDriver bdrv_gluster_unix = { > .bdrv_aio_discard = qemu_gluster_aio_discard, > #endif > .create_options = qemu_gluster_create_options, > + > + .bdrv_forbid_ext_snapshots = bdrv_allow_ext_snapshots, > }; > > static BlockDriver bdrv_gluster_rdma = { > @@ -689,6 +695,8 @@ static BlockDriver bdrv_gluster_rdma = { > .bdrv_aio_discard = qemu_gluster_aio_discard, > #endif > .create_options = qemu_gluster_create_options, > + > + .bdrv_forbid_ext_snapshots = bdrv_allow_ext_snapshots, > }; > > static void bdrv_gluster_init(void) > diff --git a/block/iscsi.c b/block/iscsi.c > index 6152ef1..4cc7ce0 100644 > --- a/block/iscsi.c > +++ b/block/iscsi.c > @@ -1544,6 +1544,8 @@ static BlockDriver bdrv_iscsi = { > .bdrv_ioctl = iscsi_ioctl, > .bdrv_aio_ioctl = iscsi_aio_ioctl, > #endif > + > + .bdrv_forbid_ext_snapshots = bdrv_allow_ext_snapshots, > }; > > static QemuOptsList qemu_iscsi_opts = { > diff --git a/block/nbd.c b/block/nbd.c > index c8deeee..7eac042 100644 > --- a/block/nbd.c > +++ b/block/nbd.c > @@ -651,6 +651,8 @@ static BlockDriver bdrv_nbd = { > .bdrv_co_flush_to_os = nbd_co_flush, > .bdrv_co_discard = nbd_co_discard, > .bdrv_getlength = nbd_getlength, > + > + .bdrv_forbid_ext_snapshots = bdrv_allow_ext_snapshots, > }; > > static BlockDriver bdrv_nbd_tcp = { > @@ -665,6 +667,8 @@ static BlockDriver bdrv_nbd_tcp = { > .bdrv_co_flush_to_os = nbd_co_flush, > .bdrv_co_discard = nbd_co_discard, > .bdrv_getlength = nbd_getlength, > + > + .bdrv_forbid_ext_snapshots = bdrv_allow_ext_snapshots, > }; > > static BlockDriver bdrv_nbd_unix = { > @@ -679,6 +683,8 @@ static BlockDriver bdrv_nbd_unix = { > .bdrv_co_flush_to_os = nbd_co_flush, > .bdrv_co_discard = nbd_co_discard, > .bdrv_getlength = nbd_getlength, > + > + .bdrv_forbid_ext_snapshots = bdrv_allow_ext_snapshots, > }; > > static void bdrv_nbd_init(void) > diff --git a/block/parallels.c b/block/parallels.c > index 2121e43..01ee036 100644 > --- a/block/parallels.c > +++ b/block/parallels.c > @@ -168,6 +168,8 @@ static BlockDriver bdrv_parallels = { > .bdrv_open = parallels_open, > .bdrv_read = parallels_co_read, > .bdrv_close = parallels_close, > + > + .bdrv_forbid_ext_snapshots = bdrv_recurse_forbid_ext_snapshots, > }; > > static void bdrv_parallels_init(void) > diff --git a/block/qcow.c b/block/qcow.c > index c470e05..47f3b44 100644 > --- a/block/qcow.c > +++ b/block/qcow.c > @@ -918,6 +918,8 @@ static BlockDriver bdrv_qcow = { > .bdrv_get_info = qcow_get_info, > > .create_options = qcow_create_options, > + > + .bdrv_forbid_ext_snapshots = bdrv_recurse_forbid_ext_snapshots, > }; > > static void bdrv_qcow_init(void) > diff --git a/block/qcow2.c b/block/qcow2.c > index 318d95d..c560133b 100644 > --- a/block/qcow2.c > +++ b/block/qcow2.c > @@ -2141,6 +2141,8 @@ static BlockDriver bdrv_qcow2 = { > .create_options = qcow2_create_options, > .bdrv_check = qcow2_check, > .bdrv_amend_options = qcow2_amend_options, > + > + .bdrv_forbid_ext_snapshots = bdrv_recurse_forbid_ext_snapshots, > }; > > static void bdrv_qcow2_init(void) > diff --git a/block/qed.c b/block/qed.c > index 6c0cba0..e7aaa98 100644 > --- a/block/qed.c > +++ b/block/qed.c > @@ -1615,6 +1615,8 @@ static BlockDriver bdrv_qed = { > .bdrv_change_backing_file = bdrv_qed_change_backing_file, > .bdrv_invalidate_cache = bdrv_qed_invalidate_cache, > .bdrv_check = bdrv_qed_check, > + > + .bdrv_forbid_ext_snapshots = bdrv_recurse_forbid_ext_snapshots, > }; > > static void bdrv_qed_init(void) > diff --git a/block/raw-posix.c b/block/raw-posix.c > index f7f102d..ab2a64b 100644 > --- a/block/raw-posix.c > +++ b/block/raw-posix.c > @@ -1222,6 +1222,8 @@ static BlockDriver bdrv_file = { > = raw_get_allocated_file_size, > > .create_options = raw_create_options, > + > + .bdrv_forbid_ext_snapshots = bdrv_allow_ext_snapshots, > }; > > /***********************************************/ > @@ -1568,6 +1570,8 @@ static BlockDriver bdrv_host_device = { > .bdrv_ioctl = hdev_ioctl, > .bdrv_aio_ioctl = hdev_aio_ioctl, > #endif > + > + .bdrv_forbid_ext_snapshots = bdrv_allow_ext_snapshots, > }; > > #ifdef __linux__ > @@ -1692,6 +1696,8 @@ static BlockDriver bdrv_host_floppy = { > .bdrv_is_inserted = floppy_is_inserted, > .bdrv_media_changed = floppy_media_changed, > .bdrv_eject = floppy_eject, > + > + .bdrv_forbid_ext_snapshots = bdrv_allow_ext_snapshots, > }; > > static int cdrom_open(BlockDriverState *bs, QDict *options, int flags, > @@ -1799,6 +1805,8 @@ static BlockDriver bdrv_host_cdrom = { > /* generic scsi device */ > .bdrv_ioctl = hdev_ioctl, > .bdrv_aio_ioctl = hdev_aio_ioctl, > + > + .bdrv_forbid_ext_snapshots = bdrv_allow_ext_snapshots, > }; > #endif /* __linux__ */ > > @@ -1917,6 +1925,8 @@ static BlockDriver bdrv_host_cdrom = { > .bdrv_is_inserted = cdrom_is_inserted, > .bdrv_eject = cdrom_eject, > .bdrv_lock_medium = cdrom_lock_medium, > + > + .bdrv_forbid_ext_snapshots = bdrv_allow_ext_snapshots, > }; > #endif /* __FreeBSD__ */ > > diff --git a/block/raw-win32.c b/block/raw-win32.c > index 6ef320f..3479881 100644 > --- a/block/raw-win32.c > +++ b/block/raw-win32.c > @@ -475,6 +475,8 @@ static BlockDriver bdrv_file = { > = raw_get_allocated_file_size, > > .create_options = raw_create_options, > + > + .bdrv_forbid_ext_snapshots = bdrv_allow_ext_snapshots, > }; > > /***********************************************/ > @@ -614,6 +616,8 @@ static BlockDriver bdrv_host_device = { > .bdrv_getlength = raw_getlength, > .bdrv_get_allocated_file_size > = raw_get_allocated_file_size, > + > + .bdrv_forbid_ext_snapshots = bdrv_allow_ext_snapshots, > }; > > static void bdrv_file_init(void) > diff --git a/block/raw_bsd.c b/block/raw_bsd.c > index d4ace60..2d1af9a 100644 > --- a/block/raw_bsd.c > +++ b/block/raw_bsd.c > @@ -185,7 +185,9 @@ static BlockDriver bdrv_raw = { > .bdrv_ioctl = &raw_ioctl, > .bdrv_aio_ioctl = &raw_aio_ioctl, > .create_options = &raw_create_options[0], > - .bdrv_has_zero_init = &raw_has_zero_init > + .bdrv_has_zero_init = &raw_has_zero_init, > + > + .bdrv_forbid_ext_snapshots = bdrv_allow_ext_snapshots, > }; > > static void bdrv_raw_init(void) > diff --git a/block/rbd.c b/block/rbd.c > index ee1dec5..203d786 100644 > --- a/block/rbd.c > +++ b/block/rbd.c > @@ -1030,6 +1030,8 @@ static BlockDriver bdrv_rbd = { > .bdrv_snapshot_delete = qemu_rbd_snap_remove, > .bdrv_snapshot_list = qemu_rbd_snap_list, > .bdrv_snapshot_goto = qemu_rbd_snap_rollback, > + > + .bdrv_forbid_ext_snapshots = bdrv_allow_ext_snapshots, > }; > > static void bdrv_rbd_init(void) > diff --git a/block/sheepdog.c b/block/sheepdog.c > index 5f81c93..2ba2e20 100644 > --- a/block/sheepdog.c > +++ b/block/sheepdog.c > @@ -2361,6 +2361,8 @@ static BlockDriver bdrv_sheepdog = { > .bdrv_load_vmstate = sd_load_vmstate, > > .create_options = sd_create_options, > + > + .bdrv_forbid_ext_snapshots = bdrv_allow_ext_snapshots, > }; > > static BlockDriver bdrv_sheepdog_tcp = { > @@ -2390,6 +2392,8 @@ static BlockDriver bdrv_sheepdog_tcp = { > .bdrv_load_vmstate = sd_load_vmstate, > > .create_options = sd_create_options, > + > + .bdrv_forbid_ext_snapshots = bdrv_allow_ext_snapshots, > }; > > static BlockDriver bdrv_sheepdog_unix = { > @@ -2419,6 +2423,8 @@ static BlockDriver bdrv_sheepdog_unix = { > .bdrv_load_vmstate = sd_load_vmstate, > > .create_options = sd_create_options, > + > + .bdrv_forbid_ext_snapshots = bdrv_allow_ext_snapshots, > }; > > static void bdrv_sheepdog_init(void) > diff --git a/block/ssh.c b/block/ssh.c > index aa63c9d..a3c1fe5 100644 > --- a/block/ssh.c > +++ b/block/ssh.c > @@ -1052,6 +1052,8 @@ static BlockDriver bdrv_ssh = { > .bdrv_getlength = ssh_getlength, > .bdrv_co_flush_to_disk = ssh_co_flush, > .create_options = ssh_create_options, > + > + .bdrv_forbid_ext_snapshots = bdrv_allow_ext_snapshots, > }; > > static void bdrv_ssh_init(void) > diff --git a/block/vdi.c b/block/vdi.c > index dcbc27c..78a74da 100644 > --- a/block/vdi.c > +++ b/block/vdi.c > @@ -805,6 +805,8 @@ static BlockDriver bdrv_vdi = { > > .create_options = vdi_create_options, > .bdrv_check = vdi_check, > + > + .bdrv_forbid_ext_snapshots = bdrv_recurse_forbid_ext_snapshots, > }; > > static void bdrv_vdi_init(void) > diff --git a/block/vhdx.c b/block/vhdx.c > index b8aa49c..ddf3744 100644 > --- a/block/vhdx.c > +++ b/block/vhdx.c > @@ -963,6 +963,8 @@ static BlockDriver bdrv_vhdx = { > .bdrv_reopen_prepare = vhdx_reopen_prepare, > .bdrv_co_readv = vhdx_co_readv, > .bdrv_co_writev = vhdx_co_writev, > + > + .bdrv_forbid_ext_snapshots = bdrv_recurse_forbid_ext_snapshots, > }; > > static void bdrv_vhdx_init(void) > diff --git a/block/vmdk.c b/block/vmdk.c > index 5d56e31..78719f0 100644 > --- a/block/vmdk.c > +++ b/block/vmdk.c > @@ -1868,6 +1868,8 @@ static BlockDriver bdrv_vmdk = { > .bdrv_has_zero_init = vmdk_has_zero_init, > > .create_options = vmdk_create_options, > + > + .bdrv_forbid_ext_snapshots = bdrv_recurse_forbid_ext_snapshots, > }; > > static void bdrv_vmdk_init(void) > diff --git a/block/vpc.c b/block/vpc.c > index db61274..f7fef97 100644 > --- a/block/vpc.c > +++ b/block/vpc.c > @@ -843,6 +843,8 @@ static BlockDriver bdrv_vpc = { > > .create_options = vpc_create_options, > .bdrv_has_zero_init = vpc_has_zero_init, > + > + .bdrv_forbid_ext_snapshots = bdrv_recurse_forbid_ext_snapshots, > }; > > static void bdrv_vpc_init(void) > diff --git a/block/vvfat.c b/block/vvfat.c > index 3ddaa0b..eef207c 100644 > --- a/block/vvfat.c > +++ b/block/vvfat.c > @@ -2993,6 +2993,8 @@ static BlockDriver bdrv_vvfat = { > .bdrv_read = vvfat_co_read, > .bdrv_write = vvfat_co_write, > .bdrv_co_get_block_status = vvfat_co_get_block_status, > + > + .bdrv_forbid_ext_snapshots = bdrv_allow_ext_snapshots, > }; > > static void bdrv_vvfat_init(void) > diff --git a/blockdev.c b/blockdev.c > index 8aa66a9..12528cf 100644 > --- a/blockdev.c > +++ b/blockdev.c > @@ -1133,6 +1133,11 @@ static void external_snapshot_prepare(BlkTransactionState *common, > > flags = state->old_bs->open_flags; > > + if (bdrv_is_ext_snapshot_forbidden(state->old_bs)) { > + error_set(errp, QERR_FEATURE_DISABLED, "snapshot"); > + return; > + } > + > /* create new image w/backing file */ > if (mode != NEW_IMAGE_MODE_EXISTING) { > bdrv_img_create(new_image_file, format, > diff --git a/include/block/block.h b/include/block/block.h > index f808550..98f1cbb 100644 > --- a/include/block/block.h > +++ b/include/block/block.h > @@ -130,6 +130,16 @@ void bdrv_info(Monitor *mon, QObject **ret_data); > void bdrv_stats_print(Monitor *mon, const QObject *data); > void bdrv_info_stats(Monitor *mon, QObject **ret_data); > > +/* used to know if an external snapshot could be taken */ > +bool bdrv_is_ext_snapshot_forbidden(BlockDriverState *bs); > +/* generics .bdrv_forbid_snapshots implementations */ > +/* Used by drivers having a bs->file */ > +bool bdrv_recurse_forbid_ext_snapshots(BlockDriverState *bs); > +/* Used by drivers where snapshots does not makes sense without block filters */ > +bool bdrv_forbid_ext_snapshots(BlockDriverState *bs); > +/* Used by regular protocols and bottom of the stack drivers */ > +bool bdrv_allow_ext_snapshots(BlockDriverState *bs); > + > /* disk I/O throttling */ > void bdrv_io_limits_enable(BlockDriverState *bs); > void bdrv_io_limits_disable(BlockDriverState *bs); > diff --git a/include/block/block_int.h b/include/block/block_int.h > index 211087a..c41336b 100644 > --- a/include/block/block_int.h > +++ b/include/block/block_int.h > @@ -217,6 +217,13 @@ struct BlockDriver { > */ > int (*bdrv_has_zero_init)(BlockDriverState *bs); > > + /* This function return true in BlockDriver for which the extern snapshot > + * operation does not make sense. > + * Else it's recursively called from the top to the down of the > + * BlockDriverState chain. > + */ > + bool (*bdrv_forbid_ext_snapshots)(BlockDriverState *bs); > + > QLIST_ENTRY(BlockDriver) list; > }; > > -- > 1.8.1.2 > > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] block: Add bdrv_forbid_ext_snapshots. 2013-09-26 2:01 ` Jeff Cody @ 2013-09-26 11:43 ` Kevin Wolf 2013-09-26 13:35 ` Benoît Canet 0 siblings, 1 reply; 6+ messages in thread From: Kevin Wolf @ 2013-09-26 11:43 UTC (permalink / raw) To: Jeff Cody; +Cc: Benoît Canet, stefanha, qemu-devel Am 26.09.2013 um 04:01 hat Jeff Cody geschrieben: > On Wed, Sep 25, 2013 at 04:23:22PM +0200, Benoît Canet wrote: > > Drivers having a bs->file where set to recurse the call to their child. > > Protocol and drivers designed to be on the bottom of the stack where set to allow > > snapshots. > > Future protocols like quorum where creating snapshots does not make sense > > without block filters will be set to forbid snapshots. > > > > Signed-off-by: Benoit Canet <benoit@irqsave.net> > > diff --git a/block.c b/block.c > > index 4a98250..ff296df 100644 > > --- a/block.c > > +++ b/block.c > > @@ -4651,3 +4651,30 @@ int bdrv_amend_options(BlockDriverState *bs, QEMUOptionParameter *options) > > } > > return bs->drv->bdrv_amend_options(bs, options); > > } > > + > > +bool bdrv_is_ext_snapshot_forbidden(BlockDriverState *bs) > > +{ > > I think either: > A) Name this function bdrv_forbid_ext_snapshots(), or > B) Name the BlockDriver function ptr to .bdrv_is_ext_snapshot_forbidden > > The idea being that this function and the BlockDriver function ptr > should have the same name (e.g. bdrv_has_zero_init, and > bs->drv->bdrv_has_zero_init, etc..) Yes, I agree, some consistent naming is desirable. I don't think bdrv_forbid_ext_snapshots() is a good name, because it implies that calling this function is what forbids the snapshot (i.e. an action similar to adding a migration blocker), whereas in fact it just checks whether snapshots are forbidden. How about bdrv_ext_snapshot_allowed(), which avoid double negations when we check for "not forbidden"? Or perhaps even bdrv_check_ext_snapshot(), which would be a more generic name that could be extended to the three-way distinction we intended to have in the end: - External snapshots are forbidden - May snapshot, but below this BDS (ask bs->file; this is for filters) - Do the snapshot here Kevin ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] block: Add bdrv_forbid_ext_snapshots. 2013-09-26 11:43 ` Kevin Wolf @ 2013-09-26 13:35 ` Benoît Canet 2013-09-26 14:05 ` Kevin Wolf 0 siblings, 1 reply; 6+ messages in thread From: Benoît Canet @ 2013-09-26 13:35 UTC (permalink / raw) To: Kevin Wolf; +Cc: Jeff Cody, qemu-devel, stefanha Le Thursday 26 Sep 2013 à 13:43:19 (+0200), Kevin Wolf a écrit : > Am 26.09.2013 um 04:01 hat Jeff Cody geschrieben: > > On Wed, Sep 25, 2013 at 04:23:22PM +0200, Benoît Canet wrote: > > > Drivers having a bs->file where set to recurse the call to their child. > > > Protocol and drivers designed to be on the bottom of the stack where set to allow > > > snapshots. > > > Future protocols like quorum where creating snapshots does not make sense > > > without block filters will be set to forbid snapshots. > > > > > > Signed-off-by: Benoit Canet <benoit@irqsave.net> > > > > diff --git a/block.c b/block.c > > > index 4a98250..ff296df 100644 > > > --- a/block.c > > > +++ b/block.c > > > @@ -4651,3 +4651,30 @@ int bdrv_amend_options(BlockDriverState *bs, QEMUOptionParameter *options) > > > } > > > return bs->drv->bdrv_amend_options(bs, options); > > > } > > > + > > > +bool bdrv_is_ext_snapshot_forbidden(BlockDriverState *bs) > > > +{ > > > > I think either: > > A) Name this function bdrv_forbid_ext_snapshots(), or > > B) Name the BlockDriver function ptr to .bdrv_is_ext_snapshot_forbidden > > > > The idea being that this function and the BlockDriver function ptr > > should have the same name (e.g. bdrv_has_zero_init, and > > bs->drv->bdrv_has_zero_init, etc..) > > Yes, I agree, some consistent naming is desirable. I don't think > bdrv_forbid_ext_snapshots() is a good name, because it implies that > calling this function is what forbids the snapshot (i.e. an action > similar to adding a migration blocker), whereas in fact it just checks > whether snapshots are forbidden. > > How about bdrv_ext_snapshot_allowed(), which avoid double negations when > we check for "not forbidden"? Or perhaps even bdrv_check_ext_snapshot(), > which would be a more generic name that could be extended to the > three-way distinction we intended to have in the end: > > - External snapshots are forbidden > - May snapshot, but below this BDS (ask bs->file; this is for filters) > - Do the snapshot here Whould .bdrv_check_ext_snapshot being NULL imply "- Do the snapshot here" as Jeff suggested ? Best regards Benoît > > Kevin ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] block: Add bdrv_forbid_ext_snapshots. 2013-09-26 13:35 ` Benoît Canet @ 2013-09-26 14:05 ` Kevin Wolf 0 siblings, 0 replies; 6+ messages in thread From: Kevin Wolf @ 2013-09-26 14:05 UTC (permalink / raw) To: Benoît Canet; +Cc: Jeff Cody, qemu-devel, stefanha Am 26.09.2013 um 15:35 hat Benoît Canet geschrieben: > Le Thursday 26 Sep 2013 à 13:43:19 (+0200), Kevin Wolf a écrit : > > Am 26.09.2013 um 04:01 hat Jeff Cody geschrieben: > > > On Wed, Sep 25, 2013 at 04:23:22PM +0200, Benoît Canet wrote: > > > > Drivers having a bs->file where set to recurse the call to their child. > > > > Protocol and drivers designed to be on the bottom of the stack where set to allow > > > > snapshots. > > > > Future protocols like quorum where creating snapshots does not make sense > > > > without block filters will be set to forbid snapshots. > > > > > > > > Signed-off-by: Benoit Canet <benoit@irqsave.net> > > > > > > diff --git a/block.c b/block.c > > > > index 4a98250..ff296df 100644 > > > > --- a/block.c > > > > +++ b/block.c > > > > @@ -4651,3 +4651,30 @@ int bdrv_amend_options(BlockDriverState *bs, QEMUOptionParameter *options) > > > > } > > > > return bs->drv->bdrv_amend_options(bs, options); > > > > } > > > > + > > > > +bool bdrv_is_ext_snapshot_forbidden(BlockDriverState *bs) > > > > +{ > > > > > > I think either: > > > A) Name this function bdrv_forbid_ext_snapshots(), or > > > B) Name the BlockDriver function ptr to .bdrv_is_ext_snapshot_forbidden > > > > > > The idea being that this function and the BlockDriver function ptr > > > should have the same name (e.g. bdrv_has_zero_init, and > > > bs->drv->bdrv_has_zero_init, etc..) > > > > Yes, I agree, some consistent naming is desirable. I don't think > > bdrv_forbid_ext_snapshots() is a good name, because it implies that > > calling this function is what forbids the snapshot (i.e. an action > > similar to adding a migration blocker), whereas in fact it just checks > > whether snapshots are forbidden. > > > > How about bdrv_ext_snapshot_allowed(), which avoid double negations when > > we check for "not forbidden"? Or perhaps even bdrv_check_ext_snapshot(), > > which would be a more generic name that could be extended to the > > three-way distinction we intended to have in the end: > > > > - External snapshots are forbidden > > - May snapshot, but below this BDS (ask bs->file; this is for filters) > > - Do the snapshot here > > Whould .bdrv_check_ext_snapshot being NULL imply "- Do the snapshot here" as > Jeff suggested ? That would probably be the most convenient option. Kevin ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2013-09-26 14:06 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-09-25 14:23 [Qemu-devel] [PATCH] Add infrastructure to forbid external snapshot Benoît Canet 2013-09-25 14:23 ` [Qemu-devel] [PATCH] block: Add bdrv_forbid_ext_snapshots Benoît Canet 2013-09-26 2:01 ` Jeff Cody 2013-09-26 11:43 ` Kevin Wolf 2013-09-26 13:35 ` Benoît Canet 2013-09-26 14:05 ` Kevin Wolf
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).