* [Qemu-devel] [PATCH V4 0/4] Implement sync modes for drive-backup. @ 2013-07-17 20:04 Ian Main 2013-07-17 20:04 ` [Qemu-devel] [PATCH V4 1/4] " Ian Main ` (3 more replies) 0 siblings, 4 replies; 18+ messages in thread From: Ian Main @ 2013-07-17 20:04 UTC (permalink / raw) To: qemu-devel; +Cc: Ian Main This patch adds sync modes on top of the work that Stefan Hajnoczi has done. These patches apply on kevin/block with '[PATCH] block: add drive_backup HMP command' also applied. Hopefully all is in order as this is my first QEMU patch. Many thanks to Stephan and Fam Zheng for their help. V2: - No longer poll, instead use qemu_coroutine_yield(). - Use bdrv_co_is_allocated(). - Much better SYNC_MODE_NONE test. V3: - A few style fixes. - Better commit message explaining how TOP and NONE operate. - Verified using checkpatch.pl. V4: - Add patch to use the source as a backing hd during backup. - Add patch to default sync mode none to qcow2. Ian Main (4): Implement sync modes for drive-backup. Add tests for sync modes 'TOP' and 'NONE' Add backing drive while performing backup. Change default to qcow2 for sync mode none. block/backup.c | 104 ++++++++++++++++++++++++++++++------------ blockdev.c | 27 +++++++---- include/block/block_int.h | 4 +- qapi-schema.json | 4 ++ qmp-commands.hx | 1 + tests/qemu-iotests/055 | 67 +++++++++++++++++++++++++-- tests/qemu-iotests/055.out | 4 +- tests/qemu-iotests/group | 2 +- tests/qemu-iotests/iotests.py | 5 ++ 9 files changed, 171 insertions(+), 47 deletions(-) -- 1.8.1.4 ^ permalink raw reply [flat|nested] 18+ messages in thread
* [Qemu-devel] [PATCH V4 1/4] Implement sync modes for drive-backup. 2013-07-17 20:04 [Qemu-devel] [PATCH V4 0/4] Implement sync modes for drive-backup Ian Main @ 2013-07-17 20:04 ` Ian Main 2013-07-18 17:19 ` Eric Blake 2013-07-17 20:04 ` [Qemu-devel] [PATCH V4 2/4] Add tests for sync modes 'TOP' and 'NONE' Ian Main ` (2 subsequent siblings) 3 siblings, 1 reply; 18+ messages in thread From: Ian Main @ 2013-07-17 20:04 UTC (permalink / raw) To: qemu-devel; +Cc: Ian Main This patch adds sync-modes to the drive-backup interface and implements the FULL, NONE and TOP modes of synchronization. FULL performs as before copying the entire contents of the drive while preserving the point-in-time using CoW. NONE only copies new writes to the target drive. TOP copies changes to the topmost drive image and preserves the point-in-time using CoW. For sync mode TOP are creating a new target image using the same backing file as the original disk image. Then any new data that has been laid on top of it since creation is copied in the main backup_run() loop. There is an extra check in the 'TOP' case so that we don't bother to copy all the data of the backing file as it already exists in the target. This is where the bdrv_co_is_allocated() is used to determine if the data exists in the topmost layer or below. Also any new data being written is intercepted via the write_notifier hook which ends up calling backup_do_cow() to copy old data out before it gets overwritten. For mode 'NONE' we create the new target image and only copy in the original data from the disk image starting from the time the call was made. This preserves the point in time data by only copying the parts that are *going to change* to the target image. This way we can reconstruct the final image by checking to see if the given block exists in the new target image first, and if it does not, you can get it from the original image. This is basically an optimization allowing you to do point-in-time snapshots with low overhead vs the 'FULL' version. Since there is no old data to copy out the loop in backup_run() for the NONE case just calls qemu_coroutine_yield() which only wakes up after an event (usually cancel in this case). The rest is handled by the before_write notifier which again calls backup_do_cow() to write out the old data so it can be preserved. Signed-off-by: Ian Main <imain@redhat.com> --- block/backup.c | 91 +++++++++++++++++++++++++++++++---------------- blockdev.c | 25 ++++++++----- include/block/block_int.h | 4 ++- qapi-schema.json | 4 +++ qmp-commands.hx | 1 + 5 files changed, 86 insertions(+), 39 deletions(-) diff --git a/block/backup.c b/block/backup.c index 16105d4..68abd23 100644 --- a/block/backup.c +++ b/block/backup.c @@ -37,6 +37,7 @@ typedef struct CowRequest { typedef struct BackupBlockJob { BlockJob common; BlockDriverState *target; + MirrorSyncMode sync_mode; RateLimit limit; BlockdevOnError on_source_error; BlockdevOnError on_target_error; @@ -247,40 +248,69 @@ static void coroutine_fn backup_run(void *opaque) bdrv_add_before_write_notifier(bs, &before_write); - for (; start < end; start++) { - bool error_is_read; - - if (block_job_is_cancelled(&job->common)) { - break; + if (job->sync_mode == MIRROR_SYNC_MODE_NONE) { + while (!block_job_is_cancelled(&job->common)) { + /* Yield until the job is cancelled. We just let our before_write + * notify callback service CoW requests. */ + job->common.busy = false; + qemu_coroutine_yield(); + job->common.busy = true; } + } else { + /* Both FULL and TOP SYNC_MODE's require copying.. */ + for (; start < end; start++) { + bool error_is_read; - /* we need to yield so that qemu_aio_flush() returns. - * (without, VM does not reboot) - */ - if (job->common.speed) { - uint64_t delay_ns = ratelimit_calculate_delay( - &job->limit, job->sectors_read); - job->sectors_read = 0; - block_job_sleep_ns(&job->common, rt_clock, delay_ns); - } else { - block_job_sleep_ns(&job->common, rt_clock, 0); - } + if (block_job_is_cancelled(&job->common)) { + break; + } - if (block_job_is_cancelled(&job->common)) { - break; - } + /* we need to yield so that qemu_aio_flush() returns. + * (without, VM does not reboot) + */ + if (job->common.speed) { + uint64_t delay_ns = ratelimit_calculate_delay( + &job->limit, job->sectors_read); + job->sectors_read = 0; + block_job_sleep_ns(&job->common, rt_clock, delay_ns); + } else { + block_job_sleep_ns(&job->common, rt_clock, 0); + } - ret = backup_do_cow(bs, start * BACKUP_SECTORS_PER_CLUSTER, - BACKUP_SECTORS_PER_CLUSTER, &error_is_read); - if (ret < 0) { - /* Depending on error action, fail now or retry cluster */ - BlockErrorAction action = - backup_error_action(job, error_is_read, -ret); - if (action == BDRV_ACTION_REPORT) { + if (block_job_is_cancelled(&job->common)) { break; - } else { - start--; - continue; + } + + if (job->sync_mode == MIRROR_SYNC_MODE_TOP) { + int n, alloced; + + /* Check to see if these blocks are already in the + * backing file. */ + + alloced = + bdrv_co_is_allocated(bs, start * BACKUP_SECTORS_PER_CLUSTER, + BACKUP_SECTORS_PER_CLUSTER, &n); + /* The above call returns true if the given sector is in the + * topmost image. If that is the case then we must copy it as + * it has been modified from the original backing file. + * Otherwise we skip it. */ + if (alloced == 0) { + continue; + } + } + /* FULL sync mode we copy the whole drive. */ + ret = backup_do_cow(bs, start * BACKUP_SECTORS_PER_CLUSTER, + BACKUP_SECTORS_PER_CLUSTER, &error_is_read); + if (ret < 0) { + /* Depending on error action, fail now or retry cluster */ + BlockErrorAction action = + backup_error_action(job, error_is_read, -ret); + if (action == BDRV_ACTION_REPORT) { + break; + } else { + start--; + continue; + } } } } @@ -300,7 +330,7 @@ static void coroutine_fn backup_run(void *opaque) } void backup_start(BlockDriverState *bs, BlockDriverState *target, - int64_t speed, + int64_t speed, MirrorSyncMode sync_mode, BlockdevOnError on_source_error, BlockdevOnError on_target_error, BlockDriverCompletionFunc *cb, void *opaque, @@ -335,6 +365,7 @@ void backup_start(BlockDriverState *bs, BlockDriverState *target, job->on_source_error = on_source_error; job->on_target_error = on_target_error; job->target = target; + job->sync_mode = sync_mode; job->common.len = len; job->common.co = qemu_coroutine_create(backup_run); qemu_coroutine_enter(job->common.co, job); diff --git a/blockdev.c b/blockdev.c index c5abd65..000dea6 100644 --- a/blockdev.c +++ b/blockdev.c @@ -1430,17 +1430,13 @@ void qmp_drive_backup(const char *device, const char *target, Error **errp) { BlockDriverState *bs; - BlockDriverState *target_bs; + BlockDriverState *target_bs, *source; BlockDriver *drv = NULL; Error *local_err = NULL; int flags; int64_t size; int ret; - if (sync != MIRROR_SYNC_MODE_FULL) { - error_setg(errp, "only sync mode 'full' is currently supported"); - return; - } if (!has_speed) { speed = 0; } @@ -1483,6 +1479,13 @@ void qmp_drive_backup(const char *device, const char *target, flags = bs->open_flags | BDRV_O_RDWR; + /* See if we have a backing HD we can use to create our new image + * on top of. */ + source = bs->backing_hd; + if (!source && sync == MIRROR_SYNC_MODE_TOP) { + sync = MIRROR_SYNC_MODE_FULL; + } + size = bdrv_getlength(bs); if (size < 0) { error_setg_errno(errp, -size, "bdrv_getlength failed"); @@ -1491,8 +1494,14 @@ void qmp_drive_backup(const char *device, const char *target, if (mode != NEW_IMAGE_MODE_EXISTING) { assert(format && drv); - bdrv_img_create(target, format, - NULL, NULL, NULL, size, flags, &local_err, false); + if (sync == MIRROR_SYNC_MODE_TOP) { + bdrv_img_create(target, format, source->filename, + source->drv->format_name, NULL, + size, flags, &local_err, false); + } else { + bdrv_img_create(target, format, NULL, NULL, NULL, + size, flags, &local_err, false); + } } if (error_is_set(&local_err)) { @@ -1508,7 +1517,7 @@ void qmp_drive_backup(const char *device, const char *target, return; } - backup_start(bs, target_bs, speed, on_source_error, on_target_error, + backup_start(bs, target_bs, speed, sync, on_source_error, on_target_error, block_job_cb, bs, &local_err); if (local_err != NULL) { bdrv_delete(target_bs); diff --git a/include/block/block_int.h b/include/block/block_int.h index c6ac871..e45f2a0 100644 --- a/include/block/block_int.h +++ b/include/block/block_int.h @@ -404,6 +404,7 @@ void mirror_start(BlockDriverState *bs, BlockDriverState *target, * @bs: Block device to operate on. * @target: Block device to write to. * @speed: The maximum speed, in bytes per second, or 0 for unlimited. + * @sync_mode: What parts of the disk image should be copied to the destination. * @on_source_error: The action to take upon error reading from the source. * @on_target_error: The action to take upon error writing to the target. * @cb: Completion function for the job. @@ -413,7 +414,8 @@ void mirror_start(BlockDriverState *bs, BlockDriverState *target, * until the job is cancelled or manually completed. */ void backup_start(BlockDriverState *bs, BlockDriverState *target, - int64_t speed, BlockdevOnError on_source_error, + int64_t speed, MirrorSyncMode sync_mode, + BlockdevOnError on_source_error, BlockdevOnError on_target_error, BlockDriverCompletionFunc *cb, void *opaque, Error **errp); diff --git a/qapi-schema.json b/qapi-schema.json index cdd2d6b..b3f6c2a 100644 --- a/qapi-schema.json +++ b/qapi-schema.json @@ -1807,6 +1807,10 @@ # @format: #optional the format of the new destination, default is to # probe if @mode is 'existing', else the format of the source # +# @sync: what parts of the disk image should be copied to the destination +# (all the disk, only the sectors allocated in the topmost image, or +# only new I/O). +# # @mode: #optional whether and how QEMU should create a new image, default is # 'absolute-paths'. # diff --git a/qmp-commands.hx b/qmp-commands.hx index e075df4..f3f6b3d 100644 --- a/qmp-commands.hx +++ b/qmp-commands.hx @@ -957,6 +957,7 @@ Arguments: Example: -> { "execute": "drive-backup", "arguments": { "device": "drive0", + "sync": "full", "target": "backup.img" } } <- { "return": {} } EQMP -- 1.8.1.4 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] [PATCH V4 1/4] Implement sync modes for drive-backup. 2013-07-17 20:04 ` [Qemu-devel] [PATCH V4 1/4] " Ian Main @ 2013-07-18 17:19 ` Eric Blake 2013-07-18 19:06 ` Ian Main 0 siblings, 1 reply; 18+ messages in thread From: Eric Blake @ 2013-07-18 17:19 UTC (permalink / raw) To: Ian Main; +Cc: qemu-devel, Stefan Hajnoczi [-- Attachment #1: Type: text/plain, Size: 1781 bytes --] On 07/17/2013 02:04 PM, Ian Main wrote: > This patch adds sync-modes to the drive-backup interface and > implements the FULL, NONE and TOP modes of synchronization. > > FULL performs as before copying the entire contents of the drive > while preserving the point-in-time using CoW. > NONE only copies new writes to the target drive. > TOP copies changes to the topmost drive image and preserves the > point-in-time using CoW. > > +++ b/qapi-schema.json > @@ -1807,6 +1807,10 @@ > # @format: #optional the format of the new destination, default is to > # probe if @mode is 'existing', else the format of the source > # > +# @sync: what parts of the disk image should be copied to the destination > +# (all the disk, only the sectors allocated in the topmost image, or > +# only new I/O). > +# > # @mode: #optional whether and how QEMU should create a new image, default is > # 'absolute-paths'. This hunk looks bogus; the 'DriveBackup' type already documents @sync as of commit b53169e, which makes me suspect this hunk got misapplied during rebasing. > # > diff --git a/qmp-commands.hx b/qmp-commands.hx > index e075df4..f3f6b3d 100644 > --- a/qmp-commands.hx > +++ b/qmp-commands.hx > @@ -957,6 +957,7 @@ Arguments: > > Example: > -> { "execute": "drive-backup", "arguments": { "device": "drive0", > + "sync": "full", > "target": "backup.img" } } Ouch - commit b53169e made 'sync' mandatory, but forgot to update the example to match; perhaps this hunk ought to be applied independently? -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 621 bytes --] ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] [PATCH V4 1/4] Implement sync modes for drive-backup. 2013-07-18 17:19 ` Eric Blake @ 2013-07-18 19:06 ` Ian Main 2013-07-18 19:54 ` Eric Blake 0 siblings, 1 reply; 18+ messages in thread From: Ian Main @ 2013-07-18 19:06 UTC (permalink / raw) To: Eric Blake; +Cc: qemu-devel, Stefan Hajnoczi On Thu, Jul 18, 2013 at 11:19:43AM -0600, Eric Blake wrote: > On 07/17/2013 02:04 PM, Ian Main wrote: > > This patch adds sync-modes to the drive-backup interface and > > implements the FULL, NONE and TOP modes of synchronization. > > > > FULL performs as before copying the entire contents of the drive > > while preserving the point-in-time using CoW. > > NONE only copies new writes to the target drive. > > TOP copies changes to the topmost drive image and preserves the > > point-in-time using CoW. > > > > > +++ b/qapi-schema.json > > @@ -1807,6 +1807,10 @@ > > # @format: #optional the format of the new destination, default is to > > # probe if @mode is 'existing', else the format of the source > > # > > +# @sync: what parts of the disk image should be copied to the destination > > +# (all the disk, only the sectors allocated in the topmost image, or > > +# only new I/O). > > +# > > # @mode: #optional whether and how QEMU should create a new image, default is > > # 'absolute-paths'. > > This hunk looks bogus; the 'DriveBackup' type already documents @sync as > of commit b53169e, which makes me suspect this hunk got misapplied > during rebasing. Did you check that? I know there was one bit of documentation missing that I fixed here. I also just did a clean rebase (git am) to kevin/block and it all applied fine. > > # > > diff --git a/qmp-commands.hx b/qmp-commands.hx > > index e075df4..f3f6b3d 100644 > > --- a/qmp-commands.hx > > +++ b/qmp-commands.hx > > @@ -957,6 +957,7 @@ Arguments: > > > > Example: > > -> { "execute": "drive-backup", "arguments": { "device": "drive0", > > + "sync": "full", > > "target": "backup.img" } } > > Ouch - commit b53169e made 'sync' mandatory, but forgot to update the > example to match; perhaps this hunk ought to be applied independently? That's up to you guys, I can split it out if needed. Ian ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] [PATCH V4 1/4] Implement sync modes for drive-backup. 2013-07-18 19:06 ` Ian Main @ 2013-07-18 19:54 ` Eric Blake 2013-07-19 21:56 ` Ian Main 0 siblings, 1 reply; 18+ messages in thread From: Eric Blake @ 2013-07-18 19:54 UTC (permalink / raw) To: Ian Main; +Cc: qemu-devel, Stefan Hajnoczi [-- Attachment #1: Type: text/plain, Size: 2433 bytes --] On 07/18/2013 01:06 PM, Ian Main wrote: > On Thu, Jul 18, 2013 at 11:19:43AM -0600, Eric Blake wrote: >> On 07/17/2013 02:04 PM, Ian Main wrote: >>> This patch adds sync-modes to the drive-backup interface and >>> implements the FULL, NONE and TOP modes of synchronization. >>> >>> @@ -1807,6 +1807,10 @@ >>> # @format: #optional the format of the new destination, default is to >>> # probe if @mode is 'existing', else the format of the source >>> # >>> +# @sync: what parts of the disk image should be copied to the destination >>> +# (all the disk, only the sectors allocated in the topmost image, or >>> +# only new I/O). >>> +# >>> # @mode: #optional whether and how QEMU should create a new image, default is >>> # 'absolute-paths'. >> >> This hunk looks bogus; the 'DriveBackup' type already documents @sync as >> of commit b53169e, which makes me suspect this hunk got misapplied >> during rebasing. > > Did you check that? I know there was one bit of documentation missing > that I fixed here. I also just did a clean rebase (git am) to > kevin/block and it all applied fine. Yes, it _applied_ fine, because of git's insistence on applying hunks regardless of line fuzzing. It probably applied to 'drive-mirror', since I see this context in the section for 'drive-mirror' when reading the unpatched file at current qemu.git head: > # @format: #optional the format of the new destination, default is to > # probe if @mode is 'existing', else the format of the source > # > # @mode: #optional whether and how QEMU should create a new image, default is > # 'absolute-paths'. > # Hence, my argument that you DON'T want this hunk. >>> Example: >>> -> { "execute": "drive-backup", "arguments": { "device": "drive0", >>> + "sync": "full", >>> "target": "backup.img" } } >> >> Ouch - commit b53169e made 'sync' mandatory, but forgot to update the >> example to match; perhaps this hunk ought to be applied independently? > > That's up to you guys, I can split it out if needed. I don't care either way as long as it gets fixed before 1.6; it all depends on how long the review of the rest of your series takes. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 621 bytes --] ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] [PATCH V4 1/4] Implement sync modes for drive-backup. 2013-07-18 19:54 ` Eric Blake @ 2013-07-19 21:56 ` Ian Main 0 siblings, 0 replies; 18+ messages in thread From: Ian Main @ 2013-07-19 21:56 UTC (permalink / raw) To: Eric Blake; +Cc: qemu-devel, Stefan Hajnoczi On Thu, Jul 18, 2013 at 01:54:45PM -0600, Eric Blake wrote: > On 07/18/2013 01:06 PM, Ian Main wrote: > > On Thu, Jul 18, 2013 at 11:19:43AM -0600, Eric Blake wrote: > >> On 07/17/2013 02:04 PM, Ian Main wrote: > >>> This patch adds sync-modes to the drive-backup interface and > >>> implements the FULL, NONE and TOP modes of synchronization. > >>> > > >>> @@ -1807,6 +1807,10 @@ > >>> # @format: #optional the format of the new destination, default is to > >>> # probe if @mode is 'existing', else the format of the source > >>> # > >>> +# @sync: what parts of the disk image should be copied to the destination > >>> +# (all the disk, only the sectors allocated in the topmost image, or > >>> +# only new I/O). > >>> +# > >>> # @mode: #optional whether and how QEMU should create a new image, default is > >>> # 'absolute-paths'. > >> > >> This hunk looks bogus; the 'DriveBackup' type already documents @sync as > >> of commit b53169e, which makes me suspect this hunk got misapplied > >> during rebasing. > > > > Did you check that? I know there was one bit of documentation missing > > that I fixed here. I also just did a clean rebase (git am) to > > kevin/block and it all applied fine. > > Yes, it _applied_ fine, because of git's insistence on applying hunks > regardless of line fuzzing. It probably applied to 'drive-mirror', > since I see this context in the section for 'drive-mirror' when reading > the unpatched file at current qemu.git head: > > > # @format: #optional the format of the new destination, default is to > > # probe if @mode is 'existing', else the format of the source > > # > > # @mode: #optional whether and how QEMU should create a new image, default is > > # 'absolute-paths'. > > # > > Hence, my argument that you DON'T want this hunk. Ah, yes you are right. I'll fix it next rev along with other things mentioned. Ian ^ permalink raw reply [flat|nested] 18+ messages in thread
* [Qemu-devel] [PATCH V4 2/4] Add tests for sync modes 'TOP' and 'NONE' 2013-07-17 20:04 [Qemu-devel] [PATCH V4 0/4] Implement sync modes for drive-backup Ian Main 2013-07-17 20:04 ` [Qemu-devel] [PATCH V4 1/4] " Ian Main @ 2013-07-17 20:04 ` Ian Main 2013-07-17 20:04 ` [Qemu-devel] [PATCH V4 3/4] Add backing drive while performing backup Ian Main 2013-07-17 20:04 ` [Qemu-devel] [PATCH V4 4/4] Change default to qcow2 for sync mode none Ian Main 3 siblings, 0 replies; 18+ messages in thread From: Ian Main @ 2013-07-17 20:04 UTC (permalink / raw) To: qemu-devel; +Cc: Ian Main This patch adds tests for sync modes top and none. Signed-off-by: Ian Main <imain@redhat.com> --- tests/qemu-iotests/055 | 67 ++++++++++++++++++++++++++++++++++++++++--- tests/qemu-iotests/055.out | 4 +-- tests/qemu-iotests/group | 2 +- tests/qemu-iotests/iotests.py | 5 ++++ 4 files changed, 71 insertions(+), 7 deletions(-) diff --git a/tests/qemu-iotests/055 b/tests/qemu-iotests/055 index c66f8db..06d1f77 100755 --- a/tests/qemu-iotests/055 +++ b/tests/qemu-iotests/055 @@ -23,8 +23,9 @@ import time import os import iotests -from iotests import qemu_img, qemu_io +from iotests import qemu_img, qemu_io, create_image +backing_img = os.path.join(iotests.test_dir, 'backing.img') test_img = os.path.join(iotests.test_dir, 'test.img') target_img = os.path.join(iotests.test_dir, 'target.img') @@ -34,7 +35,7 @@ class TestSingleDrive(iotests.QMPTestCase): def setUp(self): # Write data to the image so we can compare later qemu_img('create', '-f', iotests.imgfmt, test_img, str(TestSingleDrive.image_len)) - qemu_io('-c', 'write -P0x5d 0 64k', test_img) + qemu_io('-c', 'write -P0x41 0 512', test_img) qemu_io('-c', 'write -P0xd5 1M 32k', test_img) qemu_io('-c', 'write -P0xdc 32M 124k', test_img) qemu_io('-c', 'write -P0xdc 67043328 64k', test_img) @@ -60,6 +61,22 @@ class TestSingleDrive(iotests.QMPTestCase): event = self.cancel_and_wait() self.assert_qmp(event, 'data/type', 'backup') + def test_cancel_sync_none(self): + self.assert_no_active_block_jobs() + + result = self.vm.qmp('drive-backup', device='drive0', + sync='none', format='raw', target=target_img) + self.assert_qmp(result, 'return', {}) + time.sleep(1) + self.vm.hmp_qemu_io('drive0', 'write -P0x5e 0 512') + self.vm.hmp_qemu_io('drive0', 'aio_flush') + time.sleep(1) + # Verify that the original contents exist in the target image. + self.assertEqual(-1, qemu_io('-c', 'read -P0x41 0 512', target_img).find("verification failed")) + + event = self.cancel_and_wait() + self.assert_qmp(event, 'data/type', 'backup') + def test_pause(self): self.assert_no_active_block_jobs() @@ -102,6 +119,47 @@ class TestSingleDrive(iotests.QMPTestCase): target=target_img, sync='full') self.assert_qmp(result, 'error/class', 'DeviceNotFound') +class TestSyncModeTop(iotests.QMPTestCase): + image_len = 2 * 1024 * 1024 # MB + + def setUp(self): + create_image(backing_img, TestSyncModeTop.image_len) + qemu_img('create', '-f', iotests.imgfmt, '-o', 'backing_file=%s' % backing_img, test_img) + qemu_io('-c', 'write -P0x5d 0 64k', test_img) + qemu_io('-c', 'write -P0xd5 1M 32k', test_img) + qemu_io('-c', 'write -P0xdc 32M 124k', test_img) + self.vm = iotests.VM().add_drive(test_img) + self.vm.launch() + + def tearDown(self): + self.vm.shutdown() + os.remove(test_img) + os.remove(backing_img) + try: + os.remove(target_img) + except OSError: + pass + + def test_complete_top(self): + self.assert_no_active_block_jobs() + result = self.vm.qmp('drive-backup', device='drive0', sync='top', + target=target_img) + self.assert_qmp(result, 'return', {}) + + # Custom completed check as we are not copying all data. + completed = False + while not completed: + for event in self.vm.get_qmp_events(wait=True): + if event['event'] == 'BLOCK_JOB_COMPLETED': + self.assert_qmp(event, 'data/device', 'drive0') + self.assert_qmp_absent(event, 'data/error') + completed = True + + self.assert_no_active_block_jobs() + self.vm.shutdown() + self.assertTrue(iotests.compare_images(test_img, target_img), + 'target image does not match source after backup') + class TestSetSpeed(iotests.QMPTestCase): image_len = 80 * 1024 * 1024 # MB @@ -127,7 +185,8 @@ class TestSetSpeed(iotests.QMPTestCase): self.assert_qmp(result, 'return[0]/device', 'drive0') self.assert_qmp(result, 'return[0]/speed', 0) - result = self.vm.qmp('block-job-set-speed', device='drive0', speed=8 * 1024 * 1024) + result = self.vm.qmp('block-job-set-speed', device='drive0', + speed=8 * 1024 * 1024) self.assert_qmp(result, 'return', {}) # Ensure the speed we set was accepted @@ -285,4 +344,4 @@ class TestSingleTransaction(iotests.QMPTestCase): self.assert_no_active_block_jobs() if __name__ == '__main__': - iotests.main(supported_fmts=['raw', 'qcow2']) + iotests.main(supported_fmts=['qcow2', 'qed']) diff --git a/tests/qemu-iotests/055.out b/tests/qemu-iotests/055.out index fa16b5c..96961ed 100644 --- a/tests/qemu-iotests/055.out +++ b/tests/qemu-iotests/055.out @@ -1,5 +1,5 @@ -............. +............... ---------------------------------------------------------------------- -Ran 13 tests +Ran 15 tests OK diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group index fdc6ed1..d3c3f61 100644 --- a/tests/qemu-iotests/group +++ b/tests/qemu-iotests/group @@ -61,4 +61,4 @@ 052 rw auto backing 053 rw auto 054 rw auto -055 rw auto +055 rw auto backing diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py index b028a89..33ad0ec 100644 --- a/tests/qemu-iotests/iotests.py +++ b/tests/qemu-iotests/iotests.py @@ -95,6 +95,11 @@ class VM(object): self._num_drives += 1 return self + def hmp_qemu_io(self, drive, cmd): + '''Write to a given drive using an HMP command''' + return self.qmp('human-monitor-command', + command_line='qemu-io %s "%s"' % (drive, cmd)) + def add_fd(self, fd, fdset, opaque, opts=''): '''Pass a file descriptor to the VM''' options = ['fd=%d' % fd, -- 1.8.1.4 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* [Qemu-devel] [PATCH V4 3/4] Add backing drive while performing backup. 2013-07-17 20:04 [Qemu-devel] [PATCH V4 0/4] Implement sync modes for drive-backup Ian Main 2013-07-17 20:04 ` [Qemu-devel] [PATCH V4 1/4] " Ian Main 2013-07-17 20:04 ` [Qemu-devel] [PATCH V4 2/4] Add tests for sync modes 'TOP' and 'NONE' Ian Main @ 2013-07-17 20:04 ` Ian Main 2013-07-18 6:39 ` Fam Zheng 2013-07-17 20:04 ` [Qemu-devel] [PATCH V4 4/4] Change default to qcow2 for sync mode none Ian Main 3 siblings, 1 reply; 18+ messages in thread From: Ian Main @ 2013-07-17 20:04 UTC (permalink / raw) To: qemu-devel; +Cc: Ian Main This patch adds the original source drive as a backing drive to our target image so that the target image will appear complete during backup. This is especially useful for SYNC_MODE_NONE as it allows export via NBD to have a complete point-in-time snapshot available for export. Signed-off-by: Ian Main <imain@redhat.com> --- block/backup.c | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/block/backup.c b/block/backup.c index 68abd23..e086e76 100644 --- a/block/backup.c +++ b/block/backup.c @@ -323,6 +323,10 @@ static void coroutine_fn backup_run(void *opaque) hbitmap_free(job->bitmap); + /* Set the target backing drive back to NULL before calling delete or + * it will also delete the underlying drive. */ + target->backing_hd = NULL; + bdrv_iostatus_disable(target); bdrv_delete(target); @@ -362,6 +366,15 @@ void backup_start(BlockDriverState *bs, BlockDriverState *target, return; } + /* Manually set the backing hd to be the backup source drive so + * that all reads done while we are backing up will be passed + * on to the original source drive. This allows reading from the + * image while the backup is in progress, or in the case of + * SYNC_MODE_NONE allows a complete image to be present for export. + * Note that we do this for all modes including SYNC_MODE_TOP as + * even then it allows on-the-fly reading. */ + target->backing_hd = bs; + job->on_source_error = on_source_error; job->on_target_error = on_target_error; job->target = target; -- 1.8.1.4 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] [PATCH V4 3/4] Add backing drive while performing backup. 2013-07-17 20:04 ` [Qemu-devel] [PATCH V4 3/4] Add backing drive while performing backup Ian Main @ 2013-07-18 6:39 ` Fam Zheng 2013-07-18 13:13 ` Paolo Bonzini 0 siblings, 1 reply; 18+ messages in thread From: Fam Zheng @ 2013-07-18 6:39 UTC (permalink / raw) To: Ian Main, Paolo Bonzini; +Cc: qemu-devel On Wed, 07/17 13:04, Ian Main wrote: > This patch adds the original source drive as a backing drive to our target > image so that the target image will appear complete during backup. This > is especially useful for SYNC_MODE_NONE as it allows export via NBD to > have a complete point-in-time snapshot available for export. > > Signed-off-by: Ian Main <imain@redhat.com> > --- > block/backup.c | 13 +++++++++++++ > 1 file changed, 13 insertions(+) > > diff --git a/block/backup.c b/block/backup.c > index 68abd23..e086e76 100644 > --- a/block/backup.c > +++ b/block/backup.c > @@ -323,6 +323,10 @@ static void coroutine_fn backup_run(void *opaque) > > hbitmap_free(job->bitmap); > > + /* Set the target backing drive back to NULL before calling delete or > + * it will also delete the underlying drive. */ > + target->backing_hd = NULL; > + > bdrv_iostatus_disable(target); > bdrv_delete(target); > > @@ -362,6 +366,15 @@ void backup_start(BlockDriverState *bs, BlockDriverState *target, > return; > } > > + /* Manually set the backing hd to be the backup source drive so > + * that all reads done while we are backing up will be passed > + * on to the original source drive. This allows reading from the > + * image while the backup is in progress, or in the case of > + * SYNC_MODE_NONE allows a complete image to be present for export. > + * Note that we do this for all modes including SYNC_MODE_TOP as > + * even then it allows on-the-fly reading. */ > + target->backing_hd = bs; > + Also set target->backing_file and target->backing_format here? Paolo? Thanks. Fam ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] [PATCH V4 3/4] Add backing drive while performing backup. 2013-07-18 6:39 ` Fam Zheng @ 2013-07-18 13:13 ` Paolo Bonzini 2013-07-19 1:13 ` Fam Zheng 0 siblings, 1 reply; 18+ messages in thread From: Paolo Bonzini @ 2013-07-18 13:13 UTC (permalink / raw) To: famz; +Cc: Ian Main, qemu-devel Il 18/07/2013 08:39, Fam Zheng ha scritto: > On Wed, 07/17 13:04, Ian Main wrote: >> This patch adds the original source drive as a backing drive to our target >> image so that the target image will appear complete during backup. This >> is especially useful for SYNC_MODE_NONE as it allows export via NBD to >> have a complete point-in-time snapshot available for export. >> >> Signed-off-by: Ian Main <imain@redhat.com> >> --- >> block/backup.c | 13 +++++++++++++ >> 1 file changed, 13 insertions(+) >> >> diff --git a/block/backup.c b/block/backup.c >> index 68abd23..e086e76 100644 >> --- a/block/backup.c >> +++ b/block/backup.c >> @@ -323,6 +323,10 @@ static void coroutine_fn backup_run(void *opaque) >> >> hbitmap_free(job->bitmap); >> >> + /* Set the target backing drive back to NULL before calling delete or >> + * it will also delete the underlying drive. */ >> + target->backing_hd = NULL; >> + >> bdrv_iostatus_disable(target); >> bdrv_delete(target); >> >> @@ -362,6 +366,15 @@ void backup_start(BlockDriverState *bs, BlockDriverState *target, >> return; >> } >> >> + /* Manually set the backing hd to be the backup source drive so >> + * that all reads done while we are backing up will be passed >> + * on to the original source drive. This allows reading from the >> + * image while the backup is in progress, or in the case of >> + * SYNC_MODE_NONE allows a complete image to be present for export. >> + * Note that we do this for all modes including SYNC_MODE_TOP as >> + * even then it allows on-the-fly reading. */ >> + target->backing_hd = bs; >> + > > Also set target->backing_file and target->backing_format here? Paolo? I don't think so, it is temporary while the job runs so that the NBD server can already return the actual data. Paolo ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] [PATCH V4 3/4] Add backing drive while performing backup. 2013-07-18 13:13 ` Paolo Bonzini @ 2013-07-19 1:13 ` Fam Zheng 2013-07-19 20:15 ` Paolo Bonzini 0 siblings, 1 reply; 18+ messages in thread From: Fam Zheng @ 2013-07-19 1:13 UTC (permalink / raw) To: Paolo Bonzini; +Cc: Ian Main, qemu-devel On Thu, 07/18 15:13, Paolo Bonzini wrote: > Il 18/07/2013 08:39, Fam Zheng ha scritto: > > On Wed, 07/17 13:04, Ian Main wrote: > >> This patch adds the original source drive as a backing drive to our target > >> image so that the target image will appear complete during backup. This > >> is especially useful for SYNC_MODE_NONE as it allows export via NBD to > >> have a complete point-in-time snapshot available for export. > >> > >> Signed-off-by: Ian Main <imain@redhat.com> > >> --- > >> block/backup.c | 13 +++++++++++++ > >> 1 file changed, 13 insertions(+) > >> > >> diff --git a/block/backup.c b/block/backup.c > >> index 68abd23..e086e76 100644 > >> --- a/block/backup.c > >> +++ b/block/backup.c > >> @@ -323,6 +323,10 @@ static void coroutine_fn backup_run(void *opaque) > >> > >> hbitmap_free(job->bitmap); > >> > >> + /* Set the target backing drive back to NULL before calling delete or > >> + * it will also delete the underlying drive. */ > >> + target->backing_hd = NULL; > >> + > >> bdrv_iostatus_disable(target); > >> bdrv_delete(target); > >> > >> @@ -362,6 +366,15 @@ void backup_start(BlockDriverState *bs, BlockDriverState *target, > >> return; > >> } > >> > >> + /* Manually set the backing hd to be the backup source drive so > >> + * that all reads done while we are backing up will be passed > >> + * on to the original source drive. This allows reading from the > >> + * image while the backup is in progress, or in the case of > >> + * SYNC_MODE_NONE allows a complete image to be present for export. > >> + * Note that we do this for all modes including SYNC_MODE_TOP as > >> + * even then it allows on-the-fly reading. */ > >> + target->backing_hd = bs; > >> + > > > > Also set target->backing_file and target->backing_format here? Paolo? > > I don't think so, it is temporary while the job runs so that the NBD > server can already return the actual data. OK. For NBD export, it's also going to have a name, to be used with nbd_server_add. So should we call bdrv_set_in_use() here to protect target from being used elsewhere? -- Fam ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] [PATCH V4 3/4] Add backing drive while performing backup. 2013-07-19 1:13 ` Fam Zheng @ 2013-07-19 20:15 ` Paolo Bonzini 0 siblings, 0 replies; 18+ messages in thread From: Paolo Bonzini @ 2013-07-19 20:15 UTC (permalink / raw) To: famz; +Cc: Ian Main, qemu-devel Il 19/07/2013 03:13, Fam Zheng ha scritto: >>> > > Also set target->backing_file and target->backing_format here? Paolo? >> > >> > I don't think so, it is temporary while the job runs so that the NBD >> > server can already return the actual data. > OK. > > For NBD export, it's also going to have a name, to be used with > nbd_server_add. So should we call bdrv_set_in_use() here to protect > target from being used elsewhere? I think it'd still be safe, but it's probably a good idea anyway. Paolo ^ permalink raw reply [flat|nested] 18+ messages in thread
* [Qemu-devel] [PATCH V4 4/4] Change default to qcow2 for sync mode none. 2013-07-17 20:04 [Qemu-devel] [PATCH V4 0/4] Implement sync modes for drive-backup Ian Main ` (2 preceding siblings ...) 2013-07-17 20:04 ` [Qemu-devel] [PATCH V4 3/4] Add backing drive while performing backup Ian Main @ 2013-07-17 20:04 ` Ian Main 2013-07-18 17:27 ` Eric Blake 3 siblings, 1 reply; 18+ messages in thread From: Ian Main @ 2013-07-17 20:04 UTC (permalink / raw) To: qemu-devel; +Cc: Ian Main qcow2 supports backing files so it makes sense to default to qcow2 for MIRROR_SYNC_MODE_NONE so that we can use the source as a backing drive and export it via nbd. Defaulting FULL and TOP to SYNC_MODE_NONE breaks tests but that could be fixed if we wanted it. Signed-off-by: Ian Main <imain@redhat.com> --- blockdev.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/blockdev.c b/blockdev.c index 000dea6..805b0e5 100644 --- a/blockdev.c +++ b/blockdev.c @@ -1462,7 +1462,7 @@ void qmp_drive_backup(const char *device, const char *target, } if (!has_format) { - format = mode == NEW_IMAGE_MODE_EXISTING ? NULL : bs->drv->format_name; + format = mode == NEW_IMAGE_MODE_EXISTING ? NULL : "qcow2"; } if (format) { drv = bdrv_find_format(format); -- 1.8.1.4 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] [PATCH V4 4/4] Change default to qcow2 for sync mode none. 2013-07-17 20:04 ` [Qemu-devel] [PATCH V4 4/4] Change default to qcow2 for sync mode none Ian Main @ 2013-07-18 17:27 ` Eric Blake 2013-07-18 17:32 ` Eric Blake 2013-07-18 18:06 ` Ian Main 0 siblings, 2 replies; 18+ messages in thread From: Eric Blake @ 2013-07-18 17:27 UTC (permalink / raw) To: Ian Main; +Cc: qemu-devel [-- Attachment #1: Type: text/plain, Size: 1598 bytes --] On 07/17/2013 02:04 PM, Ian Main wrote: > qcow2 supports backing files so it makes sense to default to qcow2 > for MIRROR_SYNC_MODE_NONE so that we can use the source as a backing > drive and export it via nbd. Defaulting FULL and TOP to SYNC_MODE_NONE > breaks tests but that could be fixed if we wanted it. > > Signed-off-by: Ian Main <imain@redhat.com> > --- > blockdev.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/blockdev.c b/blockdev.c > index 000dea6..805b0e5 100644 > --- a/blockdev.c > +++ b/blockdev.c > @@ -1462,7 +1462,7 @@ void qmp_drive_backup(const char *device, const char *target, > } > > if (!has_format) { > - format = mode == NEW_IMAGE_MODE_EXISTING ? NULL : bs->drv->format_name; > + format = mode == NEW_IMAGE_MODE_EXISTING ? NULL : "qcow2"; Is this the right thing to do? Or should we do: if (!has_format) { if (mode == NEW_IMAGE_MODE_EXISTING) { format = NULL; } else { format = bs->drv->format_name ?: "qcow2"; } } That is, I think we should default to doing a backup in the format given by the original (what if the original is qed, which also supports backing files), and only use qcow2 when there is no guidance whatsoever. But in practice, I don't care - format probing is a security hole, so libvirt should always be passing a format, at which point the entire !has_format condition should be skipped when called by libvirt. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 621 bytes --] ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] [PATCH V4 4/4] Change default to qcow2 for sync mode none. 2013-07-18 17:27 ` Eric Blake @ 2013-07-18 17:32 ` Eric Blake 2013-07-18 18:03 ` Ian Main 2013-07-18 18:06 ` Ian Main 1 sibling, 1 reply; 18+ messages in thread From: Eric Blake @ 2013-07-18 17:32 UTC (permalink / raw) To: Eric Blake; +Cc: Ian Main, qemu-devel [-- Attachment #1: Type: text/plain, Size: 1293 bytes --] On 07/18/2013 11:27 AM, Eric Blake wrote: >> if (!has_format) { >> - format = mode == NEW_IMAGE_MODE_EXISTING ? NULL : bs->drv->format_name; >> + format = mode == NEW_IMAGE_MODE_EXISTING ? NULL : "qcow2"; > > Is this the right thing to do? Or should we do: > > if (!has_format) { > if (mode == NEW_IMAGE_MODE_EXISTING) { > format = NULL; > } else { > format = bs->drv->format_name ?: "qcow2"; > } > } > > That is, I think we should default to doing a backup in the format given > by the original (what if the original is qed, which also supports > backing files), and only use qcow2 when there is no guidance whatsoever. > > But in practice, I don't care Well, I _DO_ care about one thing - make sure that the qapi-schema.json page accurately documents how this variable is defaulted for callers that don't care about the implications of omitting a format. Or we could simplify life by making 'format' mandatory for drive-backup; it was optional for 'drive-mirror' due to incremental implementation, but for 'drive-backup', we still have the opportunity to do things right from the first release. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 621 bytes --] ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] [PATCH V4 4/4] Change default to qcow2 for sync mode none. 2013-07-18 17:32 ` Eric Blake @ 2013-07-18 18:03 ` Ian Main 2013-07-18 18:48 ` Eric Blake 0 siblings, 1 reply; 18+ messages in thread From: Ian Main @ 2013-07-18 18:03 UTC (permalink / raw) To: Eric Blake; +Cc: qemu-devel On Thu, Jul 18, 2013 at 11:32:52AM -0600, Eric Blake wrote: > On 07/18/2013 11:27 AM, Eric Blake wrote: > > >> if (!has_format) { > >> - format = mode == NEW_IMAGE_MODE_EXISTING ? NULL : bs->drv->format_name; > >> + format = mode == NEW_IMAGE_MODE_EXISTING ? NULL : "qcow2"; > > > > Is this the right thing to do? Or should we do: > > > > if (!has_format) { > > if (mode == NEW_IMAGE_MODE_EXISTING) { > > format = NULL; > > } else { > > format = bs->drv->format_name ?: "qcow2"; > > } > > } > > > > That is, I think we should default to doing a backup in the format given > > by the original (what if the original is qed, which also supports > > backing files), and only use qcow2 when there is no guidance whatsoever. > > > > But in practice, I don't care > > Well, I _DO_ care about one thing - make sure that the qapi-schema.json > page accurately documents how this variable is defaulted for callers > that don't care about the implications of omitting a format. > > Or we could simplify life by making 'format' mandatory for drive-backup; > it was optional for 'drive-mirror' due to incremental implementation, > but for 'drive-backup', we still have the opportunity to do things right > from the first release. Ah, I did make a doc change, I must have forgotten to add it. I'm all for making format mandatory if that is ok with everyone. Maybe that is the best solution. Ian ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] [PATCH V4 4/4] Change default to qcow2 for sync mode none. 2013-07-18 18:03 ` Ian Main @ 2013-07-18 18:48 ` Eric Blake 0 siblings, 0 replies; 18+ messages in thread From: Eric Blake @ 2013-07-18 18:48 UTC (permalink / raw) To: Ian Main; +Cc: kwolf, Fam Zheng, qemu-devel, Stefan Hajnoczi [-- Attachment #1: Type: text/plain, Size: 1166 bytes --] On 07/18/2013 12:03 PM, Ian Main wrote: >> >> Or we could simplify life by making 'format' mandatory for drive-backup; >> it was optional for 'drive-mirror' due to incremental implementation, >> but for 'drive-backup', we still have the opportunity to do things right >> from the first release. > > Ah, I did make a doc change, I must have forgotten to add it. > > I'm all for making format mandatory if that is ok with everyone. Maybe > that is the best solution. We can always change our mind in 1.7 to make it optional if we change our minds, but I'd definitely like to see patches that make 'format' mandatory for DriveBackup for 1.6 - simpler all around. Converting mandatory to optional is discoverable via introspection; while converting optional to mandatory is an API break. And since we can argue that optional formats is relatively risky, it's better to have our initial release be conservative by requiring the field until (and unless) someone comes up with a use case why leaving it unspecified makes a difference. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 621 bytes --] ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] [PATCH V4 4/4] Change default to qcow2 for sync mode none. 2013-07-18 17:27 ` Eric Blake 2013-07-18 17:32 ` Eric Blake @ 2013-07-18 18:06 ` Ian Main 1 sibling, 0 replies; 18+ messages in thread From: Ian Main @ 2013-07-18 18:06 UTC (permalink / raw) To: Eric Blake; +Cc: qemu-devel On Thu, Jul 18, 2013 at 11:27:21AM -0600, Eric Blake wrote: > On 07/17/2013 02:04 PM, Ian Main wrote: > > qcow2 supports backing files so it makes sense to default to qcow2 > > for MIRROR_SYNC_MODE_NONE so that we can use the source as a backing > > drive and export it via nbd. Defaulting FULL and TOP to SYNC_MODE_NONE > > breaks tests but that could be fixed if we wanted it. > > > > Signed-off-by: Ian Main <imain@redhat.com> > > --- > > blockdev.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/blockdev.c b/blockdev.c > > index 000dea6..805b0e5 100644 > > --- a/blockdev.c > > +++ b/blockdev.c > > @@ -1462,7 +1462,7 @@ void qmp_drive_backup(const char *device, const char *target, > > } > > > > if (!has_format) { > > - format = mode == NEW_IMAGE_MODE_EXISTING ? NULL : bs->drv->format_name; > > + format = mode == NEW_IMAGE_MODE_EXISTING ? NULL : "qcow2"; > > Is this the right thing to do? Or should we do: > > if (!has_format) { > if (mode == NEW_IMAGE_MODE_EXISTING) { > format = NULL; > } else { > format = bs->drv->format_name ?: "qcow2"; > } > } > > That is, I think we should default to doing a backup in the format given > by the original (what if the original is qed, which also supports > backing files), and only use qcow2 when there is no guidance whatsoever. > > But in practice, I don't care - format probing is a security hole, so > libvirt should always be passing a format, at which point the entire > !has_format condition should be skipped when called by libvirt. Heh, actually that is basically what I have now, as with the doc change I forgot to git add it. Doh! I'll repost.. I'm also not opposed to format being non-optional. Ian ^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2013-07-19 21:56 UTC | newest] Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2013-07-17 20:04 [Qemu-devel] [PATCH V4 0/4] Implement sync modes for drive-backup Ian Main 2013-07-17 20:04 ` [Qemu-devel] [PATCH V4 1/4] " Ian Main 2013-07-18 17:19 ` Eric Blake 2013-07-18 19:06 ` Ian Main 2013-07-18 19:54 ` Eric Blake 2013-07-19 21:56 ` Ian Main 2013-07-17 20:04 ` [Qemu-devel] [PATCH V4 2/4] Add tests for sync modes 'TOP' and 'NONE' Ian Main 2013-07-17 20:04 ` [Qemu-devel] [PATCH V4 3/4] Add backing drive while performing backup Ian Main 2013-07-18 6:39 ` Fam Zheng 2013-07-18 13:13 ` Paolo Bonzini 2013-07-19 1:13 ` Fam Zheng 2013-07-19 20:15 ` Paolo Bonzini 2013-07-17 20:04 ` [Qemu-devel] [PATCH V4 4/4] Change default to qcow2 for sync mode none Ian Main 2013-07-18 17:27 ` Eric Blake 2013-07-18 17:32 ` Eric Blake 2013-07-18 18:03 ` Ian Main 2013-07-18 18:48 ` Eric Blake 2013-07-18 18:06 ` Ian Main
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.