Re: [libvirt] [PATCH v2 02/22] qemu: Introduce qemuBlockJobUpdate
On Wed, Jun 10, 2015 at 07:21:27 -0400, John Ferlan wrote:
On 06/02/2015 08:34 AM, Jiri Denemark wrote:
The wrapper is useful for calling qemuBlockJobEventProcess with the
event details stored in disk's privateData, which is the most likely
usage of qemuBlockJobEventProcess.
Signed-off-by: Jiri Denemark jdene...@redhat.com
---
...
diff --git a/src/qemu/qemu_blockjob.c b/src/qemu/qemu_blockjob.c
index 098a43a..605c2a5 100644
--- a/src/qemu/qemu_blockjob.c
+++ b/src/qemu/qemu_blockjob.c
...
@@ -218,9 +243,7 @@ qemuBlockJobSyncEnd(virQEMUDriverPtr driver,
if (diskPriv-blockJobSync diskPriv-blockJobStatus != -1) {
if (ret_status)
*ret_status = diskPriv-blockJobStatus;
-qemuBlockJobEventProcess(driver, vm, disk,
- diskPriv-blockJobType,
- diskPriv-blockJobStatus);
+qemuBlockJobUpdate(driver, vm, disk);
^^
This doesn't get the returned status...
qemuBlockJobUpdate returns the original value of
diskPriv-blockJobStatus, which is already stored to *ret_status above.
Not to mention that one of the following patches will completely remove
ret_status from qemuBlockJobSyncEnd.
Jirka
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 02/22] qemu: Introduce qemuBlockJobUpdate
On 06/02/2015 08:34 AM, Jiri Denemark wrote:
The wrapper is useful for calling qemuBlockJobEventProcess with the
event details stored in disk's privateData, which is the most likely
usage of qemuBlockJobEventProcess.
Signed-off-by: Jiri Denemark jdene...@redhat.com
---
Notes:
Already ACKed in version 1.
Version 2:
- no changes
src/libvirt_private.syms | 2 ++
src/qemu/qemu_blockjob.c | 37 +
src/qemu/qemu_blockjob.h | 3 +++
3 files changed, 34 insertions(+), 8 deletions(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 9076135..8846dea 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -265,6 +265,8 @@ virDomainDiskInsert;
virDomainDiskInsertPreAlloced;
virDomainDiskIoTypeFromString;
virDomainDiskIoTypeToString;
+virDomainDiskMirrorStateTypeFromString;
+virDomainDiskMirrorStateTypeToString;
virDomainDiskPathByName;
virDomainDiskRemove;
virDomainDiskRemoveByName;
diff --git a/src/qemu/qemu_blockjob.c b/src/qemu/qemu_blockjob.c
index 098a43a..605c2a5 100644
--- a/src/qemu/qemu_blockjob.c
+++ b/src/qemu/qemu_blockjob.c
@@ -38,6 +38,27 @@
VIR_LOG_INIT(qemu.qemu_blockjob);
+
+int
+qemuBlockJobUpdate(virQEMUDriverPtr driver,
+ virDomainObjPtr vm,
+ virDomainDiskDefPtr disk)
+{
+qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk);
+int ret;
+
+if ((ret = diskPriv-blockJobStatus) == -1)
+return -1;
+
+qemuBlockJobEventProcess(driver, vm, disk,
+ diskPriv-blockJobType,
+ diskPriv-blockJobStatus);
+diskPriv-blockJobStatus = -1;
+
+return ret;
+}
+
+
/**
* qemuBlockJobEventProcess:
* @driver: qemu driver
@@ -49,8 +70,6 @@ VIR_LOG_INIT(qemu.qemu_blockjob);
* Update disk's mirror state in response to a block job event
* from QEMU. For mirror state's that must survive libvirt
* restart, also update the domain's status XML.
- *
- * Returns 0 on success, -1 otherwise.
*/
void
qemuBlockJobEventProcess(virQEMUDriverPtr driver,
@@ -67,6 +86,12 @@ qemuBlockJobEventProcess(virQEMUDriverPtr driver,
bool save = false;
qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk);
+VIR_DEBUG(disk=%s, mirrorState=%s, type=%d, status=%d,
+ disk-dst,
+
NULLSTR(virDomainDiskMirrorStateTypeToString(disk-mirrorState)),
+ type,
+ status);
+
/* Have to generate two variants of the event for old vs. new
* client callbacks */
if (type == VIR_DOMAIN_BLOCK_JOB_TYPE_COMMIT
@@ -218,9 +243,7 @@ qemuBlockJobSyncEnd(virQEMUDriverPtr driver,
if (diskPriv-blockJobSync diskPriv-blockJobStatus != -1) {
if (ret_status)
*ret_status = diskPriv-blockJobStatus;
-qemuBlockJobEventProcess(driver, vm, disk,
- diskPriv-blockJobType,
- diskPriv-blockJobStatus);
+qemuBlockJobUpdate(driver, vm, disk);
^^
This doesn't get the returned status...
John
diskPriv-blockJobStatus = -1;
}
diskPriv-blockJobSync = false;
@@ -300,9 +323,7 @@ qemuBlockJobSyncWaitWithTimeout(virQEMUDriverPtr driver,
if (ret_status)
*ret_status = diskPriv-blockJobStatus;
-qemuBlockJobEventProcess(driver, vm, disk,
- diskPriv-blockJobType,
- diskPriv-blockJobStatus);
+qemuBlockJobUpdate(driver, vm, disk);
diskPriv-blockJobStatus = -1;
return 0;
diff --git a/src/qemu/qemu_blockjob.h b/src/qemu/qemu_blockjob.h
index ba372a2..81e893e 100644
--- a/src/qemu/qemu_blockjob.h
+++ b/src/qemu/qemu_blockjob.h
@@ -25,6 +25,9 @@
# include internal.h
# include qemu_conf.h
+int qemuBlockJobUpdate(virQEMUDriverPtr driver,
+ virDomainObjPtr vm,
+ virDomainDiskDefPtr disk);
void qemuBlockJobEventProcess(virQEMUDriverPtr driver,
virDomainObjPtr vm,
virDomainDiskDefPtr disk,
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 02/22] qemu: Introduce qemuBlockJobUpdate
On 06/10/2015 07:31 AM, Jiri Denemark wrote:
On Wed, Jun 10, 2015 at 07:21:27 -0400, John Ferlan wrote:
On 06/02/2015 08:34 AM, Jiri Denemark wrote:
The wrapper is useful for calling qemuBlockJobEventProcess with the
event details stored in disk's privateData, which is the most likely
usage of qemuBlockJobEventProcess.
Signed-off-by: Jiri Denemark jdene...@redhat.com
---
...
diff --git a/src/qemu/qemu_blockjob.c b/src/qemu/qemu_blockjob.c
index 098a43a..605c2a5 100644
--- a/src/qemu/qemu_blockjob.c
+++ b/src/qemu/qemu_blockjob.c
...
@@ -218,9 +243,7 @@ qemuBlockJobSyncEnd(virQEMUDriverPtr driver,
if (diskPriv-blockJobSync diskPriv-blockJobStatus != -1) {
if (ret_status)
*ret_status = diskPriv-blockJobStatus;
-qemuBlockJobEventProcess(driver, vm, disk,
- diskPriv-blockJobType,
- diskPriv-blockJobStatus);
+qemuBlockJobUpdate(driver, vm, disk);
^^
This doesn't get the returned status...
qemuBlockJobUpdate returns the original value of
diskPriv-blockJobStatus, which is already stored to *ret_status above.
Not to mention that one of the following patches will completely remove
ret_status from qemuBlockJobSyncEnd.
I was just perusing to 'learn' a bit about the code... I hadn't looked
forward yet.
In essence it's a void function though... I see that qemuBlockJobUpdate
checks/sets blockJobStatus = -1 and the same setting is done right after
the call.
John
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 02/22] qemu: Introduce qemuBlockJobUpdate
On Tue, Jun 02, 2015 at 14:34:07 +0200, Jiri Denemark wrote: The wrapper is useful for calling qemuBlockJobEventProcess with the event details stored in disk's privateData, which is the most likely usage of qemuBlockJobEventProcess. Signed-off-by: Jiri Denemark jdene...@redhat.com --- Notes: Already ACKed in version 1. Version 2: - no changes src/libvirt_private.syms | 2 ++ src/qemu/qemu_blockjob.c | 37 + src/qemu/qemu_blockjob.h | 3 +++ 3 files changed, 34 insertions(+), 8 deletions(-) ACK stands. Peter signature.asc Description: Digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 02/22] qemu: Introduce qemuBlockJobUpdate
On Tue, Jun 02, 2015 at 14:34:07 +0200, Jiri Denemark wrote:
The wrapper is useful for calling qemuBlockJobEventProcess with the
event details stored in disk's privateData, which is the most likely
usage of qemuBlockJobEventProcess.
Signed-off-by: Jiri Denemark jdene...@redhat.com
---
Notes:
Already ACKed in version 1.
Version 2:
- no changes
src/libvirt_private.syms | 2 ++
src/qemu/qemu_blockjob.c | 37 +
src/qemu/qemu_blockjob.h | 3 +++
3 files changed, 34 insertions(+), 8 deletions(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 9076135..8846dea 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -265,6 +265,8 @@ virDomainDiskInsert;
virDomainDiskInsertPreAlloced;
virDomainDiskIoTypeFromString;
virDomainDiskIoTypeToString;
+virDomainDiskMirrorStateTypeFromString;
+virDomainDiskMirrorStateTypeToString;
virDomainDiskPathByName;
virDomainDiskRemove;
virDomainDiskRemoveByName;
diff --git a/src/qemu/qemu_blockjob.c b/src/qemu/qemu_blockjob.c
index 098a43a..605c2a5 100644
--- a/src/qemu/qemu_blockjob.c
+++ b/src/qemu/qemu_blockjob.c
@@ -38,6 +38,27 @@
VIR_LOG_INIT(qemu.qemu_blockjob);
+
+int
+qemuBlockJobUpdate(virQEMUDriverPtr driver,
+ virDomainObjPtr vm,
+ virDomainDiskDefPtr disk)
+{
+qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk);
+int ret;
+
+if ((ret = diskPriv-blockJobStatus) == -1)
+return -1;
+
+qemuBlockJobEventProcess(driver, vm, disk,
+ diskPriv-blockJobType,
+ diskPriv-blockJobStatus);
+diskPriv-blockJobStatus = -1;
+
+return ret;
+}
While reading this function the second time I realized that the control
flow looks weird.
How about:
int
qemuBlockJobUpdate(virQEMUDriverPtr driver,
virDomainObjPtr vm,
virDomainDiskDefPtr disk)
{
qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk);
int ret = diskPriv-blockJobStatus;
if (diskPriv-blockJobStatus != -1) {
qemuBlockJobEventProcess(driver, vm, disk,
diskPriv-blockJobType,
diskPriv-blockJobStatus);
diskPriv-blockJobStatus = -1;
}
return ret;
}
Peter
signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 02/22] qemu: Introduce qemuBlockJobUpdate
On Tue, Jun 09, 2015 at 16:56:34 +0200, Peter Krempa wrote:
On Tue, Jun 02, 2015 at 14:34:07 +0200, Jiri Denemark wrote:
The wrapper is useful for calling qemuBlockJobEventProcess with the
event details stored in disk's privateData, which is the most likely
usage of qemuBlockJobEventProcess.
Signed-off-by: Jiri Denemark jdene...@redhat.com
---
Notes:
Already ACKed in version 1.
Version 2:
- no changes
src/libvirt_private.syms | 2 ++
src/qemu/qemu_blockjob.c | 37 +
src/qemu/qemu_blockjob.h | 3 +++
3 files changed, 34 insertions(+), 8 deletions(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 9076135..8846dea 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -265,6 +265,8 @@ virDomainDiskInsert;
virDomainDiskInsertPreAlloced;
virDomainDiskIoTypeFromString;
virDomainDiskIoTypeToString;
+virDomainDiskMirrorStateTypeFromString;
+virDomainDiskMirrorStateTypeToString;
virDomainDiskPathByName;
virDomainDiskRemove;
virDomainDiskRemoveByName;
diff --git a/src/qemu/qemu_blockjob.c b/src/qemu/qemu_blockjob.c
index 098a43a..605c2a5 100644
--- a/src/qemu/qemu_blockjob.c
+++ b/src/qemu/qemu_blockjob.c
@@ -38,6 +38,27 @@
VIR_LOG_INIT(qemu.qemu_blockjob);
+
+int
+qemuBlockJobUpdate(virQEMUDriverPtr driver,
+ virDomainObjPtr vm,
+ virDomainDiskDefPtr disk)
+{
+qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk);
+int ret;
+
+if ((ret = diskPriv-blockJobStatus) == -1)
+return -1;
+
+qemuBlockJobEventProcess(driver, vm, disk,
+ diskPriv-blockJobType,
+ diskPriv-blockJobStatus);
+diskPriv-blockJobStatus = -1;
+
+return ret;
+}
While reading this function the second time I realized that the control
flow looks weird.
How about:
int
qemuBlockJobUpdate(virQEMUDriverPtr driver,
virDomainObjPtr vm,
virDomainDiskDefPtr disk)
{
qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk);
int ret = diskPriv-blockJobStatus;
if (diskPriv-blockJobStatus != -1) {
qemuBlockJobEventProcess(driver, vm, disk,
diskPriv-blockJobType,
diskPriv-blockJobStatus);
diskPriv-blockJobStatus = -1;
}
return ret;
}
Yeah, although I will also rename ret to status since the name
implicitly suggests semantics of -1... anyone seeing ret = -1 would
consider it a failure. But this function does not fail, it just returns
the original value stored in blockJobStatus.
Jirka
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 02/22] qemu: Introduce qemuBlockJobUpdate
The wrapper is useful for calling qemuBlockJobEventProcess with the
event details stored in disk's privateData, which is the most likely
usage of qemuBlockJobEventProcess.
Signed-off-by: Jiri Denemark jdene...@redhat.com
---
Notes:
Already ACKed in version 1.
Version 2:
- no changes
src/libvirt_private.syms | 2 ++
src/qemu/qemu_blockjob.c | 37 +
src/qemu/qemu_blockjob.h | 3 +++
3 files changed, 34 insertions(+), 8 deletions(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 9076135..8846dea 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -265,6 +265,8 @@ virDomainDiskInsert;
virDomainDiskInsertPreAlloced;
virDomainDiskIoTypeFromString;
virDomainDiskIoTypeToString;
+virDomainDiskMirrorStateTypeFromString;
+virDomainDiskMirrorStateTypeToString;
virDomainDiskPathByName;
virDomainDiskRemove;
virDomainDiskRemoveByName;
diff --git a/src/qemu/qemu_blockjob.c b/src/qemu/qemu_blockjob.c
index 098a43a..605c2a5 100644
--- a/src/qemu/qemu_blockjob.c
+++ b/src/qemu/qemu_blockjob.c
@@ -38,6 +38,27 @@
VIR_LOG_INIT(qemu.qemu_blockjob);
+
+int
+qemuBlockJobUpdate(virQEMUDriverPtr driver,
+ virDomainObjPtr vm,
+ virDomainDiskDefPtr disk)
+{
+qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk);
+int ret;
+
+if ((ret = diskPriv-blockJobStatus) == -1)
+return -1;
+
+qemuBlockJobEventProcess(driver, vm, disk,
+ diskPriv-blockJobType,
+ diskPriv-blockJobStatus);
+diskPriv-blockJobStatus = -1;
+
+return ret;
+}
+
+
/**
* qemuBlockJobEventProcess:
* @driver: qemu driver
@@ -49,8 +70,6 @@ VIR_LOG_INIT(qemu.qemu_blockjob);
* Update disk's mirror state in response to a block job event
* from QEMU. For mirror state's that must survive libvirt
* restart, also update the domain's status XML.
- *
- * Returns 0 on success, -1 otherwise.
*/
void
qemuBlockJobEventProcess(virQEMUDriverPtr driver,
@@ -67,6 +86,12 @@ qemuBlockJobEventProcess(virQEMUDriverPtr driver,
bool save = false;
qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk);
+VIR_DEBUG(disk=%s, mirrorState=%s, type=%d, status=%d,
+ disk-dst,
+ NULLSTR(virDomainDiskMirrorStateTypeToString(disk-mirrorState)),
+ type,
+ status);
+
/* Have to generate two variants of the event for old vs. new
* client callbacks */
if (type == VIR_DOMAIN_BLOCK_JOB_TYPE_COMMIT
@@ -218,9 +243,7 @@ qemuBlockJobSyncEnd(virQEMUDriverPtr driver,
if (diskPriv-blockJobSync diskPriv-blockJobStatus != -1) {
if (ret_status)
*ret_status = diskPriv-blockJobStatus;
-qemuBlockJobEventProcess(driver, vm, disk,
- diskPriv-blockJobType,
- diskPriv-blockJobStatus);
+qemuBlockJobUpdate(driver, vm, disk);
diskPriv-blockJobStatus = -1;
}
diskPriv-blockJobSync = false;
@@ -300,9 +323,7 @@ qemuBlockJobSyncWaitWithTimeout(virQEMUDriverPtr driver,
if (ret_status)
*ret_status = diskPriv-blockJobStatus;
-qemuBlockJobEventProcess(driver, vm, disk,
- diskPriv-blockJobType,
- diskPriv-blockJobStatus);
+qemuBlockJobUpdate(driver, vm, disk);
diskPriv-blockJobStatus = -1;
return 0;
diff --git a/src/qemu/qemu_blockjob.h b/src/qemu/qemu_blockjob.h
index ba372a2..81e893e 100644
--- a/src/qemu/qemu_blockjob.h
+++ b/src/qemu/qemu_blockjob.h
@@ -25,6 +25,9 @@
# include internal.h
# include qemu_conf.h
+int qemuBlockJobUpdate(virQEMUDriverPtr driver,
+ virDomainObjPtr vm,
+ virDomainDiskDefPtr disk);
void qemuBlockJobEventProcess(virQEMUDriverPtr driver,
virDomainObjPtr vm,
virDomainDiskDefPtr disk,
--
2.4.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
