From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:49051) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XwouF-0004W1-Ti for qemu-devel@nongnu.org; Fri, 05 Dec 2014 04:10:38 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Xwou9-0007MX-IH for qemu-devel@nongnu.org; Fri, 05 Dec 2014 04:10:31 -0500 Received: from mx1.redhat.com ([209.132.183.28]:40942) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Xwou7-0007Kf-5i for qemu-devel@nongnu.org; Fri, 05 Dec 2014 04:10:25 -0500 Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id sB59AKGj021156 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL) for ; Fri, 5 Dec 2014 04:10:21 -0500 Message-ID: <54817679.505@redhat.com> Date: Fri, 05 Dec 2014 10:10:17 +0100 From: Max Reitz MIME-Version: 1.0 References: <1417660192-3773-1-git-send-email-famz@redhat.com> <1417660192-3773-2-git-send-email-famz@redhat.com> <54806518.4040301@redhat.com> <20141205061249.GA15691@ad.nay.redhat.com> In-Reply-To: <20141205061249.GA15691@ad.nay.redhat.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v4 1/3] qmp: Add command 'blockdev-backup' List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Fam Zheng Cc: Kevin Wolf , qemu-devel@nongnu.org, Stefan Hajnoczi , Markus Armbruster On 2014-12-05 at 07:12, Fam Zheng wrote: > On Thu, 12/04 14:43, Max Reitz wrote: >>> + if (!bs) { >>> + error_set(errp, QERR_DEVICE_NOT_FOUND, device); >>> + return; >>> + } >>> + >>> + target_bs = bdrv_find(target); >>> + if (!target_bs) { >>> + error_set(errp, QERR_DEVICE_NOT_FOUND, target); >>> + return; >>> + } >>> + >>> + bdrv_ref(target_bs); >>> + bdrv_set_aio_context(target_bs, bdrv_get_aio_context(bs)); >> In the cover letter you said you were acquiring the AIO context but you're >> not. Something like the aio_context_acquire() call in qmp_drive_backup() >> seems missing. > Yes. Adding. > >>> + backup_start(bs, target_bs, speed, sync, on_source_error, on_target_error, >>> + block_job_cb, bs, &local_err); >>> + if (local_err != NULL) { >>> + bdrv_unref(target_bs); >> Hm, as far as I can see, backup_complete() is always run, regardless of the >> operation status. backup_complete() in turn calls bdrv_unref(s->target), so >> this seems unnecessary to me. > In error case, backup_start does nothing. So we need to release for the > bdrv_ref two lines above. Ah, right, I forgot about early errors. local_err is not even set if the block job itself fails, because by the time the block job is started, backup_start() will have already returned. My fault. Max >>> +SQMP >>> +blockdev-backup >>> +------------ >>> + >>> +The device version of drive-backup: this command takes an existing named device >>> +as backup target. >>> + >>> +Arguments: >>> + >>> +- "device": the name of the device which should be copied. >>> + (json-string) >>> +- "target": the target of the new image. If the file exists, or if it is a >>> + device, the existing file/device will be used as the new >>> + destination. If it does not exist, a new file will be created. >>> + (json-string) >>> +- "sync": what parts of the disk image should be copied to the destination; >>> + possibilities include "full" for all the disk, "top" for only the >>> + sectors allocated in the topmost image, or "none" to only replicate >>> + new I/O (MirrorSyncMode). >>> +- "speed": the maximum speed, in bytes per second (json-int, optional) >>> +- "on-source-error": the action to take on an error on the source, default >>> + 'report'. 'stop' and 'enospc' can only be used >>> + if the block device supports io-status. >>> + (BlockdevOnError, optional) >>> +- "on-target-error": the action to take on an error on the target, default >>> + 'report' (no limitations, since this applies to >>> + a different block device than device). >>> + (BlockdevOnError, optional) >>> + >>> +Example: >>> +-> { "execute": "blockdev-backup", "arguments": { "device": "src-id", >>> + "target": "tgt-id" } } >> Isn't "sync" missing? > Yes. > > Thanks, > Fam