[libvirt] [PATCH] BlockCopy support rbd external destination file
liuqing
liuqing at chinac.com
Mon Nov 14 09:59:30 UTC 2016
Thanks Peter, I've do some change in v2.
On 2016/11/11 21:53, Peter Krempa wrote:
> On Wed, Nov 09, 2016 at 10:44:54 +0800, Liu Qing wrote:
>> Currently qemuDomainBlockCopy only support local file. This patch make
>> the rbd external destination file could be passed to qemu driver_mirror.
>> If the external rbd file is not exist, qemuDomainBlockCopy will report
>> an error.
>
> Qemu will report an errro, sice libvirt can't detect currently that the
> file does not exist.
>
>>
>> Signed-off-by: Liu Qing <liuqing at chinac.com>
>> ---
>> src/qemu/qemu_driver.c | 82 +++++++++++++++++++++++++++++++-------------------
>> 1 file changed, 51 insertions(+), 31 deletions(-)
>>
>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>> index 38c8414..ee8db69 100644
>> --- a/src/qemu/qemu_driver.c
>> +++ b/src/qemu/qemu_driver.c
>
> [...]
>
>> @@ -16675,42 +16676,50 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm,
>> }
>>
>> /* Prepare the destination file. */
>> - /* XXX Allow non-file mirror destinations */
>> - if (!virStorageSourceIsLocalStorage(mirror)) {
>> + if (!virStorageSourceIsLocalStorage(mirror) &&
>> + (mirror->type != VIR_STORAGE_TYPE_NETWORK ||
>> + mirror->protocol != VIR_STORAGE_NET_PROTOCOL_RBD)) {
>> virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
>> _("non-file destination not supported yet"));
>> goto endjob;
>> }
>> - if (stat(mirror->path, &st) < 0) {
>> - if (errno != ENOENT) {
>> - virReportSystemError(errno, _("unable to stat for disk %s: %s"),
>> - disk->dst, mirror->path);
>> - goto endjob;
>> - } else if (flags & VIR_DOMAIN_BLOCK_COPY_REUSE_EXT ||
>> - desttype == VIR_STORAGE_TYPE_BLOCK) {
>> - virReportSystemError(errno,
>> - _("missing destination file for disk %s: %s"),
>> - disk->dst, mirror->path);
>> - goto endjob;
>> - }
>> - } else if (!S_ISBLK(st.st_mode)) {
>> - if (st.st_size && !(flags & VIR_DOMAIN_BLOCK_COPY_REUSE_EXT)) {
>> - virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>> - _("external destination file for disk %s already "
>> - "exists and is not a block device: %s"),
>> - disk->dst, mirror->path);
>> - goto endjob;
>> - }
>> - if (desttype == VIR_STORAGE_TYPE_BLOCK) {
>> - virReportError(VIR_ERR_INVALID_ARG,
>> - _("blockdev flag requested for disk %s, but file "
>> - "'%s' is not a block device"),
>> - disk->dst, mirror->path);
>> - goto endjob;
>> + if (virStorageSourceIsLocalStorage(mirror)) {
>> + if (stat(mirror->path, &st) < 0) {
>> + if (errno != ENOENT) {
>> + virReportSystemError(errno, _("unable to stat for disk %s: %s"),
>> + disk->dst, mirror->path);
>> + goto endjob;
>> + } else if (flags & VIR_DOMAIN_BLOCK_COPY_REUSE_EXT ||
>> + desttype == VIR_STORAGE_TYPE_BLOCK) {
>> + virReportSystemError(errno,
>> + _("missing destination file for disk %s: %s"),
>> + disk->dst, mirror->path);
>> + goto endjob;
>> + }
>> + } else if (!S_ISBLK(st.st_mode)) {
>> + if (st.st_size && !(flags & VIR_DOMAIN_BLOCK_COPY_REUSE_EXT)) {
>> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>> + _("external destination file for disk %s already "
>> + "exists and is not a block device: %s"),
>> + disk->dst, mirror->path);
>> + goto endjob;
>> + }
>> + if (desttype == VIR_STORAGE_TYPE_BLOCK) {
>> + virReportError(VIR_ERR_INVALID_ARG,
>> + _("blockdev flag requested for disk %s, but file "
>> + "'%s' is not a block device"),
>> + disk->dst, mirror->path);
>> + goto endjob;
>> + }
>> }
>> + } else if (!(flags & VIR_DOMAIN_BLOCK_COPY_REUSE_EXT)) {
>> + virReportError(VIR_ERR_INVALID_ARG,
>
> You are missing a formating character in the error string. For strings
> which don't need one you need to add a separate argument with a "%s"
> formatting string.
>
> Please make sure you run make syntax-check before posting since it
> catches those:
>
> prohibit_diagnostic_without_format
> src/qemu/qemu_driver.c:16716: virReportError(VIR_ERR_INVALID_ARG,
> src/qemu/qemu_driver.c-16717- _("only external destination file is supported for "
> src/qemu/qemu_driver.c-16718- "rbd protocol"));
> maint.mk: found diagnostic without %
> make: *** [cfg.mk:653: sc_prohibit_diagnostic_without_format] Error 1
>
Fixed
>> + _("only external destination file is supported for "
>> + "rbd protocol"));
>> + goto endjob;
>> }
>>
>> - if (!mirror->format) {
>> + if (!mirror->format && virStorageSourceIsLocalStorage(mirror)) {
>> if (!(flags & VIR_DOMAIN_BLOCK_COPY_REUSE_EXT)) {
>> mirror->format = disk->src->format;
>> } else {
>> @@ -16721,8 +16730,12 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm,
>> mirror->format = virStorageFileProbeFormat(mirror->path, cfg->user,
>> cfg->group);
>> }
>> + } else {
>> + virReportError(VIR_ERR_INVALID_ARG,
>> + _("format is required for %s"),
>> + mirror->path);
>> + goto endjob;
>
> This case is executed if mirror->format is not _FORMAT_NONE or if the
> storage is not local, thus always in your case of RBD.
>
> Are you sure that this works at all?
>
Right. I add the else case after doing my test, but never retest it.
Fixed in v2.
>> }
>> -
>
> Spurious whitespace change.
>
>> /* pre-create the image file */
>> if (!(flags & VIR_DOMAIN_BLOCK_COPY_REUSE_EXT)) {
>> int fd = qemuOpenFile(driver, vm, mirror->path,
>> @@ -16744,10 +16757,16 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm,
>> qemuDomainDiskChainElementRevoke(driver, vm, mirror);
>> goto endjob;
>> }
>> + if (mirror->protocol == VIR_STORAGE_NET_PROTOCOL_RBD) {
>> + if (qemuGetDriveSourceString(mirror, conn, &mirr_path) < 0)
>
> This fails to compile. Did you base the version on the current master of
> libvirt? We changed the prototype of the function a while ago.
>
> qemu/qemu_driver.c: In function 'qemuDomainBlockCopyCommon':
> qemu/qemu_driver.c:16761:46: error: passing argument 2 of 'qemuGetDriveSourceString' from incompatible pointer type [-Werror=incompatible-pointer-types]
> if (qemuGetDriveSourceString(mirror, conn, &mirr_path) < 0)
> ^
>
My original working version is 1.2.17. I build a new environment for the
latest version.
The new parameter secinfo make the patch a little tough. I use mirror to
replace disk->src to get the secinfo and then swap it back in v2.
>> + goto endjob;
>> + }
>> + else if (VIR_STRDUP(mirr_path, mirror->path) < 0)
>
> qemuGetDriveSourceString handles this case as well, so you can use it in
> both cases.
>
> This case also breaks the coding style. If either of the branches of an
> 'if' statement uses a block, both need to use it.
>
fixed
>> + goto endjob;
>>
>> /* Actually start the mirroring */
>> qemuDomainObjEnterMonitor(driver, vm);
>> - ret = qemuMonitorDriveMirror(priv->mon, device, mirror->path, format,
>> + ret = qemuMonitorDriveMirror(priv->mon, device, mirr_path, format,
>> bandwidth, granularity, buf_size, flags);
>> virDomainAuditDisk(vm, NULL, mirror, "mirror", ret >= 0);
>> if (qemuDomainObjExitMonitor(driver, vm) < 0)
>
> Peter
>
>
>
> --
> libvir-list mailing list
> libvir-list at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
>
More information about the libvir-list
mailing list