10:17 a.m.
Pavel Hrdina (4):
virCondWaitUntil: add another return value
virDomainObjSignal: drop this function
monitor: detect that eject fails because the tray is locked
qemu_hotplug: try harder to eject media
src/conf/domain_conf.c | 27 ++++++++--------
src/conf/domain_conf.h | 1 -
src/libvirt_private.syms | 1 -
src/qemu/qemu_hotplug.c | 73 +++++++++++++++++++++-----------------------
src/qemu/qemu_monitor_json.c | 14 +++++++++
src/qemu/qemu_monitor_text.c | 10 ++++++
src/qemu/qemu_process.c | 6 ++--
7 files changed, 77 insertions(+), 55 deletions(-)
--
2.4.4
10:17 a.m.
New subject: [libvirt] [PATCH v2 1/4] virCondWaitUntil: add another return value
We should distinguish between success and timeout, to let the user
handle those two events differently.
Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com>
---
src/conf/domain_conf.c | 20 +++++++++++++++-----
1 file changed, 15 insertions(+), 5 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 2c3b96b..988818c 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -2686,15 +2686,25 @@ virDomainObjWait(virDomainObjPtr vm)
}
+/**
+ * Waits for domain condition to be triggered for a specific period of time.
+ *
+ * Returns:
+ * -1 in case of error
+ * 0 on success
+ * 1 on timeout
+ */
int
virDomainObjWaitUntil(virDomainObjPtr vm,
unsigned long long whenms)
{
- if (virCondWaitUntil(&vm->cond, &vm->parent.lock, whenms) < 0
&&
- errno != ETIMEDOUT) {
- virReportSystemError(errno, "%s",
- _("failed to wait for domain condition"));
- return -1;
+ if (virCondWaitUntil(&vm->cond, &vm->parent.lock, whenms) < 0) {
+ if (errno != ETIMEDOUT) {
+ virReportSystemError(errno, "%s",
+ _("failed to wait for domain condition"));
+ return -1;
+ }
+ return 1;
}
return 0;
}
--
2.4.4
10:17 a.m.
New subject: [libvirt] [PATCH v2 2/4] virDomainObjSignal: drop this function
There are multiple consumers for the domain condition and we should
always wake them all.
Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com>
---
src/conf/domain_conf.c | 7 -------
src/conf/domain_conf.h | 1 -
src/libvirt_private.syms | 1 -
src/qemu/qemu_process.c | 4 ++--
4 files changed, 2 insertions(+), 11 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 988818c..9efe175 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -2661,13 +2661,6 @@ virDomainObjEndAPI(virDomainObjPtr *vm)
void
-virDomainObjSignal(virDomainObjPtr vm)
-{
- virCondSignal(&vm->cond);
-}
-
-
-void
virDomainObjBroadcast(virDomainObjPtr vm)
{
virCondBroadcast(&vm->cond);
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index aeba5a5..59655c1 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -2442,7 +2442,6 @@ void virDomainObjEndAPI(virDomainObjPtr *vm);
bool virDomainObjTaint(virDomainObjPtr obj,
virDomainTaintFlags taint);
-void virDomainObjSignal(virDomainObjPtr vm);
void virDomainObjBroadcast(virDomainObjPtr vm);
int virDomainObjWait(virDomainObjPtr vm);
int virDomainObjWaitUntil(virDomainObjPtr vm,
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 1566d11..720afdf 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -412,7 +412,6 @@ virDomainObjParseNode;
virDomainObjSetDefTransient;
virDomainObjSetMetadata;
virDomainObjSetState;
-virDomainObjSignal;
virDomainObjTaint;
virDomainObjUpdateModificationImpact;
virDomainObjWait;
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index ba84182..a688feb 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -1004,7 +1004,7 @@ qemuProcessHandleBlockJob(qemuMonitorPtr mon ATTRIBUTE_UNUSED,
/* We have a SYNC API waiting for this event, dispatch it back */
diskPriv->blockJobType = type;
diskPriv->blockJobStatus = status;
- virDomainObjSignal(vm);
+ virDomainObjBroadcast(vm);
} else {
/* there is no waiting SYNC API, dispatch the update to a thread */
if (VIR_ALLOC(processEvent) < 0)
@@ -1500,7 +1500,7 @@ qemuProcessHandleSpiceMigrated(qemuMonitorPtr mon ATTRIBUTE_UNUSED,
}
priv->job.spiceMigrated = true;
- virDomainObjSignal(vm);
+ virDomainObjBroadcast(vm);
cleanup:
virObjectUnlock(vm);
--
2.4.4
4:29 p.m.
New subject: [libvirt] [PATCH v2 2/4] virDomainObjSignal: drop this function
On 06/29/2015 11:17 AM, Pavel Hrdina wrote:
There are multiple consumers for the domain condition and we should
always wake them all.
Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com>
---
src/conf/domain_conf.c | 7 -------
src/conf/domain_conf.h | 1 -
src/libvirt_private.syms | 1 -
src/qemu/qemu_process.c | 4 ++--
4 files changed, 2 insertions(+), 11 deletions(-)
Hmmm... This is perhaps quite a difference as you're now unblocking all
threads for HandleBlockJob and HandleSpiceMigrated.
Using virCondBroadcast means all threads waiting on the condition will
be unblocked as opposed to unblocking 'at least one of the threads' when
using virCondSignal - for some reason that just doesn't feel right.
I can certainly understand unblocking everything waiting for
HandleTrayChange since it's a single physical block and perhaps
HandleSpiceMigrate since that seems to be a single SPICE server
transferring things (such as perhaps ports). Whereas, it just feels like
there could be multiple causes for BlockJob wanting synchronized access.
I guess I'd like to read others input on this.
The other patches seem mostly reasonable - it's just this one that's
causing me to think longer and question things.
John
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 988818c..9efe175 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -2661,13 +2661,6 @@ virDomainObjEndAPI(virDomainObjPtr *vm)
void
-virDomainObjSignal(virDomainObjPtr vm)
-{
- virCondSignal(&vm->cond);
-}
-
-
-void
virDomainObjBroadcast(virDomainObjPtr vm)
{
virCondBroadcast(&vm->cond);
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index aeba5a5..59655c1 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -2442,7 +2442,6 @@ void virDomainObjEndAPI(virDomainObjPtr *vm);
bool virDomainObjTaint(virDomainObjPtr obj,
virDomainTaintFlags taint);
-void virDomainObjSignal(virDomainObjPtr vm);
void virDomainObjBroadcast(virDomainObjPtr vm);
int virDomainObjWait(virDomainObjPtr vm);
int virDomainObjWaitUntil(virDomainObjPtr vm,
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 1566d11..720afdf 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -412,7 +412,6 @@ virDomainObjParseNode;
virDomainObjSetDefTransient;
virDomainObjSetMetadata;
virDomainObjSetState;
-virDomainObjSignal;
virDomainObjTaint;
virDomainObjUpdateModificationImpact;
virDomainObjWait;
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index ba84182..a688feb 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -1004,7 +1004,7 @@ qemuProcessHandleBlockJob(qemuMonitorPtr mon ATTRIBUTE_UNUSED,
/* We have a SYNC API waiting for this event, dispatch it back */
diskPriv->blockJobType = type;
diskPriv->blockJobStatus = status;
- virDomainObjSignal(vm);
+ virDomainObjBroadcast(vm);
} else {
/* there is no waiting SYNC API, dispatch the update to a thread */
if (VIR_ALLOC(processEvent) < 0)
@@ -1500,7 +1500,7 @@ qemuProcessHandleSpiceMigrated(qemuMonitorPtr mon
ATTRIBUTE_UNUSED,
}
priv->job.spiceMigrated = true;
- virDomainObjSignal(vm);
+ virDomainObjBroadcast(vm);
cleanup:
virObjectUnlock(vm);
6:44 a.m.
New subject: [libvirt] [PATCH v2 2/4] virDomainObjSignal: drop this function
On Wed, Jul 01, 2015 at 17:29:09 -0400, John Ferlan wrote:
On 06/29/2015 11:17 AM, Pavel Hrdina wrote:
> There are multiple consumers for the domain condition and we should
> always wake them all.
>
> Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com>
> ---
> src/conf/domain_conf.c | 7 -------
> src/conf/domain_conf.h | 1 -
> src/libvirt_private.syms | 1 -
> src/qemu/qemu_process.c | 4 ++--
> 4 files changed, 2 insertions(+), 11 deletions(-)
>
Hmmm... This is perhaps quite a difference as you're now unblocking all
threads for HandleBlockJob and HandleSpiceMigrated.
Using virCondBroadcast means all threads waiting on the condition will
be unblocked as opposed to unblocking 'at least one of the threads' when
using virCondSignal - for some reason that just doesn't feel right.
Well, since a later patch in this series adds a code path that may
signal a condition asyncrhonously to any known combination of threads
and in the future we might add more than one waiting thread the change
is desired.
I can certainly understand unblocking everything waiting for
HandleTrayChange since it's a single physical block and perhaps
HandleSpiceMigrate since that seems to be a single SPICE server
transferring things (such as perhaps ports). Whereas, it just feels like
there could be multiple causes for BlockJob wanting synchronized access.
Every single instance of waiting needs to check a extra variable to
decide whether it should be unblocked, since pthreads may cause spurious
wakeup in some cases so the existing code is robust to that.
Peter
10:17 a.m.
New subject: [libvirt] [PATCH v2 3/4] monitor: detect that eject fails because the tray is locked
Modify the eject monitor functions to parse the return code and detect,
whether the error contains "is locked" to report this type of failure to
upper layers.
Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com>
---
src/qemu/qemu_monitor_json.c | 14 ++++++++++++++
src/qemu/qemu_monitor_text.c | 10 ++++++++++
2 files changed, 24 insertions(+)
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index d3e98d4..9aac342 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -44,6 +44,7 @@
#include "virprobe.h"
#include "virstring.h"
#include "cpu/cpu_x86.h"
+#include "c-strcasestr.h"
#ifdef WITH_DTRACE_PROBES
# include "libvirt_qemu_probes.h"
@@ -2116,6 +2117,14 @@ int qemuMonitorJSONSetCPU(qemuMonitorPtr mon,
}
+/**
+ * Run QMP command to eject a media from ejectable device.
+ *
+ * Returns:
+ * -2 on error, when the tray is locked
+ * -1 on all other errors
+ * 0 on success
+ */
int qemuMonitorJSONEjectMedia(qemuMonitorPtr mon,
const char *dev_name,
bool force)
@@ -2134,6 +2143,11 @@ int qemuMonitorJSONEjectMedia(qemuMonitorPtr mon,
if (ret == 0)
ret = qemuMonitorJSONCheckError(cmd, reply);
+ VIR_DEBUG("%s", virJSONValueToString(reply, false));
+
+ if (ret < 0 && c_strcasestr(virJSONValueToString(reply, false), "is
locked"))
+ ret = -2;
+
virJSONValueFree(cmd);
virJSONValueFree(reply);
return ret;
diff --git a/src/qemu/qemu_monitor_text.c b/src/qemu/qemu_monitor_text.c
index 2e77534..2aa0460 100644
--- a/src/qemu/qemu_monitor_text.c
+++ b/src/qemu/qemu_monitor_text.c
@@ -1169,6 +1169,14 @@ int qemuMonitorTextSetCPU(qemuMonitorPtr mon, int cpu, bool
online)
}
+/**
+ * Run HMP command to eject a media from ejectable device.
+ *
+ * Returns:
+ * -2 on error, when the tray is locked
+ * -1 on all other errors
+ * 0 on success
+ */
int qemuMonitorTextEjectMedia(qemuMonitorPtr mon,
const char *dev_name,
bool force)
@@ -1187,6 +1195,8 @@ int qemuMonitorTextEjectMedia(qemuMonitorPtr mon,
* device not found, device is locked ...
* No message is printed on success it seems */
if (c_strcasestr(reply, "device ")) {
+ if (c_strcasestr(reply, "is locked"))
+ ret = -2;
virReportError(VIR_ERR_OPERATION_FAILED,
_("could not eject media on %s: %s"), dev_name, reply);
goto cleanup;
--
2.4.4
10:17 a.m.
New subject: [libvirt] [PATCH v2 4/4] qemu_hotplug: try harder to eject media
Some guests lock the tray and QEMU eject command will simply fail to
eject the media. But the guest OS can handle this attempt to eject the
media and can unlock the tray and open it. In this case, we should try
again to actually eject the media.
If the first attempt fails to detect a tray_open we will fail with
error, from monitor. If we receive that event, we know, that the guest
properly reacted to the eject request, unlocked the tray and opened it.
In this case, we need to run the command again to actually eject the
media from the device. The reason to call it again is, that QEMU don't
wait for the guest to react and report an error, that the tray is
locked.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1147471
Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com>
---
src/qemu/qemu_hotplug.c | 73 +++++++++++++++++++++++--------------------------
src/qemu/qemu_process.c | 2 ++
2 files changed, 36 insertions(+), 39 deletions(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 0628964..17595b7 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -59,7 +59,7 @@
VIR_LOG_INIT("qemu.qemu_hotplug");
-#define CHANGE_MEDIA_RETRIES 10
+#define CHANGE_MEDIA_TIMEOUT 5000
/* Wait up to 5 seconds for device removal to finish. */
unsigned long long qemuDomainRemoveDeviceWaitTime = 1000ull * 5;
@@ -166,12 +166,13 @@ qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver,
virStorageSourcePtr newsrc,
bool force)
{
- int ret = -1;
+ int ret = -1, rc;
char *driveAlias = NULL;
qemuDomainObjPrivatePtr priv = vm->privateData;
- int retries = CHANGE_MEDIA_RETRIES;
const char *format = NULL;
char *sourcestr = NULL;
+ bool ejectRetry = false;
+ unsigned long long now;
if (!disk->info.alias) {
virReportError(VIR_ERR_INTERNAL_ERROR,
@@ -193,36 +194,31 @@ qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver,
if (!(driveAlias = qemuDeviceDriveHostAlias(disk, priv->qemuCaps)))
goto error;
- qemuDomainObjEnterMonitor(driver, vm);
- ret = qemuMonitorEjectMedia(priv->mon, driveAlias, force);
- if (qemuDomainObjExitMonitor(driver, vm) < 0) {
- ret = -1;
- goto cleanup;
- }
-
- if (ret < 0)
- goto error;
+ do {
+ qemuDomainObjEnterMonitor(driver, vm);
+ rc = qemuMonitorEjectMedia(priv->mon, driveAlias, force);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ goto cleanup;
- virObjectRef(vm);
- /* we don't want to report errors from media tray_open polling */
- while (retries) {
- if (disk->tray_status == VIR_DOMAIN_DISK_TRAY_OPEN)
- break;
+ if (rc == -2) {
+ /* we've already tried, error out */
+ if (ejectRetry)
+ goto error;
+ VIR_DEBUG("tray is locked, wait for the guest to unlock "
+ "the tray and try to eject it again");
+ ejectRetry = true;
+ } else if (rc < 0) {
+ goto error;
+ }
- retries--;
- virObjectUnlock(vm);
- VIR_DEBUG("Waiting 500ms for tray to open. Retries left %d", retries);
- usleep(500 * 1000); /* sleep 500ms */
- virObjectLock(vm);
- }
- virObjectUnref(vm);
+ if (virTimeMillisNow(&now) < 0)
+ goto error;
- if (retries <= 0) {
- virReportError(VIR_ERR_OPERATION_FAILED, "%s",
- _("Unable to eject media"));
- ret = -1;
- goto error;
- }
+ while (disk->tray_status != VIR_DOMAIN_DISK_TRAY_OPEN) {
+ if (virDomainObjWaitUntil(vm, now + CHANGE_MEDIA_TIMEOUT) != 0)
+ goto error;
+ }
+ } while (ejectRetry && rc != 0);
if (!virStorageSourceIsEmpty(newsrc)) {
if (qemuGetDriveSourceString(newsrc, conn, &sourcestr) < 0)
@@ -237,19 +233,17 @@ qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver,
}
}
qemuDomainObjEnterMonitor(driver, vm);
- ret = qemuMonitorChangeMedia(priv->mon,
- driveAlias,
- sourcestr,
- format);
- if (qemuDomainObjExitMonitor(driver, vm) < 0) {
- ret = -1;
+ rc = qemuMonitorChangeMedia(priv->mon,
+ driveAlias,
+ sourcestr,
+ format);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
goto cleanup;
- }
}
- virDomainAuditDisk(vm, disk->src, newsrc, "update", ret >= 0);
+ virDomainAuditDisk(vm, disk->src, newsrc, "update", rc >= 0);
- if (ret < 0)
+ if (rc < 0)
goto error;
/* remove the old source from shared device list */
@@ -259,6 +253,7 @@ qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver,
virStorageSourceFree(disk->src);
disk->src = newsrc;
newsrc = NULL;
+ ret = 0;
cleanup:
VIR_FREE(driveAlias);
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index a688feb..648ba00 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -1152,6 +1152,8 @@ qemuProcessHandleTrayChange(qemuMonitorPtr mon ATTRIBUTE_UNUSED,
VIR_WARN("Unable to save status on vm %s after tray moved event",
vm->def->name);
}
+
+ virDomainObjBroadcast(vm);
}
virObjectUnlock(vm);
--
2.4.4
4:39 p.m.
New subject: [libvirt] [PATCH v2 4/4] qemu_hotplug: try harder to eject media
On 06/29/2015 11:17 AM, Pavel Hrdina wrote:
Some guests lock the tray and QEMU eject command will simply fail to
eject the media. But the guest OS can handle this attempt to eject the
media and can unlock the tray and open it. In this case, we should try
again to actually eject the media.
If the first attempt fails to detect a tray_open we will fail with
error, from monitor. If we receive that event, we know, that the guest
properly reacted to the eject request, unlocked the tray and opened it.
In this case, we need to run the command again to actually eject the
media from the device. The reason to call it again is, that QEMU don't
wait for the guest to react and report an error, that the tray is
locked.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1147471
Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com>
---
src/qemu/qemu_hotplug.c | 73 +++++++++++++++++++++++--------------------------
src/qemu/qemu_process.c | 2 ++
2 files changed, 36 insertions(+), 39 deletions(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 0628964..17595b7 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -59,7 +59,7 @@
VIR_LOG_INIT("qemu.qemu_hotplug");
-#define CHANGE_MEDIA_RETRIES 10
+#define CHANGE_MEDIA_TIMEOUT 5000
/* Wait up to 5 seconds for device removal to finish. */
unsigned long long qemuDomainRemoveDeviceWaitTime = 1000ull * 5;
@@ -166,12 +166,13 @@ qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver,
virStorageSourcePtr newsrc,
bool force)
{
- int ret = -1;
+ int ret = -1, rc;
char *driveAlias = NULL;
qemuDomainObjPrivatePtr priv = vm->privateData;
- int retries = CHANGE_MEDIA_RETRIES;
const char *format = NULL;
char *sourcestr = NULL;
+ bool ejectRetry = false;
+ unsigned long long now;
if (!disk->info.alias) {
virReportError(VIR_ERR_INTERNAL_ERROR,
@@ -193,36 +194,31 @@ qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver,
if (!(driveAlias = qemuDeviceDriveHostAlias(disk, priv->qemuCaps)))
goto error;
- qemuDomainObjEnterMonitor(driver, vm);
- ret = qemuMonitorEjectMedia(priv->mon, driveAlias, force);
- if (qemuDomainObjExitMonitor(driver, vm) < 0) {
- ret = -1;
- goto cleanup;
- }
-
- if (ret < 0)
- goto error;
+ do {
+ qemuDomainObjEnterMonitor(driver, vm);
+ rc = qemuMonitorEjectMedia(priv->mon, driveAlias, force);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ goto cleanup;
- virObjectRef(vm);
- /* we don't want to report errors from media tray_open polling */
- while (retries) {
- if (disk->tray_status == VIR_DOMAIN_DISK_TRAY_OPEN)
- break;
+ if (rc == -2) {
+ /* we've already tried, error out */
+ if (ejectRetry)
+ goto error;
+ VIR_DEBUG("tray is locked, wait for the guest to unlock "
+ "the tray and try to eject it again");
+ ejectRetry = true;
+ } else if (rc < 0) {
+ goto error;
+ }
- retries--;
- virObjectUnlock(vm);
- VIR_DEBUG("Waiting 500ms for tray to open. Retries left %d",
retries);
- usleep(500 * 1000); /* sleep 500ms */
- virObjectLock(vm);
- }
- virObjectUnref(vm);
+ if (virTimeMillisNow(&now) < 0)
+ goto error;
- if (retries <= 0) {
- virReportError(VIR_ERR_OPERATION_FAILED, "%s",
- _("Unable to eject media"));
- ret = -1;
- goto error;
- }
+ while (disk->tray_status != VIR_DOMAIN_DISK_TRAY_OPEN) {
+ if (virDomainObjWaitUntil(vm, now + CHANGE_MEDIA_TIMEOUT) != 0)
+ goto error;
+ }
Seems this should be
if (rc == -2) wait for TRAY_MOVED (need a new event)
else wait for TRAY_OPEN
+ } while (ejectRetry && rc != 0);
if (!virStorageSourceIsEmpty(newsrc)) {
if (qemuGetDriveSourceString(newsrc, conn, &sourcestr) < 0)
@@ -237,19 +233,17 @@ qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver,
}
}
qemuDomainObjEnterMonitor(driver, vm);
- ret = qemuMonitorChangeMedia(priv->mon,
- driveAlias,
- sourcestr,
- format);
- if (qemuDomainObjExitMonitor(driver, vm) < 0) {
- ret = -1;
+ rc = qemuMonitorChangeMedia(priv->mon,
+ driveAlias,
+ sourcestr,
+ format);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
goto cleanup;
- }
}
- virDomainAuditDisk(vm, disk->src, newsrc, "update", ret >= 0);
+ virDomainAuditDisk(vm, disk->src, newsrc, "update", rc >= 0);
- if (ret < 0)
+ if (rc < 0)
goto error;
/* remove the old source from shared device list */
@@ -259,6 +253,7 @@ qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver,
virStorageSourceFree(disk->src);
disk->src = newsrc;
newsrc = NULL;
+ ret = 0;
cleanup:
VIR_FREE(driveAlias);
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index a688feb..648ba00 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -1152,6 +1152,8 @@ qemuProcessHandleTrayChange(qemuMonitorPtr mon ATTRIBUTE_UNUSED,
VIR_WARN("Unable to save status on vm %s after tray moved event",
vm->def->name);
}
Seems there should be some sort of DEVICE_TRAY_MOVED event... as well as
OPEN and CLOSE
John
+
+ virDomainObjBroadcast(vm);
}
virObjectUnlock(vm);
8:51 a.m.
New subject: [libvirt] [PATCH v2 4/4] qemu_hotplug: try harder to eject media
On Wed, Jul 01, 2015 at 05:39:49PM -0400, John Ferlan wrote:
On 06/29/2015 11:17 AM, Pavel Hrdina wrote:
> Some guests lock the tray and QEMU eject command will simply fail to
> eject the media. But the guest OS can handle this attempt to eject the
> media and can unlock the tray and open it. In this case, we should try
> again to actually eject the media.
>
> If the first attempt fails to detect a tray_open we will fail with
> error, from monitor. If we receive that event, we know, that the guest
> properly reacted to the eject request, unlocked the tray and opened it.
> In this case, we need to run the command again to actually eject the
> media from the device. The reason to call it again is, that QEMU don't
> wait for the guest to react and report an error, that the tray is
> locked.
>
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1147471
>
> Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com>
> ---
> src/qemu/qemu_hotplug.c | 73 +++++++++++++++++++++++--------------------------
> src/qemu/qemu_process.c | 2 ++
> 2 files changed, 36 insertions(+), 39 deletions(-)
>
> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index 0628964..17595b7 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -59,7 +59,7 @@
>
> VIR_LOG_INIT("qemu.qemu_hotplug");
>
> -#define CHANGE_MEDIA_RETRIES 10
> +#define CHANGE_MEDIA_TIMEOUT 5000
>
> /* Wait up to 5 seconds for device removal to finish. */
> unsigned long long qemuDomainRemoveDeviceWaitTime = 1000ull * 5;
> @@ -166,12 +166,13 @@ qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver,
> virStorageSourcePtr newsrc,
> bool force)
> {
> - int ret = -1;
> + int ret = -1, rc;
> char *driveAlias = NULL;
> qemuDomainObjPrivatePtr priv = vm->privateData;
> - int retries = CHANGE_MEDIA_RETRIES;
> const char *format = NULL;
> char *sourcestr = NULL;
> + bool ejectRetry = false;
> + unsigned long long now;
>
> if (!disk->info.alias) {
> virReportError(VIR_ERR_INTERNAL_ERROR,
> @@ -193,36 +194,31 @@ qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver,
> if (!(driveAlias = qemuDeviceDriveHostAlias(disk, priv->qemuCaps)))
> goto error;
>
> - qemuDomainObjEnterMonitor(driver, vm);
> - ret = qemuMonitorEjectMedia(priv->mon, driveAlias, force);
> - if (qemuDomainObjExitMonitor(driver, vm) < 0) {
> - ret = -1;
> - goto cleanup;
> - }
> -
> - if (ret < 0)
> - goto error;
> + do {
> + qemuDomainObjEnterMonitor(driver, vm);
> + rc = qemuMonitorEjectMedia(priv->mon, driveAlias, force);
> + if (qemuDomainObjExitMonitor(driver, vm) < 0)
> + goto cleanup;
>
> - virObjectRef(vm);
> - /* we don't want to report errors from media tray_open polling */
> - while (retries) {
> - if (disk->tray_status == VIR_DOMAIN_DISK_TRAY_OPEN)
> - break;
> + if (rc == -2) {
> + /* we've already tried, error out */
> + if (ejectRetry)
> + goto error;
> + VIR_DEBUG("tray is locked, wait for the guest to unlock "
> + "the tray and try to eject it again");
> + ejectRetry = true;
> + } else if (rc < 0) {
> + goto error;
> + }
>
> - retries--;
> - virObjectUnlock(vm);
> - VIR_DEBUG("Waiting 500ms for tray to open. Retries left %d",
retries);
> - usleep(500 * 1000); /* sleep 500ms */
> - virObjectLock(vm);
> - }
> - virObjectUnref(vm);
> + if (virTimeMillisNow(&now) < 0)
> + goto error;
>
> - if (retries <= 0) {
> - virReportError(VIR_ERR_OPERATION_FAILED, "%s",
> - _("Unable to eject media"));
> - ret = -1;
> - goto error;
> - }
> + while (disk->tray_status != VIR_DOMAIN_DISK_TRAY_OPEN) {
> + if (virDomainObjWaitUntil(vm, now + CHANGE_MEDIA_TIMEOUT) != 0)
> + goto error;
> + }
Seems this should be
if (rc == -2) wait for TRAY_MOVED (need a new event)
else wait for TRAY_OPEN
There is no event TRAY_OPEN or TRAY_CLOSE. QEMU has only one event,
DEVICE_TRAY_MOVED. This event is handled by qemuMonitorJSONHandleTrayChange,
which will set the reason to OPEN/CLOSE and emit a qemu monitor event. For this
qemu monitor event there is a handler called qemuProcessHandleTrayChange, where
the tray_status is set to VIR_DOMAIN_DISK_TRAY_OPEN or
VIR_DOMAIN_DISK_TRAY_CLOSED. This handler also wakes up that condition we are
waiting in this code. There is no need for a new event. We are already using
everything, that is available.
Pavel
> + } while (ejectRetry && rc != 0);
>
> if (!virStorageSourceIsEmpty(newsrc)) {
> if (qemuGetDriveSourceString(newsrc, conn, &sourcestr) < 0)
> @@ -237,19 +233,17 @@ qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver,
> }
> }
> qemuDomainObjEnterMonitor(driver, vm);
> - ret = qemuMonitorChangeMedia(priv->mon,
> - driveAlias,
> - sourcestr,
> - format);
> - if (qemuDomainObjExitMonitor(driver, vm) < 0) {
> - ret = -1;
> + rc = qemuMonitorChangeMedia(priv->mon,
> + driveAlias,
> + sourcestr,
> + format);
> + if (qemuDomainObjExitMonitor(driver, vm) < 0)
> goto cleanup;
> - }
> }
>
> - virDomainAuditDisk(vm, disk->src, newsrc, "update", ret >= 0);
> + virDomainAuditDisk(vm, disk->src, newsrc, "update", rc >= 0);
>
> - if (ret < 0)
> + if (rc < 0)
> goto error;
>
> /* remove the old source from shared device list */
> @@ -259,6 +253,7 @@ qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver,
> virStorageSourceFree(disk->src);
> disk->src = newsrc;
> newsrc = NULL;
> + ret = 0;
>
> cleanup:
> VIR_FREE(driveAlias);
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index a688feb..648ba00 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -1152,6 +1152,8 @@ qemuProcessHandleTrayChange(qemuMonitorPtr mon
ATTRIBUTE_UNUSED,
> VIR_WARN("Unable to save status on vm %s after tray moved
event",
> vm->def->name);
> }
Seems there should be some sort of DEVICE_TRAY_MOVED event... as well as
OPEN and CLOSE
John
> +
> + virDomainObjBroadcast(vm);
> }
>
> virObjectUnlock(vm);
>
8:46 a.m.
New subject: [libvirt] [PATCH v2 4/4] qemu_hotplug: try harder to eject media
On 07/03/2015 09:51 AM, Pavel Hrdina wrote:
On Wed, Jul 01, 2015 at 05:39:49PM -0400, John Ferlan wrote:
>
>
> On 06/29/2015 11:17 AM, Pavel Hrdina wrote:
>> Some guests lock the tray and QEMU eject command will simply fail to
>> eject the media. But the guest OS can handle this attempt to eject the
>> media and can unlock the tray and open it. In this case, we should try
>> again to actually eject the media.
>>
>> If the first attempt fails to detect a tray_open we will fail with
>> error, from monitor. If we receive that event, we know, that the guest
>> properly reacted to the eject request, unlocked the tray and opened it.
>> In this case, we need to run the command again to actually eject the
>> media from the device. The reason to call it again is, that QEMU don't
s/don't/doesn't
>> wait for the guest to react and report an error, that the
tray is
>> locked.
>>
>> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1147471
>>
>> Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com>
>> ---
>> src/qemu/qemu_hotplug.c | 73 +++++++++++++++++++++++--------------------------
>> src/qemu/qemu_process.c | 2 ++
>> 2 files changed, 36 insertions(+), 39 deletions(-)
>>
I suppose if I had gone back to the commit message and reread it in
light of what I was reading in the qemu-devel message linked in the bz
and what I saw in the code, then perhaps it would all make sense. Of
course the text you add below now makes it clear what the order of
processing is and why the while loop is used.
Personally I think this is a case where that commit message could be
placed into the code to make it "clearer" what is being done and why. I
know there are those against that because it's in the commit message or
perhaps some linked bug, but if I'm reading code and wondering what it
does, I'm not necessarily going back to the commit/bug that added the
code (OK, at least not at first).
Whether you add a comment into the code to indicate why this while loop
is important and how the processing works due to the event and locked
media...
ACK series,
John
>> diff --git a/src/qemu/qemu_hotplug.c
b/src/qemu/qemu_hotplug.c
>> index 0628964..17595b7 100644
>> --- a/src/qemu/qemu_hotplug.c
>> +++ b/src/qemu/qemu_hotplug.c
>> @@ -59,7 +59,7 @@
>>
>> VIR_LOG_INIT("qemu.qemu_hotplug");
>>
>> -#define CHANGE_MEDIA_RETRIES 10
>> +#define CHANGE_MEDIA_TIMEOUT 5000
>>
>> /* Wait up to 5 seconds for device removal to finish. */
>> unsigned long long qemuDomainRemoveDeviceWaitTime = 1000ull * 5;
>> @@ -166,12 +166,13 @@ qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver,
>> virStorageSourcePtr newsrc,
>> bool force)
>> {
>> - int ret = -1;
>> + int ret = -1, rc;
>> char *driveAlias = NULL;
>> qemuDomainObjPrivatePtr priv = vm->privateData;
>> - int retries = CHANGE_MEDIA_RETRIES;
>> const char *format = NULL;
>> char *sourcestr = NULL;
>> + bool ejectRetry = false;
>> + unsigned long long now;
>>
>> if (!disk->info.alias) {
>> virReportError(VIR_ERR_INTERNAL_ERROR,
>> @@ -193,36 +194,31 @@ qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver,
>> if (!(driveAlias = qemuDeviceDriveHostAlias(disk, priv->qemuCaps)))
>> goto error;
>>
>> - qemuDomainObjEnterMonitor(driver, vm);
>> - ret = qemuMonitorEjectMedia(priv->mon, driveAlias, force);
>> - if (qemuDomainObjExitMonitor(driver, vm) < 0) {
>> - ret = -1;
>> - goto cleanup;
>> - }
>> -
>> - if (ret < 0)
>> - goto error;
>> + do {
>> + qemuDomainObjEnterMonitor(driver, vm);
>> + rc = qemuMonitorEjectMedia(priv->mon, driveAlias, force);
>> + if (qemuDomainObjExitMonitor(driver, vm) < 0)
>> + goto cleanup;
>>
>> - virObjectRef(vm);
>> - /* we don't want to report errors from media tray_open polling */
>> - while (retries) {
>> - if (disk->tray_status == VIR_DOMAIN_DISK_TRAY_OPEN)
>> - break;
>> + if (rc == -2) {
>> + /* we've already tried, error out */
>> + if (ejectRetry)
>> + goto error;
>> + VIR_DEBUG("tray is locked, wait for the guest to unlock "
>> + "the tray and try to eject it again");
>> + ejectRetry = true;
>> + } else if (rc < 0) {
>> + goto error;
>> + }
>>
>> - retries--;
>> - virObjectUnlock(vm);
>> - VIR_DEBUG("Waiting 500ms for tray to open. Retries left %d",
retries);
>> - usleep(500 * 1000); /* sleep 500ms */
>> - virObjectLock(vm);
>> - }
>> - virObjectUnref(vm);
>> + if (virTimeMillisNow(&now) < 0)
>> + goto error;
>>
>> - if (retries <= 0) {
>> - virReportError(VIR_ERR_OPERATION_FAILED, "%s",
>> - _("Unable to eject media"));
>> - ret = -1;
>> - goto error;
>> - }
>> + while (disk->tray_status != VIR_DOMAIN_DISK_TRAY_OPEN) {
>> + if (virDomainObjWaitUntil(vm, now + CHANGE_MEDIA_TIMEOUT) != 0)
>> + goto error;
>> + }
>
> Seems this should be
>
> if (rc == -2) wait for TRAY_MOVED (need a new event)
> else wait for TRAY_OPEN
>
There is no event TRAY_OPEN or TRAY_CLOSE. QEMU has only one event,
DEVICE_TRAY_MOVED. This event is handled by qemuMonitorJSONHandleTrayChange,
which will set the reason to OPEN/CLOSE and emit a qemu monitor event. For this
qemu monitor event there is a handler called qemuProcessHandleTrayChange, where
the tray_status is set to VIR_DOMAIN_DISK_TRAY_OPEN or
VIR_DOMAIN_DISK_TRAY_CLOSED. This handler also wakes up that condition we are
waiting in this code. There is no need for a new event. We are already using
everything, that is available.
Pavel
3458
days inactive
3468
days old
9 comments
3 participants
participants (3)
-
John Ferlan -
Pavel Hrdina -
Peter Krempa