[libvirt] [PATCH 0/5] qemu: Modernize storage file access for block copy
Refactor access of the block copy target to use the storage driver APIs. Peter Krempa (5): qemu: driver: Split out access to VIR_DOMAIN_BLOCK_COPY_REUSE_EXT qemu: blockcopy: Explicitly assert 'reuse' for block devices qemu: blockcopy: reuse storage driver APIs to pre-create copy target qemu: blockcopy: Split out checking of the target image file qemu: blockcopy: Refactor logic checking the target storage file src/qemu/qemu_driver.c | 120 ++++++++++++++++++++++++++++++++----------------- 1 file changed, 79 insertions(+), 41 deletions(-) -- 2.12.2
Extract the presence of the flag into a boolean to simplify conditions and allow further manipulation of the state of the flag. --- src/qemu/qemu_driver.c | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index a7019c53c..d03a9dbc3 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -16709,6 +16709,7 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm, const char *format = NULL; int desttype = virStorageSourceGetActualType(mirror); virErrorPtr monitor_error = NULL; + bool reuse = !!(flags & VIR_DOMAIN_BLOCK_COPY_REUSE_EXT); /* Preliminaries: find the disk we are editing, sanity checks */ virCheckFlags(VIR_DOMAIN_BLOCK_COPY_SHALLOW | @@ -16769,8 +16770,7 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm, /* unless the user provides a pre-created file, shallow copy into a raw * file is not possible */ - if ((flags & VIR_DOMAIN_BLOCK_COPY_SHALLOW) && - !(flags & VIR_DOMAIN_BLOCK_COPY_REUSE_EXT) && + if ((flags & VIR_DOMAIN_BLOCK_COPY_SHALLOW) && !reuse && mirror->format == VIR_STORAGE_FILE_RAW) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("shallow copy of disk '%s' into a raw file " @@ -16791,15 +16791,14 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm, 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) { + } else if (reuse || 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)) { + if (st.st_size && !reuse) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("external destination file for disk %s already " "exists and is not a block device: %s"), @@ -16816,7 +16815,7 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm, } if (!mirror->format) { - if (!(flags & VIR_DOMAIN_BLOCK_COPY_REUSE_EXT)) { + if (!reuse) { mirror->format = disk->src->format; } else { /* If the user passed the REUSE_EXT flag, then either they @@ -16829,7 +16828,7 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm, } /* pre-create the image file */ - if (!(flags & VIR_DOMAIN_BLOCK_COPY_REUSE_EXT)) { + if (!reuse) { int fd = qemuOpenFile(driver, vm, mirror->path, O_WRONLY | O_TRUNC | O_CREAT, &need_unlink, NULL); -- 2.12.2
On 07/11/2017 11:46 AM, Peter Krempa wrote:
Extract the presence of the flag into a boolean to simplify conditions and allow further manipulation of the state of the flag. --- src/qemu/qemu_driver.c | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-)
Reviewed-by: John Ferlan <jferlan@redhat.com> John
When copying to a block device, the block device will already exist. To allow users using a block device without any preparation, they need to use the block copy without VIR_DOMAIN_BLOCK_COPY_REUSE_EXT. This means that if the target is an existing block device we don't need to prepare it, but we can't reject it as being existing. To avoid breaking this feature, explicitly assume that existing block devices will be reused even without that flag explicitly specified, while skipping attempts to create it. qemuMonitorDriveMirror still needs to honor the flag as specified by the user, since qemu overwrites the metadata otherwise. --- src/qemu/qemu_driver.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index d03a9dbc3..d00166f23 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -16812,6 +16812,10 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm, disk->dst, mirror->path); goto endjob; } + } else { + /* if the target is a block device, assume that we are reusing it, so + * there are no attempts to create it */ + reuse = true; } if (!mirror->format) { @@ -16851,6 +16855,8 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm, /* Actually start the mirroring */ qemuDomainObjEnterMonitor(driver, vm); + /* qemuMonitorDriveMirror needs to honor the REUSE_EXT flag as specified + * by the user regardless of how @reuse was modified */ ret = qemuMonitorDriveMirror(priv->mon, device, mirror->path, format, bandwidth, granularity, buf_size, flags); virDomainAuditDisk(vm, NULL, mirror, "mirror", ret >= 0); -- 2.12.2
On 07/11/2017 11:46 AM, Peter Krempa wrote:
When copying to a block device, the block device will already exist. To allow users using a block device without any preparation, they need to use the block copy without VIR_DOMAIN_BLOCK_COPY_REUSE_EXT.
This means that if the target is an existing block device we don't need to prepare it, but we can't reject it as being existing.
To avoid breaking this feature, explicitly assume that existing block devices will be reused even without that flag explicitly specified, while skipping attempts to create it.
qemuMonitorDriveMirror still needs to honor the flag as specified by the user, since qemu overwrites the metadata otherwise. --- src/qemu/qemu_driver.c | 6 ++++++ 1 file changed, 6 insertions(+)
Reviewed-by: John Ferlan <jferlan@redhat.com> John
Rather than using the local-file only implementation 'qemuOpenFile' switch to the imagelabel aware storage driver implementation. --- src/qemu/qemu_driver.c | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index d00166f23..48dc5e5cc 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -16786,6 +16786,10 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm, _("non-file destination not supported yet")); goto endjob; } + + if (qemuDomainStorageFileInit(driver, vm, mirror) < 0) + goto endjob; + if (stat(mirror->path, &st) < 0) { if (errno != ENOENT) { virReportSystemError(errno, _("unable to stat for disk %s: %s"), @@ -16833,12 +16837,12 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm, /* pre-create the image file */ if (!reuse) { - int fd = qemuOpenFile(driver, vm, mirror->path, - O_WRONLY | O_TRUNC | O_CREAT, - &need_unlink, NULL); - if (fd < 0) + if (virStorageFileCreate(mirror) < 0) { + virReportSystemError(errno, "%s", _("failed to create copy target")); goto endjob; - VIR_FORCE_CLOSE(fd); + } + + need_unlink = true; } if (mirror->format > 0) @@ -16870,6 +16874,7 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm, /* Update vm in place to match changes. */ need_unlink = false; + virStorageFileDeinit(mirror); disk->mirror = mirror; mirror = NULL; disk->mirrorJob = VIR_DOMAIN_BLOCK_JOB_TYPE_COPY; @@ -16880,8 +16885,10 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm, vm->def->name); endjob: - if (need_unlink && unlink(mirror->path)) - VIR_WARN("unable to unlink just-created %s", mirror->path); + if (need_unlink && virStorageFileUnlink(mirror)) + VIR_WARN("%s", _("unable to remove just-created copy target")); + virStorageFileDeinit(mirror); + virStorageSourceFree(mirror); qemuDomainObjEndJob(driver, vm); if (monitor_error) { virSetError(monitor_error); -- 2.12.2
On 07/11/2017 11:46 AM, Peter Krempa wrote:
Rather than using the local-file only implementation 'qemuOpenFile' switch to the imagelabel aware storage driver implementation. --- src/qemu/qemu_driver.c | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index d00166f23..48dc5e5cc 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -16786,6 +16786,10 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm, _("non-file destination not supported yet")); goto endjob; } + + if (qemuDomainStorageFileInit(driver, vm, mirror) < 0) + goto endjob; + if (stat(mirror->path, &st) < 0) { if (errno != ENOENT) { virReportSystemError(errno, _("unable to stat for disk %s: %s"), @@ -16833,12 +16837,12 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm,
/* pre-create the image file */ if (!reuse) { - int fd = qemuOpenFile(driver, vm, mirror->path, - O_WRONLY | O_TRUNC | O_CREAT, - &need_unlink, NULL); - if (fd < 0) + if (virStorageFileCreate(mirror) < 0) { + virReportSystemError(errno, "%s", _("failed to create copy target")); goto endjob; - VIR_FORCE_CLOSE(fd); + } + + need_unlink = true; }
if (mirror->format > 0) @@ -16870,6 +16874,7 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm,
/* Update vm in place to match changes. */ need_unlink = false; + virStorageFileDeinit(mirror); disk->mirror = mirror; mirror = NULL; disk->mirrorJob = VIR_DOMAIN_BLOCK_JOB_TYPE_COPY; @@ -16880,8 +16885,10 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm, vm->def->name);
endjob: - if (need_unlink && unlink(mirror->path)) - VIR_WARN("unable to unlink just-created %s", mirror->path); + if (need_unlink && virStorageFileUnlink(mirror))
...Unlink(mirror) < 0)
+ VIR_WARN("%s", _("unable to remove just-created copy target")); + virStorageFileDeinit(mirror); + virStorageSourceFree(mirror);
Isn't this duplicitous with the same in the cleanup: label
qemuDomainObjEndJob(driver, vm); if (monitor_error) { virSetError(monitor_error);
with fixups... Reviewed-by: John Ferlan <jferlan@redhat.com> John
On Wed, Jul 19, 2017 at 17:44:16 -0400, John Ferlan wrote:
On 07/11/2017 11:46 AM, Peter Krempa wrote:
Rather than using the local-file only implementation 'qemuOpenFile' switch to the imagelabel aware storage driver implementation. --- src/qemu/qemu_driver.c | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index d00166f23..48dc5e5cc 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c
[...]
@@ -16880,8 +16885,10 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm, vm->def->name);
endjob: - if (need_unlink && unlink(mirror->path)) - VIR_WARN("unable to unlink just-created %s", mirror->path); + if (need_unlink && virStorageFileUnlink(mirror))
...Unlink(mirror) < 0)
+ VIR_WARN("%s", _("unable to remove just-created copy target")); + virStorageFileDeinit(mirror); + virStorageSourceFree(mirror);
Isn't this duplicitous with the same in the cleanup: label
Yes indeed. It would even crash on the error paths if 'mirror' was not consumed into the disk definition structure. Good catch.
qemuDomainObjEndJob(driver, vm); if (monitor_error) { virSetError(monitor_error);
with fixups...
Reviewed-by: John Ferlan <jferlan@redhat.com>
Thanks
Move the code into a separate function so that the flow of creating the copy is more obvious and split into logical pieces. --- src/qemu/qemu_driver.c | 79 +++++++++++++++++++++++++++++--------------------- 1 file changed, 46 insertions(+), 33 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 48dc5e5cc..5beb14e64 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -16685,6 +16685,50 @@ qemuDomainBlockJobSetSpeed(virDomainPtr dom, } +static int +qemuDomainBlockCopyValidateMirror(virStorageSourcePtr mirror, + const char *dst, + bool *reuse) +{ + int desttype = virStorageSourceGetActualType(mirror); + struct stat st; + + if (stat(mirror->path, &st) < 0) { + if (errno != ENOENT) { + virReportSystemError(errno, _("unable to stat for disk %s: %s"), + dst, mirror->path); + return -1; + } else if (*reuse || desttype == VIR_STORAGE_TYPE_BLOCK) { + virReportSystemError(errno, + _("missing destination file for disk %s: %s"), + dst, mirror->path); + return -1; + } + } else if (!S_ISBLK(st.st_mode)) { + if (st.st_size && !(*reuse)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("external destination file for disk %s already " + "exists and is not a block device: %s"), + dst, mirror->path); + return -1; + } + 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"), + dst, mirror->path); + return -1; + } + } else { + /* if the target is a block device, assume that we are reusing it, so + * there are no attempts to create it */ + *reuse = true; + } + + return 0; +} + + /* bandwidth in bytes/s. Caller must lock vm beforehand, and not * access mirror afterwards. */ static int @@ -16703,11 +16747,9 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm, char *device = NULL; virDomainDiskDefPtr disk = NULL; int ret = -1; - struct stat st; bool need_unlink = false; virQEMUDriverConfigPtr cfg = NULL; const char *format = NULL; - int desttype = virStorageSourceGetActualType(mirror); virErrorPtr monitor_error = NULL; bool reuse = !!(flags & VIR_DOMAIN_BLOCK_COPY_REUSE_EXT); @@ -16790,37 +16832,8 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm, if (qemuDomainStorageFileInit(driver, vm, mirror) < 0) 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 (reuse || 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 && !reuse) { - 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 the target is a block device, assume that we are reusing it, so - * there are no attempts to create it */ - reuse = true; - } + if (qemuDomainBlockCopyValidateMirror(mirror, disk->dst, &reuse) < 0) + goto endjob; if (!mirror->format) { if (!reuse) { -- 2.12.2
On 07/11/2017 11:46 AM, Peter Krempa wrote:
Move the code into a separate function so that the flow of creating the copy is more obvious and split into logical pieces. --- src/qemu/qemu_driver.c | 79 +++++++++++++++++++++++++++++--------------------- 1 file changed, 46 insertions(+), 33 deletions(-)
Reviewed-by: John Ferlan <jferlan@redhat.com> John
Use virStorageSource accessors to check the file and call virStorageFileAccess before even attempting to stat the target. This will be helpful once we try to add network destinations for block copy, since there will be no need to stat them. --- src/qemu/qemu_driver.c | 53 +++++++++++++++++++++++++++++++------------------- 1 file changed, 33 insertions(+), 20 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 5beb14e64..f93c5cb21 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -16693,36 +16693,49 @@ qemuDomainBlockCopyValidateMirror(virStorageSourcePtr mirror, int desttype = virStorageSourceGetActualType(mirror); struct stat st; - if (stat(mirror->path, &st) < 0) { + if (virStorageFileAccess(mirror, F_OK) < 0) { if (errno != ENOENT) { - virReportSystemError(errno, _("unable to stat for disk %s: %s"), - dst, mirror->path); + virReportSystemError(errno, "%s", + _("unable to verify existance of " + "block copy target")); return -1; - } else if (*reuse || desttype == VIR_STORAGE_TYPE_BLOCK) { + } + + if (*reuse || desttype == VIR_STORAGE_TYPE_BLOCK) { virReportSystemError(errno, _("missing destination file for disk %s: %s"), dst, mirror->path); return -1; } - } else if (!S_ISBLK(st.st_mode)) { - if (st.st_size && !(*reuse)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("external destination file for disk %s already " - "exists and is not a block device: %s"), - dst, mirror->path); + } else { + if (virStorageFileStat(mirror, &st) < 0) { + virReportSystemError(errno, + _("unable to stat block copy target '%s'"), + mirror->path); return -1; } - 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"), - dst, mirror->path); - return -1; + + if (S_ISBLK(st.st_mode)) { + /* if the target is a block device, assume that we are reusing it, + * so there are no attempts to create it */ + *reuse = true; + } else { + if (st.st_size && !(*reuse)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("external destination file for disk %s already " + "exists and is not a block device: %s"), + dst, mirror->path); + return -1; + } + + 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"), + dst, mirror->path); + return -1; + } } - } else { - /* if the target is a block device, assume that we are reusing it, so - * there are no attempts to create it */ - *reuse = true; } return 0; -- 2.12.2
On 07/11/2017 11:46 AM, Peter Krempa wrote:
Use virStorageSource accessors to check the file and call virStorageFileAccess before even attempting to stat the target. This will be helpful once we try to add network destinations for block copy, since there will be no need to stat them. --- src/qemu/qemu_driver.c | 53 +++++++++++++++++++++++++++++++------------------- 1 file changed, 33 insertions(+), 20 deletions(-)
Reviewed-by: John Ferlan <jferlan@redhat.com> John
participants (2)
-
John Ferlan -
Peter Krempa