On 24.12.2015 04:25, Fam Zheng wrote:
> On Thu, 12/24 01:53, Max Reitz wrote:
>> On 23.12.2015 06:59, Fam Zheng wrote:
>>> This will start a mirror job from a named device to another named
>>> device, its relation with drive-mirror is similar with blockdev-backup
>>> to drive-backup.
>>>
>>> In blockdev-mirror, the target node should be prepared by blockdev-add,
>>> which will be responsible for assigning a name to the new node, so
>>> we don't have 'node-name' parameter.
>>>
>>> Signed-off-by: Fam Zheng <f...@redhat.com>
>>> ---
>>> blockdev.c | 62
>>> ++++++++++++++++++++++++++++++++++++++++++++++++++++
>>> qapi/block-core.json | 47 +++++++++++++++++++++++++++++++++++++++
>>> qmp-commands.hx | 48 ++++++++++++++++++++++++++++++++++++++++
>>> 3 files changed, 157 insertions(+)
>>
>> It appears you haven't addressed the comments for v2. I only had a
>> single one (regarding documentation), but Markus had a couple ones, so
>> those may be worth addressing.
>
> Will look into that.
>
>>
>>>
>>> diff --git a/blockdev.c b/blockdev.c
>>> index f42e171..2df0c6d 100644
>>> --- a/blockdev.c
>>> +++ b/blockdev.c
>>> @@ -3345,6 +3345,10 @@ static void blockdev_mirror_common(BlockDriverState
>>> *bs,
>>> if (bdrv_op_is_blocked(target, BLOCK_OP_TYPE_MIRROR_TARGET, errp)) {
>>> return;
>>> }
>>> + if (target->blk) {
>>> + error_setg(errp, "Cannot mirror to an attached block device");
>>> + return;
>>> + }
>>>
>>> if (!bs->backing && sync == MIRROR_SYNC_MODE_TOP) {
>>> sync = MIRROR_SYNC_MODE_FULL;
>>> @@ -3518,6 +3522,64 @@ out:
>>> aio_context_release(aio_context);
>>> }
>>>
>>> +void qmp_blockdev_mirror(const char *device, const char *target,
>>> + bool has_replaces, const char *replaces,
>>> + MirrorSyncMode sync,
>>> + bool has_speed, int64_t speed,
>>> + bool has_granularity, uint32_t granularity,
>>> + bool has_buf_size, int64_t buf_size,
>>> + bool has_on_source_error,
>>> + BlockdevOnError on_source_error,
>>> + bool has_on_target_error,
>>> + BlockdevOnError on_target_error,
>>> + Error **errp)
>>> +{
>>> + BlockDriverState *bs;
>>> + BlockBackend *blk;
>>> + BlockDriverState *target_bs;
>>> + AioContext *aio_context;
>>> + Error *local_err = NULL;
>>> +
>>> + blk = blk_by_name(device);
>>> + if (!blk) {
>>> + error_setg(errp, "Device '%s' not found", device);
>>> + return;
>>> + }
>>> + bs = blk_bs(blk);
>>> +
>>> + if (!bs) {
>>> + error_setg(errp, "Device '%s' has no media", device);
>>> + return;
>>> + }
>>> +
>>> + target_bs = bdrv_lookup_bs(target, target, errp);
>>> + if (!target_bs) {
>>> + return;
>>> + }
>>> +
>>> + aio_context = bdrv_get_aio_context(bs);
>>> + aio_context_acquire(aio_context);
>>> +
>>> + bdrv_ref(target_bs);
>>> + bdrv_set_aio_context(target_bs, aio_context);
>>> +
>>> + blockdev_mirror_common(bs, target_bs,
>>> + has_replaces, replaces, sync,
>>> + has_speed, speed,
>>> + has_granularity, granularity,
>>> + has_buf_size, buf_size,
>>> + has_on_source_error, on_source_error,
>>> + has_on_target_error, on_target_error,
>>> + true, true,
>>
>> Shouldn't this be "false, false,", or, ideally, set by the user?
>
> I think true is correct here because then it will be effectively controlled by
> open flags of target. I.e. mirror.c always sets BDRV_REQ_MAY_UNMAP, and
> bdrv_co_write_zeroes has:
>
> if (!(bs->open_flags & BDRV_O_UNMAP)) {
> flags &= ~BDRV_REQ_MAY_UNMAP;
> }I was asking because it differs from what drive-mirror does - but that is probably a good thing (drive-mirror takes this flag from the user (defaulting to false, which is why I was asking), but it takes the open_flags for the new image from the mirror source, which is... Interesting. So it's probably better this way, right. Max
signature.asc
Description: OpenPGP digital signature
