[libvirt] [PATCH] BlockCopy support rbd external destination file
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. Signed-off-by: Liu Qing <liuqing@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 @@ -16609,6 +16609,7 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm, virQEMUDriverConfigPtr cfg = NULL; const char *format = NULL; int desttype = virStorageSourceGetActualType(mirror); + char *mirr_path = NULL; /* Preliminaries: find the disk we are editing, sanity checks */ virCheckFlags(VIR_DOMAIN_BLOCK_COPY_SHALLOW | @@ -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, + _("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; } - /* 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) + goto endjob; + } + else if (VIR_STRDUP(mirr_path, mirror->path) < 0) + 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) @@ -16775,6 +16794,7 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm, qemuDomainObjEndJob(driver, vm); cleanup: + VIR_FREE(mirr_path); VIR_FREE(device); virObjectUnref(cfg); return ret; -- 1.9.1
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@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
+ _("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?
} -
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) ^
+ 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.
+ 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
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@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@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
participants (3)
-
Liu Qing -
liuqing
-
Peter Krempa