Re: [libvirt] [PATCH] BlockCopy support rbd external destination file

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(a)chinac.com&gt; > --- > 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 -- libvir-list mailing list libvir-list(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list