10:46 a.m.
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
10:46 a.m.
New subject: [libvirt] [PATCH 1/5] qemu: driver: Split out access to VIR_DOMAIN_BLOCK_COPY_REUSE_EXT
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
4:41 p.m.
New subject: [libvirt] [PATCH 1/5] qemu: driver: Split out access to VIR_DOMAIN_BLOCK_COPY_REUSE_EXT
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(a)redhat.com>
John
10:46 a.m.
New subject: [libvirt] [PATCH 2/5] qemu: blockcopy: Explicitly assert 'reuse' for block devices
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
4:43 p.m.
New subject: [libvirt] [PATCH 2/5] qemu: blockcopy: Explicitly assert 'reuse' for block devices
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(a)redhat.com>
John
10:46 a.m.
New subject: [libvirt] [PATCH 3/5] qemu: blockcopy: reuse storage driver APIs to pre-create copy target
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
4:44 p.m.
New subject: [libvirt] [PATCH 3/5] qemu: blockcopy: reuse storage driver APIs to pre-create copy target
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(a)redhat.com>
John
2:40 a.m.
New subject: [libvirt] [PATCH 3/5] qemu: blockcopy: reuse storage driver APIs to pre-create copy target
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(a)redhat.com>
Thanks
10:46 a.m.
New subject: [libvirt] [PATCH 4/5] qemu: blockcopy: Split out checking of the target image file
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
4:45 p.m.
New subject: [libvirt] [PATCH 4/5] qemu: blockcopy: Split out checking of the target image file
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(a)redhat.com>
John
10:46 a.m.
New subject: [libvirt] [PATCH 5/5] qemu: blockcopy: Refactor logic checking the target storage file
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
4:52 p.m.
New subject: [libvirt] [PATCH 5/5] qemu: blockcopy: Refactor logic checking the target storage file
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(a)redhat.com>
John
2682
days inactive
2691
days old
11 comments
2 participants
participants (2)
-
John Ferlan -
Peter Krempa