[libvirt] [PATCH 00/12] Remove devices only after DEVICE_DELETED event
This series (partially) fixes a longstanding bug in device unplug from a live domain. We considered the QEMU command used for that to be synchronous and removed the unplugged device from domain definition immediately after the command returned success. This is OK for USB devices but other devices actually need guest cooperation to be unplugged and thus they are actually unplugged asynchronously and QEMU tells us about that using DEVICE_DELETED event. This series is not complete, it does not check if any device finished unplug while libvirtd was not running. I'm working on that part but I wanted to get some feedback on this series as soon as possible. Path 3/12 explains how I decided to deal with backward compatibility of the virDomainDetachDeviceFlags API. No libvirt client/app should see any real difference in behavior after this series unless they want to. Jiri Denemark (12): Add VIR_DOMAIN_EVENT_ID_DEVICE_REMOVED event examples: Handle VIR_DOMAIN_EVENT_ID_DEVICE_REMOVED event Clarify virDomainDetachDeviceFlags documentation qemu: Add qemuDomainReleaseDeviceAddress to remove any address qemu: Separate disk device removal into a standalone function qemu: Separate controller removal into a standalone function qemu: Separate net device removal into a standalone function qemu: Separate host device removal into a standalone function Add virDomainDefFindDevice for looking up a device by its alias qemu: Add support for DEVICE_DELETED event qemu: Remove devices only after DEVICE_DELETED event qemu: Emit VIR_DOMAIN_EVENT_ID_DEVICE_REMOVED events daemon/remote.c | 32 + examples/domain-events/events-c/event-test.c | 23 +- examples/domain-events/events-python/event-test.py | 4 + include/libvirt/libvirt.h.in | 18 + python/libvirt-override-virConnect.py | 9 + python/libvirt-override.c | 52 +- src/conf/domain_conf.c | 41 ++ src/conf/domain_conf.h | 4 + src/conf/domain_event.c | 85 ++- src/conf/domain_event.h | 5 + src/libvirt.c | 12 + src/libvirt_private.syms | 3 + src/qemu/qemu_capabilities.c | 2 + src/qemu/qemu_capabilities.h | 1 + src/qemu/qemu_command.c | 35 +- src/qemu/qemu_command.h | 8 +- src/qemu/qemu_domain.h | 2 + src/qemu/qemu_driver.c | 31 +- src/qemu/qemu_hotplug.c | 660 ++++++++++++--------- src/qemu/qemu_hotplug.h | 7 +- src/qemu/qemu_monitor.c | 13 + src/qemu/qemu_monitor.h | 5 + src/qemu/qemu_monitor_json.c | 15 + src/qemu/qemu_process.c | 41 ++ src/remote/remote_driver.c | 32 + src/remote/remote_protocol.x | 13 +- src/remote_protocol-structs | 5 + 27 files changed, 847 insertions(+), 311 deletions(-) -- 1.8.3.2
--- daemon/remote.c | 32 +++++++++++++ include/libvirt/libvirt.h.in | 18 ++++++++ python/libvirt-override-virConnect.py | 9 ++++ python/libvirt-override.c | 52 ++++++++++++++++++++- src/conf/domain_event.c | 85 +++++++++++++++++++++++++++-------- src/conf/domain_event.h | 5 +++ src/libvirt_private.syms | 2 + src/remote/remote_driver.c | 32 +++++++++++++ src/remote/remote_protocol.x | 13 +++++- src/remote_protocol-structs | 5 +++ 10 files changed, 233 insertions(+), 20 deletions(-) diff --git a/daemon/remote.c b/daemon/remote.c index 5847e60..a10a308 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -600,6 +600,37 @@ static int remoteRelayDomainEventPMSuspendDisk(virConnectPtr conn ATTRIBUTE_UNUS return 0; } +static int +remoteRelayDomainEventDeviceRemoved(virConnectPtr conn ATTRIBUTE_UNUSED, + virDomainPtr dom, + const char *devAlias, + void *opaque) +{ + virNetServerClientPtr client = opaque; + remote_domain_event_device_removed_msg data; + + if (!client) + return -1; + + VIR_DEBUG("Relaying domain device removed event %s %d %s", + dom->name, dom->id, devAlias); + + /* build return data */ + memset(&data, 0, sizeof(data)); + + if (VIR_STRDUP(data.devAlias, devAlias) < 0) + return -1; + + make_nonnull_domain(&data.dom, dom); + + remoteDispatchDomainEventSend(client, remoteProgram, + REMOTE_PROC_DOMAIN_EVENT_DEVICE_REMOVED, + (xdrproc_t)xdr_remote_domain_event_device_removed_msg, + &data); + + return 0; +} + static virConnectDomainEventGenericCallback domainEventCallbacks[] = { VIR_DOMAIN_EVENT_CALLBACK(remoteRelayDomainEventLifecycle), @@ -617,6 +648,7 @@ static virConnectDomainEventGenericCallback domainEventCallbacks[] = { VIR_DOMAIN_EVENT_CALLBACK(remoteRelayDomainEventPMSuspend), VIR_DOMAIN_EVENT_CALLBACK(remoteRelayDomainEventBalloonChange), VIR_DOMAIN_EVENT_CALLBACK(remoteRelayDomainEventPMSuspendDisk), + VIR_DOMAIN_EVENT_CALLBACK(remoteRelayDomainEventDeviceRemoved), }; verify(ARRAY_CARDINALITY(domainEventCallbacks) == VIR_DOMAIN_EVENT_ID_LAST); diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index b87255a..bd7417f 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -4851,6 +4851,23 @@ typedef void (*virConnectDomainEventPMSuspendDiskCallback)(virConnectPtr conn, int reason, void *opaque); +/** + * virConnectDomainEventDeviceRemovedCallback: + * @conn: connection object + * @dom: domain on which the event occurred + * @devAlias: device alias + * @opaque: application specified data + * + * This callback occurs when a device is removed from the domain. + * + * The callback signature to use when registering for an event of type + * VIR_DOMAIN_EVENT_ID_DEVICE_REMOVED with virConnectDomainEventRegisterAny() + */ +typedef void (*virConnectDomainEventDeviceRemovedCallback)(virConnectPtr conn, + virDomainPtr dom, + const char *devAlias, + void *opaque); + /** * VIR_DOMAIN_EVENT_CALLBACK: @@ -4877,6 +4894,7 @@ typedef enum { VIR_DOMAIN_EVENT_ID_PMSUSPEND = 12, /* virConnectDomainEventPMSuspendCallback */ VIR_DOMAIN_EVENT_ID_BALLOON_CHANGE = 13, /* virConnectDomainEventBalloonChangeCallback */ VIR_DOMAIN_EVENT_ID_PMSUSPEND_DISK = 14, /* virConnectDomainEventPMSuspendDiskCallback */ + VIR_DOMAIN_EVENT_ID_DEVICE_REMOVED = 15, /* virConnectDomainEventDeviceRemovedCallback */ #ifdef VIR_ENUM_SENTINELS VIR_DOMAIN_EVENT_ID_LAST diff --git a/python/libvirt-override-virConnect.py b/python/libvirt-override-virConnect.py index 5495b70..f2b4b23 100644 --- a/python/libvirt-override-virConnect.py +++ b/python/libvirt-override-virConnect.py @@ -179,6 +179,15 @@ cb(self, virDomain(self, _obj=dom), reason, opaque) return 0 + def _dispatchDomainEventDeviceRemovedCallback(self, dom, devAlias, cbData): + """Dispatches event to python user domain device removed event callbacks + """ + cb = cbData["cb"] + opaque = cbData["opaque"] + + cb(self, virDomain(self, _obj=dom), devAlias, opaque) + return 0 + def domainEventDeregisterAny(self, callbackID): """Removes a Domain Event Callback. De-registering for a domain callback will disable delivery of this event type """ diff --git a/python/libvirt-override.c b/python/libvirt-override.c index 01c941e..2e94de9 100644 --- a/python/libvirt-override.c +++ b/python/libvirt-override.c @@ -6225,6 +6225,51 @@ libvirt_virConnectDomainEventPMSuspendDiskCallback(virConnectPtr conn ATTRIBUTE_ return ret; } +static int +libvirt_virConnectDomainEventDeviceRemovedCallback(virConnectPtr conn ATTRIBUTE_UNUSED, + virDomainPtr dom, + const char *devAlias, + void *opaque) +{ + PyObject *pyobj_cbData = (PyObject*)opaque; + PyObject *pyobj_dom; + PyObject *pyobj_ret; + PyObject *pyobj_conn; + PyObject *dictKey; + int ret = -1; + + LIBVIRT_ENSURE_THREAD_STATE; + /* Create a python instance of this virDomainPtr */ + virDomainRef(dom); + + pyobj_dom = libvirt_virDomainPtrWrap(dom); + Py_INCREF(pyobj_cbData); + + dictKey = libvirt_constcharPtrWrap("conn"); + pyobj_conn = PyDict_GetItem(pyobj_cbData, dictKey); + Py_DECREF(dictKey); + + /* Call the Callback Dispatcher */ + pyobj_ret = PyObject_CallMethod(pyobj_conn, + (char*)"_dispatchDomainEventDeviceRemovedCallback", + (char*)"OsO", + pyobj_dom, devAlias, pyobj_cbData); + + Py_DECREF(pyobj_cbData); + Py_DECREF(pyobj_dom); + + if (!pyobj_ret) { + DEBUG("%s - ret:%p\n", __FUNCTION__, pyobj_ret); + PyErr_Print(); + } else { + Py_DECREF(pyobj_ret); + ret = 0; + } + + LIBVIRT_RELEASE_THREAD_STATE; + return ret; +} + static PyObject * libvirt_virConnectDomainEventRegisterAny(ATTRIBUTE_UNUSED PyObject * self, PyObject * args) @@ -6254,7 +6299,7 @@ libvirt_virConnectDomainEventRegisterAny(ATTRIBUTE_UNUSED PyObject * self, else dom = PyvirDomain_Get(pyobj_dom); - switch (eventID) { + switch ((virDomainEventID) eventID) { case VIR_DOMAIN_EVENT_ID_LIFECYCLE: cb = VIR_DOMAIN_EVENT_CALLBACK(libvirt_virConnectDomainEventLifecycleCallback); break; @@ -6300,6 +6345,11 @@ libvirt_virConnectDomainEventRegisterAny(ATTRIBUTE_UNUSED PyObject * self, case VIR_DOMAIN_EVENT_ID_PMSUSPEND_DISK: cb = VIR_DOMAIN_EVENT_CALLBACK(libvirt_virConnectDomainEventPMSuspendDiskCallback); break; + case VIR_DOMAIN_EVENT_ID_DEVICE_REMOVED: + cb = VIR_DOMAIN_EVENT_CALLBACK(libvirt_virConnectDomainEventDeviceRemovedCallback); + + case VIR_DOMAIN_EVENT_ID_LAST: + break; } if (!cb) { diff --git a/src/conf/domain_event.c b/src/conf/domain_event.c index fde24be..640463c 100644 --- a/src/conf/domain_event.c +++ b/src/conf/domain_event.c @@ -122,6 +122,9 @@ struct _virDomainEvent { /* In unit of 1024 bytes */ unsigned long long actual; } balloonChange; + struct { + char *devAlias; + } deviceRemoved; } data; }; @@ -1157,6 +1160,44 @@ virDomainEventPtr virDomainEventBalloonChangeNewFromObj(virDomainObjPtr obj, return ev; } +static virDomainEventPtr +virDomainEventDeviceRemovedNew(int id, + const char *name, + unsigned char *uuid, + const char *devAlias) +{ + virDomainEventPtr ev = + virDomainEventNewInternal(VIR_DOMAIN_EVENT_ID_DEVICE_REMOVED, + id, name, uuid); + + if (ev) { + if (VIR_STRDUP(ev->data.deviceRemoved.devAlias, devAlias) < 0) + goto error; + } + + return ev; + +error: + virDomainEventFree(ev); + return NULL; +} + +virDomainEventPtr +virDomainEventDeviceRemovedNewFromObj(virDomainObjPtr obj, + const char *devAlias) +{ + return virDomainEventDeviceRemovedNew(obj->def->id, obj->def->name, + obj->def->uuid, devAlias); +} + +virDomainEventPtr +virDomainEventDeviceRemovedNewFromDom(virDomainPtr dom, + const char *devAlias) +{ + return virDomainEventDeviceRemovedNew(dom->id, dom->name, dom->uuid, + devAlias); +} + /** * virDomainEventQueuePush: * @evtQueue: the dom event queue @@ -1204,30 +1245,30 @@ virDomainEventDispatchDefaultFunc(virConnectPtr conn, return; dom->id = event->dom.id; - switch (event->eventID) { + switch ((virDomainEventID) event->eventID) { case VIR_DOMAIN_EVENT_ID_LIFECYCLE: ((virConnectDomainEventCallback)cb)(conn, dom, event->data.lifecycle.type, event->data.lifecycle.detail, cbopaque); - break; + goto cleanup; case VIR_DOMAIN_EVENT_ID_REBOOT: (cb)(conn, dom, cbopaque); - break; + goto cleanup; case VIR_DOMAIN_EVENT_ID_RTC_CHANGE: ((virConnectDomainEventRTCChangeCallback)cb)(conn, dom, event->data.rtcChange.offset, cbopaque); - break; + goto cleanup; case VIR_DOMAIN_EVENT_ID_WATCHDOG: ((virConnectDomainEventWatchdogCallback)cb)(conn, dom, event->data.watchdog.action, cbopaque); - break; + goto cleanup; case VIR_DOMAIN_EVENT_ID_IO_ERROR: ((virConnectDomainEventIOErrorCallback)cb)(conn, dom, @@ -1235,7 +1276,7 @@ virDomainEventDispatchDefaultFunc(virConnectPtr conn, event->data.ioError.devAlias, event->data.ioError.action, cbopaque); - break; + goto cleanup; case VIR_DOMAIN_EVENT_ID_IO_ERROR_REASON: ((virConnectDomainEventIOErrorReasonCallback)cb)(conn, dom, @@ -1244,7 +1285,7 @@ virDomainEventDispatchDefaultFunc(virConnectPtr conn, event->data.ioError.action, event->data.ioError.reason, cbopaque); - break; + goto cleanup; case VIR_DOMAIN_EVENT_ID_GRAPHICS: ((virConnectDomainEventGraphicsCallback)cb)(conn, dom, @@ -1254,12 +1295,12 @@ virDomainEventDispatchDefaultFunc(virConnectPtr conn, event->data.graphics.authScheme, event->data.graphics.subject, cbopaque); - break; + goto cleanup; case VIR_DOMAIN_EVENT_ID_CONTROL_ERROR: (cb)(conn, dom, cbopaque); - break; + goto cleanup; case VIR_DOMAIN_EVENT_ID_BLOCK_JOB: ((virConnectDomainEventBlockJobCallback)cb)(conn, dom, @@ -1267,7 +1308,7 @@ virDomainEventDispatchDefaultFunc(virConnectPtr conn, event->data.blockJob.type, event->data.blockJob.status, cbopaque); - break; + goto cleanup; case VIR_DOMAIN_EVENT_ID_DISK_CHANGE: ((virConnectDomainEventDiskChangeCallback)cb)(conn, dom, @@ -1276,38 +1317,46 @@ virDomainEventDispatchDefaultFunc(virConnectPtr conn, event->data.diskChange.devAlias, event->data.diskChange.reason, cbopaque); - break; + goto cleanup; case VIR_DOMAIN_EVENT_ID_TRAY_CHANGE: ((virConnectDomainEventTrayChangeCallback)cb)(conn, dom, event->data.trayChange.devAlias, event->data.trayChange.reason, cbopaque); - break; + goto cleanup; case VIR_DOMAIN_EVENT_ID_PMWAKEUP: ((virConnectDomainEventPMWakeupCallback)cb)(conn, dom, 0, cbopaque); - break; + goto cleanup; case VIR_DOMAIN_EVENT_ID_PMSUSPEND: ((virConnectDomainEventPMSuspendCallback)cb)(conn, dom, 0, cbopaque); - break; + goto cleanup; case VIR_DOMAIN_EVENT_ID_BALLOON_CHANGE: ((virConnectDomainEventBalloonChangeCallback)cb)(conn, dom, event->data.balloonChange.actual, cbopaque); - break; + goto cleanup; case VIR_DOMAIN_EVENT_ID_PMSUSPEND_DISK: ((virConnectDomainEventPMSuspendDiskCallback)cb)(conn, dom, 0, cbopaque); - break; + goto cleanup; - default: - VIR_WARN("Unexpected event ID %d", event->eventID); + case VIR_DOMAIN_EVENT_ID_DEVICE_REMOVED: + ((virConnectDomainEventDeviceRemovedCallback)cb)(conn, dom, + event->data.deviceRemoved.devAlias, + cbopaque); + goto cleanup; + + case VIR_DOMAIN_EVENT_ID_LAST: break; } + VIR_WARN("Unexpected event ID %d", event->eventID); + +cleanup: virDomainFree(dom); } diff --git a/src/conf/domain_event.h b/src/conf/domain_event.h index 5f64a47..f6b957d 100644 --- a/src/conf/domain_event.h +++ b/src/conf/domain_event.h @@ -132,6 +132,11 @@ virDomainEventPtr virDomainEventBalloonChangeNewFromObj(virDomainObjPtr obj, uns virDomainEventPtr virDomainEventPMSuspendDiskNewFromObj(virDomainObjPtr obj); virDomainEventPtr virDomainEventPMSuspendDiskNewFromDom(virDomainPtr dom); +virDomainEventPtr virDomainEventDeviceRemovedNewFromObj(virDomainObjPtr obj, + const char *devAlias); +virDomainEventPtr virDomainEventDeviceRemovedNewFromDom(virDomainPtr dom, + const char *devAlias); + void virDomainEventFree(virDomainEventPtr event); void virDomainEventStateFree(virDomainEventStatePtr state); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 0ab7632..49f07c1 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -405,6 +405,8 @@ virDomainEventBlockJobNewFromDom; virDomainEventBlockJobNewFromObj; virDomainEventControlErrorNewFromDom; virDomainEventControlErrorNewFromObj; +virDomainEventDeviceRemovedNewFromDom; +virDomainEventDeviceRemovedNewFromObj; virDomainEventDiskChangeNewFromDom; virDomainEventDiskChangeNewFromObj; virDomainEventFree; diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 81ecef1..b4bb926 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -268,6 +268,11 @@ remoteDomainBuildEventPMSuspendDisk(virNetClientProgramPtr prog, virNetClientPtr client, void *evdata, void *opaque); +static void +remoteDomainBuildEventDeviceRemoved(virNetClientProgramPtr prog, + virNetClientPtr client, + void *evdata, void *opaque); + static virNetClientProgramEvent remoteDomainEvents[] = { { REMOTE_PROC_DOMAIN_EVENT_RTC_CHANGE, remoteDomainBuildEventRTCChange, @@ -329,6 +334,10 @@ static virNetClientProgramEvent remoteDomainEvents[] = { remoteDomainBuildEventPMSuspendDisk, sizeof(remote_domain_event_pmsuspend_disk_msg), (xdrproc_t)xdr_remote_domain_event_pmsuspend_disk_msg }, + { REMOTE_PROC_DOMAIN_EVENT_DEVICE_REMOVED, + remoteDomainBuildEventDeviceRemoved, + sizeof(remote_domain_event_device_removed_msg), + (xdrproc_t)xdr_remote_domain_event_device_removed_msg }, }; enum virDrvOpenRemoteFlags { @@ -4695,6 +4704,29 @@ remoteDomainBuildEventPMSuspendDisk(virNetClientProgramPtr prog ATTRIBUTE_UNUSED } +static void +remoteDomainBuildEventDeviceRemoved(virNetClientProgramPtr prog ATTRIBUTE_UNUSED, + virNetClientPtr client ATTRIBUTE_UNUSED, + void *evdata, void *opaque) +{ + virConnectPtr conn = opaque; + struct private_data *priv = conn->privateData; + remote_domain_event_device_removed_msg *msg = evdata; + virDomainPtr dom; + virDomainEventPtr event = NULL; + + dom = get_nonnull_domain(conn, msg->dom); + if (!dom) + return; + + event = virDomainEventDeviceRemovedNewFromDom(dom, msg->devAlias); + + virDomainFree(dom); + + remoteDomainEventQueue(priv, event); +} + + static virDrvOpenStatus ATTRIBUTE_NONNULL(1) remoteSecretOpen(virConnectPtr conn, virConnectAuthPtr auth, unsigned int flags) diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x index 2e9dc1d..d42e5af 100644 --- a/src/remote/remote_protocol.x +++ b/src/remote/remote_protocol.x @@ -2808,6 +2808,11 @@ struct remote_domain_migrate_confirm3_params_args { int cancelled; }; +struct remote_domain_event_device_removed_msg { + remote_nonnull_domain dom; + remote_nonnull_string devAlias; +}; + /*----- Protocol. -----*/ /* Define the program number, protocol version and procedure numbers here. */ @@ -4944,6 +4949,12 @@ enum remote_procedure { * @generate: none * @acl: domain:migrate */ - REMOTE_PROC_DOMAIN_MIGRATE_CONFIRM3_PARAMS = 307 + REMOTE_PROC_DOMAIN_MIGRATE_CONFIRM3_PARAMS = 307, + + /** + * @generate: both + * @acl: none + */ + REMOTE_PROC_DOMAIN_EVENT_DEVICE_REMOVED = 308 }; diff --git a/src/remote_protocol-structs b/src/remote_protocol-structs index e38d24a..8d064fe 100644 --- a/src/remote_protocol-structs +++ b/src/remote_protocol-structs @@ -2293,6 +2293,10 @@ struct remote_domain_migrate_confirm3_params_args { u_int flags; int cancelled; }; +struct remote_domain_event_device_removed_msg { + remote_nonnull_domain dom; + remote_nonnull_string devAlias; +}; enum remote_procedure { REMOTE_PROC_CONNECT_OPEN = 1, REMOTE_PROC_CONNECT_CLOSE = 2, @@ -2601,4 +2605,5 @@ enum remote_procedure { REMOTE_PROC_DOMAIN_MIGRATE_PERFORM3_PARAMS = 305, REMOTE_PROC_DOMAIN_MIGRATE_FINISH3_PARAMS = 306, REMOTE_PROC_DOMAIN_MIGRATE_CONFIRM3_PARAMS = 307, + REMOTE_PROC_DOMAIN_EVENT_DEVICE_REMOVED = 308, }; -- 1.8.3.2
On Mon, Jul 15, 2013 at 07:11:02PM +0200, Jiri Denemark wrote:
--- daemon/remote.c | 32 +++++++++++++ include/libvirt/libvirt.h.in | 18 ++++++++ python/libvirt-override-virConnect.py | 9 ++++ python/libvirt-override.c | 52 ++++++++++++++++++++- src/conf/domain_event.c | 85 +++++++++++++++++++++++++++-------- src/conf/domain_event.h | 5 +++ src/libvirt_private.syms | 2 + src/remote/remote_driver.c | 32 +++++++++++++ src/remote/remote_protocol.x | 13 +++++- src/remote_protocol-structs | 5 +++ 10 files changed, 233 insertions(+), 20 deletions(-)
ACK Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
--- examples/domain-events/events-c/event-test.c | 23 +++++++++++++++++++++- examples/domain-events/events-python/event-test.py | 4 ++++ 2 files changed, 26 insertions(+), 1 deletion(-) diff --git a/examples/domain-events/events-c/event-test.c b/examples/domain-events/events-c/event-test.c index e9b3881..fe4eb56 100644 --- a/examples/domain-events/events-c/event-test.c +++ b/examples/domain-events/events-c/event-test.c @@ -428,6 +428,17 @@ static int myDomainEventPMSuspendDiskCallback(virConnectPtr conn ATTRIBUTE_UNUSE return 0; } +static int +myDomainEventDeviceRemovedCallback(virConnectPtr conn ATTRIBUTE_UNUSED, + virDomainPtr dom, + const char *devAlias, + void *opaque ATTRIBUTE_UNUSED) +{ + printf("%s EVENT: Domain %s(%d) device removed: %s\n", + __func__, virDomainGetName(dom), virDomainGetID(dom), devAlias); + return 0; +} + static void myFreeFunc(void *opaque) { char *str = opaque; @@ -467,6 +478,7 @@ int main(int argc, char **argv) int callback12ret = -1; int callback13ret = -1; int callback14ret = -1; + int callback15ret = -1; struct sigaction action_stop; memset(&action_stop, 0, sizeof(action_stop)); @@ -575,6 +587,12 @@ int main(int argc, char **argv) VIR_DOMAIN_EVENT_ID_PMSUSPEND_DISK, VIR_DOMAIN_EVENT_CALLBACK(myDomainEventPMSuspendDiskCallback), strdup("pmsuspend-disk"), myFreeFunc); + callback15ret = virConnectDomainEventRegisterAny(dconn, + NULL, + VIR_DOMAIN_EVENT_ID_DEVICE_REMOVED, + VIR_DOMAIN_EVENT_CALLBACK(myDomainEventDeviceRemovedCallback), + strdup("device removed"), myFreeFunc); + if ((callback1ret != -1) && (callback2ret != -1) && (callback3ret != -1) && @@ -587,7 +605,8 @@ int main(int argc, char **argv) (callback11ret != -1) && (callback12ret != -1) && (callback13ret != -1) && - (callback14ret != -1)) { + (callback14ret != -1) && + (callback15ret != -1)) { if (virConnectSetKeepAlive(dconn, 5, 3) < 0) { virErrorPtr err = virGetLastError(); fprintf(stderr, "Failed to start keepalive protocol: %s\n", @@ -616,6 +635,8 @@ int main(int argc, char **argv) virConnectDomainEventDeregisterAny(dconn, callback11ret); virConnectDomainEventDeregisterAny(dconn, callback12ret); virConnectDomainEventDeregisterAny(dconn, callback13ret); + virConnectDomainEventDeregisterAny(dconn, callback14ret); + virConnectDomainEventDeregisterAny(dconn, callback15ret); if (callback8ret != -1) virConnectDomainEventDeregisterAny(dconn, callback8ret); } diff --git a/examples/domain-events/events-python/event-test.py b/examples/domain-events/events-python/event-test.py index b456dec..b7c10d1 100644 --- a/examples/domain-events/events-python/event-test.py +++ b/examples/domain-events/events-python/event-test.py @@ -495,6 +495,9 @@ def myDomainEventBalloonChangeCallback(conn, dom, actual, opaque): def myDomainEventPMSuspendDiskCallback(conn, dom, reason, opaque): print "myDomainEventPMSuspendDiskCallback: Domain %s(%s) system pmsuspend_disk" % ( dom.name(), dom.ID()) +def myDomainEventDeviceRemovedCallback(conn, dom, dev, opaque): + print "myDomainEventDeviceRemovedCallback: Domain %s(%s) device removed: %s" % ( + dom.name(), dom.ID(), dev) run = True @@ -570,6 +573,7 @@ def main(): vc.domainEventRegisterAny(None, libvirt.VIR_DOMAIN_EVENT_ID_PMSUSPEND, myDomainEventPMSuspendCallback, None) vc.domainEventRegisterAny(None, libvirt.VIR_DOMAIN_EVENT_ID_BALLOON_CHANGE, myDomainEventBalloonChangeCallback, None) vc.domainEventRegisterAny(None, libvirt.VIR_DOMAIN_EVENT_ID_PMSUSPEND_DISK, myDomainEventPMSuspendDiskCallback, None) + vc.domainEventRegisterAny(None, libvirt.VIR_DOMAIN_EVENT_ID_DEVICE_REMOVED, myDomainEventDeviceRemovedCallback, None) vc.setKeepAlive(5, 3) -- 1.8.3.2
On Mon, Jul 15, 2013 at 07:11:03PM +0200, Jiri Denemark wrote:
--- examples/domain-events/events-c/event-test.c | 23 +++++++++++++++++++++- examples/domain-events/events-python/event-test.py | 4 ++++ 2 files changed, 26 insertions(+), 1 deletion(-)
ACK Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
--- src/libvirt.c | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/src/libvirt.c b/src/libvirt.c index 4dc91d7..860188b 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -10825,6 +10825,18 @@ error: * block copy operation on the device being detached; in that case, * use virDomainBlockJobAbort() to stop the block copy first. * + * Beware that depending on the hypervisor and device type, detaching a device + * from a running domain may be asynchronous. That is, calling + * virDomainDetachDeviceFlags may just request device removal while the device + * is actually removed later (in cooperation with a guest OS). Previously, + * this fact was ignored and the device could have been removed from domain + * configuration before it was actually removed by the hypervisor causing + * various failures on subsequent operations. To check whether the device was + * successfully removed, either recheck domain configuration using + * virDomainGetXMLDesc() or add handler for VIR_DOMAIN_EVENT_ID_DEVICE_REMOVED + * event. In case the device is already gone when virDomainDetachDeviceFlags + * returns, the event is delivered before this API call ends. + * * Returns 0 in case of success, -1 in case of failure. */ int -- 1.8.3.2
On Mon, Jul 15, 2013 at 07:11:04PM +0200, Jiri Denemark wrote:
--- src/libvirt.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)
diff --git a/src/libvirt.c b/src/libvirt.c index 4dc91d7..860188b 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -10825,6 +10825,18 @@ error: * block copy operation on the device being detached; in that case, * use virDomainBlockJobAbort() to stop the block copy first. * + * Beware that depending on the hypervisor and device type, detaching a device + * from a running domain may be asynchronous. That is, calling + * virDomainDetachDeviceFlags may just request device removal while the device + * is actually removed later (in cooperation with a guest OS). Previously, + * this fact was ignored and the device could have been removed from domain + * configuration before it was actually removed by the hypervisor causing + * various failures on subsequent operations. To check whether the device was + * successfully removed, either recheck domain configuration using + * virDomainGetXMLDesc() or add handler for VIR_DOMAIN_EVENT_ID_DEVICE_REMOVED + * event. In case the device is already gone when virDomainDetachDeviceFlags + * returns, the event is delivered before this API call ends. + * * Returns 0 in case of success, -1 in case of failure. */ int
This will need rewording based on my proposal that we should wait for completion. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
--- src/qemu/qemu_command.c | 35 ++++++++++++++++++++--- src/qemu/qemu_command.h | 8 +++--- src/qemu/qemu_hotplug.c | 75 ++++++++----------------------------------------- 3 files changed, 47 insertions(+), 71 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index d6ef9cd..b46462b 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -1144,8 +1144,9 @@ qemuDomainCCWAddressValidate(virDomainDefPtr def ATTRIBUTE_UNUSED, return qemuDomainCCWAddressAssign(info, data, false); } -int qemuDomainCCWAddressReleaseAddr(qemuDomainCCWAddressSetPtr addrs, - virDomainDeviceInfoPtr dev) +static int +qemuDomainCCWAddressReleaseAddr(qemuDomainCCWAddressSetPtr addrs, + virDomainDeviceInfoPtr dev) { char *addr; int ret; @@ -1784,8 +1785,9 @@ int qemuDomainPCIAddressReleaseAddr(qemuDomainPCIAddressSetPtr addrs, return 0; } -int qemuDomainPCIAddressReleaseSlot(qemuDomainPCIAddressSetPtr addrs, - virDevicePCIAddressPtr addr) +static int +qemuDomainPCIAddressReleaseSlot(qemuDomainPCIAddressSetPtr addrs, + virDevicePCIAddressPtr addr) { if (!qemuPCIAddressValidate(addrs, addr)) return -1; @@ -1878,6 +1880,31 @@ int qemuDomainPCIAddressSetNextAddr(qemuDomainPCIAddressSetPtr addrs, } +void +qemuDomainReleaseDeviceAddress(virDomainObjPtr vm, + virDomainDeviceInfoPtr info, + const char *devstr) +{ + qemuDomainObjPrivatePtr priv = vm->privateData; + + if (!devstr) + devstr = info->alias; + + if (info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW && + STREQLEN(vm->def->os.machine, "s390-ccw", 8) && + virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_VIRTIO_CCW) && + qemuDomainCCWAddressReleaseAddr(priv->ccwaddrs, info) < 0) + VIR_WARN("Unable to release CCW address on %s", + NULLSTR(devstr)); + else if (info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI && + virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE) && + qemuDomainPCIAddressReleaseSlot(priv->pciaddrs, + &info->addr.pci) < 0) + VIR_WARN("Unable to release PCI address on %s", + NULLSTR(devstr)); +} + + #define IS_USB2_CONTROLLER(ctrl) \ (((ctrl)->type == VIR_DOMAIN_CONTROLLER_TYPE_USB) && \ ((ctrl)->model == VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_EHCI1 || \ diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h index 88d7099..e15fe64 100644 --- a/src/qemu/qemu_command.h +++ b/src/qemu/qemu_command.h @@ -221,6 +221,10 @@ int qemuDomainAssignAddresses(virDomainDefPtr def, int qemuDomainAssignSpaprVIOAddresses(virDomainDefPtr def, virQEMUCapsPtr qemuCaps); +void qemuDomainReleaseDeviceAddress(virDomainObjPtr vm, + virDomainDeviceInfoPtr info, + const char *devstr); + int qemuDomainAssignPCIAddresses(virDomainDefPtr def, virQEMUCapsPtr qemuCaps, virDomainObjPtr obj); @@ -237,16 +241,12 @@ int qemuDomainPCIAddressEnsureAddr(qemuDomainPCIAddressSetPtr addrs, virDomainDeviceInfoPtr dev); int qemuDomainPCIAddressReleaseAddr(qemuDomainPCIAddressSetPtr addrs, virDevicePCIAddressPtr addr); -int qemuDomainPCIAddressReleaseSlot(qemuDomainPCIAddressSetPtr addrs, - virDevicePCIAddressPtr addr); void qemuDomainPCIAddressSetFree(qemuDomainPCIAddressSetPtr addrs); int qemuAssignDevicePCISlots(virDomainDefPtr def, virQEMUCapsPtr qemuCaps, qemuDomainPCIAddressSetPtr addrs); -int qemuDomainCCWAddressReleaseAddr(qemuDomainCCWAddressSetPtr addrs, - virDomainDeviceInfoPtr dev); int qemuDomainCCWAddressAssign(virDomainDeviceInfoPtr dev, qemuDomainCCWAddressSetPtr addrs, bool autoassign); void qemuDomainCCWAddressSetFree(qemuDomainCCWAddressSetPtr addrs); diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index ac9350b..2a57fef 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -327,16 +327,8 @@ cleanup: return ret; error: - if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE) && releaseaddr) { - if (disk->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI && - qemuDomainPCIAddressReleaseSlot(priv->pciaddrs, - &disk->info.addr.pci) < 0) - VIR_WARN("Unable to release PCI address on %s", disk->src); - else if (disk->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW && - qemuDomainCCWAddressReleaseAddr(priv->ccwaddrs, - &disk->info) < 0) - VIR_WARN("Unable to release CCW address on %s", disk->src); - } + if (releaseaddr) + qemuDomainReleaseDeviceAddress(vm, &disk->info, disk->src); if (virSecurityManagerRestoreImageLabel(driver->securityManager, vm->def, disk) < 0) @@ -405,13 +397,8 @@ int qemuDomainAttachPciControllerDevice(virQEMUDriverPtr driver, } cleanup: - if ((ret != 0) && - virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE) && - (controller->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) && - releaseaddr && - qemuDomainPCIAddressReleaseSlot(priv->pciaddrs, - &controller->info.addr.pci) < 0) - VIR_WARN("Unable to release PCI address on controller"); + if (ret != 0 && releaseaddr) + qemuDomainReleaseDeviceAddress(vm, &controller->info, NULL); VIR_FREE(devstr); return ret; @@ -930,19 +917,8 @@ cleanup: if (!ret) { vm->def->nets[vm->def->nnets++] = net; } else { - if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE) && - net->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI && - releaseaddr && - qemuDomainPCIAddressReleaseSlot(priv->pciaddrs, - &net->info.addr.pci) < 0) - VIR_WARN("Unable to release PCI address on NIC"); - else if (STREQLEN(vm->def->os.machine, "s390-ccw", 8) && - virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_VIRTIO_CCW) && - net->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW && - releaseaddr && - qemuDomainCCWAddressReleaseAddr(priv->ccwaddrs, - &net->info) < 0) - VIR_WARN("Unable to release CCW address on NIC"); + if (releaseaddr) + qemuDomainReleaseDeviceAddress(vm, &net->info, NULL); if (iface_connected) { virDomainConfNWFilterTeardown(net); @@ -1113,12 +1089,8 @@ int qemuDomainAttachHostPciDevice(virQEMUDriverPtr driver, return 0; error: - if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE) && - (hostdev->info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) && - releaseaddr && - qemuDomainPCIAddressReleaseSlot(priv->pciaddrs, - &hostdev->info->addr.pci) < 0) - VIR_WARN("Unable to release PCI address on host device"); + if (releaseaddr) + qemuDomainReleaseDeviceAddress(vm, hostdev->info, NULL); qemuDomainReAttachHostdevDevices(driver, vm->def->name, &hostdev, 1); @@ -2213,16 +2185,7 @@ int qemuDomainDetachVirtioDiskDevice(virQEMUDriverPtr driver, virDomainAuditDisk(vm, detach->src, NULL, "detach", true); - if (detach->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW && - STREQLEN(vm->def->os.machine, "s390-ccw", 8) && - virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_VIRTIO_CCW) && - qemuDomainCCWAddressReleaseAddr(priv->ccwaddrs, &detach->info) < 0) { - VIR_WARN("Unable to release CCW address on %s", dev->data.disk->src); - } else if (detach->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI && - virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE) && - qemuDomainPCIAddressReleaseSlot(priv->pciaddrs, - &detach->info.addr.pci) < 0) - VIR_WARN("Unable to release PCI address on %s", dev->data.disk->src); + qemuDomainReleaseDeviceAddress(vm, &detach->info, dev->data.disk->src); virDomainDiskRemove(vm->def, idx); @@ -2437,13 +2400,9 @@ int qemuDomainDetachPciControllerDevice(virQEMUDriverPtr driver, qemuDomainObjExitMonitor(driver, vm); virDomainControllerRemove(vm->def, idx); + qemuDomainReleaseDeviceAddress(vm, &detach->info, NULL); virDomainControllerDefFree(detach); - if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE) && - qemuDomainPCIAddressReleaseSlot(priv->pciaddrs, - &detach->info.addr.pci) < 0) - VIR_WARN("Unable to release PCI address on controller"); - ret = 0; cleanup: @@ -2515,10 +2474,7 @@ qemuDomainDetachHostPciDevice(virQEMUDriverPtr driver, virObjectUnlock(driver->activePciHostdevs); virObjectUnlock(driver->inactivePciHostdevs); - if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE) && - qemuDomainPCIAddressReleaseSlot(priv->pciaddrs, - &detach->info->addr.pci) < 0) - VIR_WARN("Unable to release PCI address on host device"); + qemuDomainReleaseDeviceAddress(vm, detach->info, NULL); cleanup: virObjectUnref(cfg); @@ -2841,14 +2797,7 @@ qemuDomainDetachNetDevice(virQEMUDriverPtr driver, virDomainAuditNet(vm, detach, NULL, "detach", true); - if (STREQLEN(vm->def->os.machine, "s390-ccw", 8) && - virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_VIRTIO_CCW)) { - if (qemuDomainCCWAddressReleaseAddr(priv->ccwaddrs, &detach->info) < 0) - VIR_WARN("Unable to release CCW address on NIC"); - } else if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE) && - qemuDomainPCIAddressReleaseSlot(priv->pciaddrs, - &detach->info.addr.pci) < 0) - VIR_WARN("Unable to release PCI address on NIC"); + qemuDomainReleaseDeviceAddress(vm, &detach->info, NULL); virDomainConfNWFilterTeardown(detach); -- 1.8.3.2
On Mon, Jul 15, 2013 at 07:11:05PM +0200, Jiri Denemark wrote:
--- src/qemu/qemu_command.c | 35 ++++++++++++++++++++--- src/qemu/qemu_command.h | 8 +++--- src/qemu/qemu_hotplug.c | 75 ++++++++----------------------------------------- 3 files changed, 47 insertions(+), 71 deletions(-)
ACK Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On Tue, Jul 16, 2013 at 10:54:50 +0100, Daniel Berrange wrote:
On Mon, Jul 15, 2013 at 07:11:05PM +0200, Jiri Denemark wrote:
--- src/qemu/qemu_command.c | 35 ++++++++++++++++++++--- src/qemu/qemu_command.h | 8 +++--- src/qemu/qemu_hotplug.c | 75 ++++++++----------------------------------------- 3 files changed, 47 insertions(+), 71 deletions(-)
ACK
Pushed, thanks. Jirka
--- src/qemu/qemu_driver.c | 31 +++++++++--- src/qemu/qemu_hotplug.c | 127 ++++++++++++++++++------------------------------ src/qemu/qemu_hotplug.h | 4 +- 3 files changed, 73 insertions(+), 89 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 495867a..436ce6a 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6401,21 +6401,43 @@ qemuDomainAttachDeviceLive(virDomainObjPtr vm, } static int +qemuFindDisk(virDomainDefPtr def, const char *dst) +{ + size_t i; + + for (i = 0; i < def->ndisks; i++) { + if (STREQ(def->disks[i]->dst, dst)) { + return i; + } + } + + return -1; +} + +static int qemuDomainDetachDeviceDiskLive(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainDeviceDefPtr dev) { - virDomainDiskDefPtr disk = dev->data.disk; + virDomainDiskDefPtr disk; int ret = -1; + int idx; + + if ((idx = qemuFindDisk(vm->def, dev->data.disk->dst)) < 0) { + virReportError(VIR_ERR_OPERATION_FAILED, + _("disk %s not found"), dev->data.disk->dst); + return -1; + } + disk = vm->def->disks[idx]; switch (disk->device) { case VIR_DOMAIN_DISK_DEVICE_DISK: case VIR_DOMAIN_DISK_DEVICE_LUN: if (disk->bus == VIR_DOMAIN_DISK_BUS_VIRTIO) - ret = qemuDomainDetachVirtioDiskDevice(driver, vm, dev); + ret = qemuDomainDetachVirtioDiskDevice(driver, vm, disk); else if (disk->bus == VIR_DOMAIN_DISK_BUS_SCSI || disk->bus == VIR_DOMAIN_DISK_BUS_USB) - ret = qemuDomainDetachDiskDevice(driver, vm, dev); + ret = qemuDomainDetachDiskDevice(driver, vm, disk); else virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", _("This type of disk cannot be hot unplugged")); @@ -6427,9 +6449,6 @@ qemuDomainDetachDeviceDiskLive(virQEMUDriverPtr driver, break; } - if (ret == 0) - ignore_value(qemuRemoveSharedDevice(driver, dev, vm->def->name)); - return ret; } diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 2a57fef..6db789d 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -2071,19 +2071,6 @@ cleanup: } -static inline int qemuFindDisk(virDomainDefPtr def, const char *dst) -{ - size_t i; - - for (i = 0; i < def->ndisks; i++) { - if (STREQ(def->disks[i]->dst, dst)) { - return i; - } - } - - return -1; -} - static int qemuComparePCIDevice(virDomainDefPtr def ATTRIBUTE_UNUSED, virDomainDeviceDefPtr device ATTRIBUTE_UNUSED, virDomainDeviceInfoPtr info1, @@ -2112,30 +2099,58 @@ static bool qemuIsMultiFunctionDevice(virDomainDefPtr def, } +static void +qemuDomainRemoveDiskDevice(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainDiskDefPtr disk) +{ + virDomainDeviceDef dev; + size_t i; + + VIR_DEBUG("Removing disk %s from domain %p %s", + disk->info.alias, vm, vm->def->name); + + virDomainAuditDisk(vm, disk->src, NULL, "detach", true); + + for (i = 0; i < vm->def->ndisks; i++) { + if (vm->def->disks[i] == disk) { + virDomainDiskRemove(vm->def, i); + break; + } + } + + qemuDomainReleaseDeviceAddress(vm, &disk->info, disk->src); + + if (virSecurityManagerRestoreImageLabel(driver->securityManager, + vm->def, disk) < 0) + VIR_WARN("Unable to restore security label on %s", disk->src); + + if (qemuTeardownDiskCgroup(vm, disk) < 0) + VIR_WARN("Failed to tear down cgroup for disk path %s", disk->src); + + if (virDomainLockDiskDetach(driver->lockManager, vm, disk) < 0) + VIR_WARN("Unable to release lock on %s", disk->src); + + dev.type = VIR_DOMAIN_DEVICE_DISK; + dev.data.disk = disk; + ignore_value(qemuRemoveSharedDevice(driver, &dev, vm->def->name)); + + virDomainDiskDefFree(disk); +} + + int qemuDomainDetachVirtioDiskDevice(virQEMUDriverPtr driver, virDomainObjPtr vm, - virDomainDeviceDefPtr dev) + virDomainDiskDefPtr detach) { - int idx; int ret = -1; - virDomainDiskDefPtr detach = NULL; qemuDomainObjPrivatePtr priv = vm->privateData; char *drivestr = NULL; - idx = qemuFindDisk(vm->def, dev->data.disk->dst); - - if (idx < 0) { - virReportError(VIR_ERR_OPERATION_FAILED, - _("disk %s not found"), dev->data.disk->dst); - goto cleanup; - } - - detach = vm->def->disks[idx]; - if (qemuIsMultiFunctionDevice(vm->def, &detach->info)) { virReportError(VIR_ERR_OPERATION_FAILED, _("cannot hot unplug multifunction PCI device: %s"), - dev->data.disk->dst); + detach->dst); goto cleanup; } @@ -2183,27 +2198,7 @@ int qemuDomainDetachVirtioDiskDevice(virQEMUDriverPtr driver, qemuDomainObjExitMonitor(driver, vm); - virDomainAuditDisk(vm, detach->src, NULL, "detach", true); - - qemuDomainReleaseDeviceAddress(vm, &detach->info, dev->data.disk->src); - - virDomainDiskRemove(vm->def, idx); - - dev->data.disk->backingChain = detach->backingChain; - detach->backingChain = NULL; - virDomainDiskDefFree(detach); - - if (virSecurityManagerRestoreImageLabel(driver->securityManager, - vm->def, dev->data.disk) < 0) - VIR_WARN("Unable to restore security label on %s", dev->data.disk->src); - - if (qemuTeardownDiskCgroup(vm, dev->data.disk) < 0) - VIR_WARN("Failed to teardown cgroup for disk path %s", - NULLSTR(dev->data.disk->src)); - - if (virDomainLockDiskDetach(driver->lockManager, vm, dev->data.disk) < 0) - VIR_WARN("Unable to release lock on %s", dev->data.disk->src); - + qemuDomainRemoveDiskDevice(driver, vm, detach); ret = 0; cleanup: @@ -2213,31 +2208,19 @@ cleanup: int qemuDomainDetachDiskDevice(virQEMUDriverPtr driver, virDomainObjPtr vm, - virDomainDeviceDefPtr dev) + virDomainDiskDefPtr detach) { - int idx; int ret = -1; - virDomainDiskDefPtr detach = NULL; qemuDomainObjPrivatePtr priv = vm->privateData; char *drivestr = NULL; - idx = qemuFindDisk(vm->def, dev->data.disk->dst); - - if (idx < 0) { - virReportError(VIR_ERR_OPERATION_FAILED, - _("disk %s not found"), dev->data.disk->dst); - goto cleanup; - } - if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) { virReportError(VIR_ERR_OPERATION_FAILED, _("Underlying qemu does not support %s disk removal"), - virDomainDiskBusTypeToString(dev->data.disk->bus)); + virDomainDiskBusTypeToString(detach->bus)); goto cleanup; } - detach = vm->def->disks[idx]; - if (detach->mirror) { virReportError(VIR_ERR_BLOCK_COPY_ACTIVE, _("disk '%s' is in an active block copy job"), @@ -2263,25 +2246,7 @@ int qemuDomainDetachDiskDevice(virQEMUDriverPtr driver, qemuDomainObjExitMonitor(driver, vm); - virDomainAuditDisk(vm, detach->src, NULL, "detach", true); - - virDomainDiskRemove(vm->def, idx); - - dev->data.disk->backingChain = detach->backingChain; - detach->backingChain = NULL; - virDomainDiskDefFree(detach); - - if (virSecurityManagerRestoreImageLabel(driver->securityManager, - vm->def, dev->data.disk) < 0) - VIR_WARN("Unable to restore security label on %s", dev->data.disk->src); - - if (qemuTeardownDiskCgroup(vm, dev->data.disk) < 0) - VIR_WARN("Failed to teardown cgroup for disk path %s", - NULLSTR(dev->data.disk->src)); - - if (virDomainLockDiskDetach(driver->lockManager, vm, dev->data.disk) < 0) - VIR_WARN("Unable to release lock on disk %s", dev->data.disk->src); - + qemuDomainRemoveDiskDevice(driver, vm, detach); ret = 0; cleanup: diff --git a/src/qemu/qemu_hotplug.h b/src/qemu/qemu_hotplug.h index da20eb1..da802ee 100644 --- a/src/qemu/qemu_hotplug.h +++ b/src/qemu/qemu_hotplug.h @@ -85,10 +85,10 @@ int qemuDomainChangeNetLinkState(virQEMUDriverPtr driver, int linkstate); int qemuDomainDetachVirtioDiskDevice(virQEMUDriverPtr driver, virDomainObjPtr vm, - virDomainDeviceDefPtr dev); + virDomainDiskDefPtr disk); int qemuDomainDetachDiskDevice(virQEMUDriverPtr driver, virDomainObjPtr vm, - virDomainDeviceDefPtr dev); + virDomainDiskDefPtr disk); int qemuDomainDetachPciControllerDevice(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainDeviceDefPtr dev); -- 1.8.3.2
On Mon, Jul 15, 2013 at 07:11:06PM +0200, Jiri Denemark wrote:
--- src/qemu/qemu_driver.c | 31 +++++++++--- src/qemu/qemu_hotplug.c | 127 ++++++++++++++++++------------------------------ src/qemu/qemu_hotplug.h | 4 +- 3 files changed, 73 insertions(+), 89 deletions(-)
ACK Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On Tue, Jul 16, 2013 at 10:56:13 +0100, Daniel Berrange wrote:
On Mon, Jul 15, 2013 at 07:11:06PM +0200, Jiri Denemark wrote:
--- src/qemu/qemu_driver.c | 31 +++++++++--- src/qemu/qemu_hotplug.c | 127 ++++++++++++++++++------------------------------ src/qemu/qemu_hotplug.h | 4 +- 3 files changed, 73 insertions(+), 89 deletions(-)
ACK
Pushed, thanks. Jirka
--- src/qemu/qemu_hotplug.c | 26 +++++++++++++++++++++++--- 1 file changed, 23 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 6db789d..0093245 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -2139,6 +2139,28 @@ qemuDomainRemoveDiskDevice(virQEMUDriverPtr driver, } +static void +qemuDomainRemoveControllerDevice(virQEMUDriverPtr driver ATTRIBUTE_UNUSED, + virDomainObjPtr vm, + virDomainControllerDefPtr controller) +{ + size_t i; + + VIR_DEBUG("Removing controller %s from domain %p %s", + controller->info.alias, vm, vm->def->name); + + for (i = 0; i < vm->def->ncontrollers; i++) { + if (vm->def->controllers[i] == controller) { + virDomainControllerRemove(vm->def, i); + break; + } + } + + qemuDomainReleaseDeviceAddress(vm, &controller->info, NULL); + virDomainControllerDefFree(controller); +} + + int qemuDomainDetachVirtioDiskDevice(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainDiskDefPtr detach) @@ -2364,9 +2386,7 @@ int qemuDomainDetachPciControllerDevice(virQEMUDriverPtr driver, } qemuDomainObjExitMonitor(driver, vm); - virDomainControllerRemove(vm->def, idx); - qemuDomainReleaseDeviceAddress(vm, &detach->info, NULL); - virDomainControllerDefFree(detach); + qemuDomainRemoveControllerDevice(driver, vm, detach); ret = 0; -- 1.8.3.2
On Mon, Jul 15, 2013 at 07:11:07PM +0200, Jiri Denemark wrote:
--- src/qemu/qemu_hotplug.c | 26 +++++++++++++++++++++++--- 1 file changed, 23 insertions(+), 3 deletions(-)
ACK Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On Tue, Jul 16, 2013 at 10:56:55 +0100, Daniel Berrange wrote:
On Mon, Jul 15, 2013 at 07:11:07PM +0200, Jiri Denemark wrote:
--- src/qemu/qemu_hotplug.c | 26 +++++++++++++++++++++++--- 1 file changed, 23 insertions(+), 3 deletions(-)
ACK
Pushed, thanks. Jirka
--- src/qemu/qemu_hotplug.c | 101 +++++++++++++++++++++++++++++------------------- 1 file changed, 62 insertions(+), 39 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 0093245..26a6d42 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -2161,6 +2161,62 @@ qemuDomainRemoveControllerDevice(virQEMUDriverPtr driver ATTRIBUTE_UNUSED, } +static void +qemuDomainRemoveNetDevice(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainNetDefPtr net) +{ + virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); + virNetDevVPortProfilePtr vport; + size_t i; + + VIR_DEBUG("Removing network interface %s from domain %p %s", + net->info.alias, vm, vm->def->name); + + virDomainAuditNet(vm, net, NULL, "detach", true); + + for (i = 0; i < vm->def->nnets; i++) { + if (vm->def->nets[i] == net) { + virDomainNetRemove(vm->def, i); + break; + } + } + + qemuDomainReleaseDeviceAddress(vm, &net->info, NULL); + virDomainConfNWFilterTeardown(net); + + if (virDomainNetGetActualType(net) == VIR_DOMAIN_NET_TYPE_DIRECT) { + ignore_value(virNetDevMacVLanDeleteWithVPortProfile( + net->ifname, &net->mac, + virDomainNetGetActualDirectDev(net), + virDomainNetGetActualDirectMode(net), + virDomainNetGetActualVirtPortProfile(net), + cfg->stateDir)); + VIR_FREE(net->ifname); + } + + if (cfg->macFilter && (net->ifname != NULL)) { + if ((errno = networkDisallowMacOnPort(driver, + net->ifname, + &net->mac))) { + virReportSystemError(errno, + _("failed to remove ebtables rule on '%s'"), + net->ifname); + } + } + + vport = virDomainNetGetActualVirtPortProfile(net); + if (vport && vport->virtPortType == VIR_NETDEV_VPORT_PROFILE_OPENVSWITCH) + ignore_value(virNetDevOpenvswitchRemovePort( + virDomainNetGetActualBridgeName(net), + net->ifname)); + + networkReleaseActualDevice(net); + virDomainNetDefFree(net); + virObjectUnref(cfg); +} + + int qemuDomainDetachVirtioDiskDevice(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainDiskDefPtr detach) @@ -2690,8 +2746,6 @@ qemuDomainDetachNetDevice(virQEMUDriverPtr driver, int vlan; char *hostnet_name = NULL; char mac[VIR_MAC_STRING_BUFLEN]; - virNetDevVPortProfilePtr vport = NULL; - virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); detachidx = virDomainNetFindIdx(vm->def, dev->data.net); if (detachidx == -2) { @@ -2713,6 +2767,11 @@ qemuDomainDetachNetDevice(virQEMUDriverPtr driver, ret = qemuDomainDetachThisHostDevice(driver, vm, virDomainNetGetActualHostdev(detach), -1); + if (!ret) { + networkReleaseActualDevice(detach); + virDomainNetRemove(vm->def, detachidx); + virDomainNetDefFree(detach); + } goto cleanup; } if (STREQLEN(vm->def->os.machine, "s390-ccw", 8) && @@ -2780,46 +2839,10 @@ qemuDomainDetachNetDevice(virQEMUDriverPtr driver, } qemuDomainObjExitMonitor(driver, vm); - virDomainAuditNet(vm, detach, NULL, "detach", true); - - qemuDomainReleaseDeviceAddress(vm, &detach->info, NULL); - - virDomainConfNWFilterTeardown(detach); - - if (virDomainNetGetActualType(detach) == VIR_DOMAIN_NET_TYPE_DIRECT) { - ignore_value(virNetDevMacVLanDeleteWithVPortProfile( - detach->ifname, &detach->mac, - virDomainNetGetActualDirectDev(detach), - virDomainNetGetActualDirectMode(detach), - virDomainNetGetActualVirtPortProfile(detach), - cfg->stateDir)); - VIR_FREE(detach->ifname); - } - - if (cfg->macFilter && (detach->ifname != NULL)) { - if ((errno = networkDisallowMacOnPort(driver, - detach->ifname, - &detach->mac))) { - virReportSystemError(errno, - _("failed to remove ebtables rule on '%s'"), - detach->ifname); - } - } - - vport = virDomainNetGetActualVirtPortProfile(detach); - if (vport && vport->virtPortType == VIR_NETDEV_VPORT_PROFILE_OPENVSWITCH) - ignore_value(virNetDevOpenvswitchRemovePort( - virDomainNetGetActualBridgeName(detach), - detach->ifname)); + qemuDomainRemoveNetDevice(driver, vm, detach); ret = 0; cleanup: - if (!ret) { - networkReleaseActualDevice(detach); - virDomainNetRemove(vm->def, detachidx); - virDomainNetDefFree(detach); - } VIR_FREE(hostnet_name); - virObjectUnref(cfg); return ret; } -- 1.8.3.2
On Mon, Jul 15, 2013 at 07:11:08PM +0200, Jiri Denemark wrote:
--- src/qemu/qemu_hotplug.c | 101 +++++++++++++++++++++++++++++------------------- 1 file changed, 62 insertions(+), 39 deletions(-)
ACK Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On Tue, Jul 16, 2013 at 10:57:45 +0100, Daniel Berrange wrote:
On Mon, Jul 15, 2013 at 07:11:08PM +0200, Jiri Denemark wrote:
--- src/qemu/qemu_hotplug.c | 101 +++++++++++++++++++++++++++++------------------- 1 file changed, 62 insertions(+), 39 deletions(-)
ACK
Pushed, thanks. Jirka
--- src/qemu/qemu_hotplug.c | 240 ++++++++++++++++++++++++++++-------------------- 1 file changed, 142 insertions(+), 98 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 26a6d42..169cce2 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -2217,6 +2217,136 @@ qemuDomainRemoveNetDevice(virQEMUDriverPtr driver, } +static void +qemuDomainRemovePCIHostDevice(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainHostdevDefPtr hostdev) +{ + virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); + virDomainHostdevSubsysPtr subsys = &hostdev->source.subsys; + virPCIDevicePtr pci; + virPCIDevicePtr activePci; + + /* + * For SRIOV net host devices, unset mac and port profile before + * reset and reattach device + */ + if (hostdev->parent.data.net) + qemuDomainHostdevNetConfigRestore(hostdev, cfg->stateDir); + + virObjectLock(driver->activePciHostdevs); + virObjectLock(driver->inactivePciHostdevs); + pci = virPCIDeviceNew(subsys->u.pci.addr.domain, subsys->u.pci.addr.bus, + subsys->u.pci.addr.slot, subsys->u.pci.addr.function); + if (pci) { + activePci = virPCIDeviceListSteal(driver->activePciHostdevs, pci); + if (activePci && + virPCIDeviceReset(activePci, driver->activePciHostdevs, + driver->inactivePciHostdevs) == 0) { + qemuReattachPciDevice(activePci, driver); + } else { + /* reset of the device failed, treat it as if it was returned */ + virPCIDeviceFree(activePci); + } + virPCIDeviceFree(pci); + } + virObjectUnlock(driver->activePciHostdevs); + virObjectUnlock(driver->inactivePciHostdevs); + + qemuDomainReleaseDeviceAddress(vm, hostdev->info, NULL); + virObjectUnref(cfg); +} + +static void +qemuDomainRemoveUSBHostDevice(virQEMUDriverPtr driver, + virDomainObjPtr vm ATTRIBUTE_UNUSED, + virDomainHostdevDefPtr hostdev) +{ + virDomainHostdevSubsysPtr subsys = &hostdev->source.subsys; + virUSBDevicePtr usb; + + usb = virUSBDeviceNew(subsys->u.usb.bus, subsys->u.usb.device, NULL); + if (usb) { + virObjectLock(driver->activeUsbHostdevs); + virUSBDeviceListDel(driver->activeUsbHostdevs, usb); + virObjectUnlock(driver->activeUsbHostdevs); + virUSBDeviceFree(usb); + } else { + VIR_WARN("Unable to find device %03d.%03d in list of used USB devices", + subsys->u.usb.bus, subsys->u.usb.device); + } +} + +static void +qemuDomainRemoveSCSIHostDevice(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainHostdevDefPtr hostdev) +{ + qemuDomainReAttachHostScsiDevices(driver, vm->def->name, &hostdev, 1); +} + +static void +qemuDomainRemoveHostDevice(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainHostdevDefPtr hostdev) +{ + virDomainNetDefPtr net = NULL; + size_t i; + + VIR_DEBUG("Removing host device %s from domain %p %s", + hostdev->info->alias, vm, vm->def->name); + + if (hostdev->parent.type == VIR_DOMAIN_DEVICE_NET) { + net = hostdev->parent.data.net; + + for (i = 0; i < vm->def->nnets; i++) { + if (vm->def->nets[i] == net) { + virDomainNetRemove(vm->def, i); + break; + } + } + } + + for (i = 0; i < vm->def->nhostdevs; i++) { + if (vm->def->hostdevs[i] == hostdev) { + virDomainHostdevRemove(vm->def, i); + break; + } + } + + virDomainAuditHostdev(vm, hostdev, "detach", true); + + switch ((enum virDomainHostdevSubsysType) hostdev->source.subsys.type) { + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI: + qemuDomainRemovePCIHostDevice(driver, vm, hostdev); + break; + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB: + qemuDomainRemoveUSBHostDevice(driver, vm, hostdev); + break; + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI: + qemuDomainRemoveSCSIHostDevice(driver, vm, hostdev); + break; + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_LAST: + break; + } + + if (qemuTeardownHostdevCgroup(vm, hostdev) < 0) + VIR_WARN("Failed to remove host device cgroup ACL"); + + if (virSecurityManagerRestoreHostdevLabel(driver->securityManager, + vm->def, hostdev, NULL) < 0) { + VIR_WARN("Failed to restore host device labelling"); + } + + virDomainHostdevDefFree(hostdev); + + if (net) { + networkReleaseActualDevice(net); + virDomainNetDefFree(net); + } +} + + int qemuDomainDetachVirtioDiskDevice(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainDiskDefPtr detach) @@ -2457,68 +2587,31 @@ qemuDomainDetachHostPciDevice(virQEMUDriverPtr driver, { qemuDomainObjPrivatePtr priv = vm->privateData; virDomainHostdevSubsysPtr subsys = &detach->source.subsys; - int ret = -1, rv; - virPCIDevicePtr pci; - virPCIDevicePtr activePci; - virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); + int ret; if (qemuIsMultiFunctionDevice(vm->def, detach->info)) { virReportError(VIR_ERR_OPERATION_FAILED, _("cannot hot unplug multifunction PCI device: %.4x:%.2x:%.2x.%.1x"), subsys->u.pci.addr.domain, subsys->u.pci.addr.bus, subsys->u.pci.addr.slot, subsys->u.pci.addr.function); - goto cleanup; + return -1; } if (!virDomainDeviceAddressIsValid(detach->info, VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI)) { virReportError(VIR_ERR_OPERATION_FAILED, "%s", _("device cannot be detached without a PCI address")); - goto cleanup; + return -1; } qemuDomainObjEnterMonitor(driver, vm); if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) { - rv = qemuMonitorDelDevice(priv->mon, detach->info->alias); + ret = qemuMonitorDelDevice(priv->mon, detach->info->alias); } else { - rv = qemuMonitorRemovePCIDevice(priv->mon, &detach->info->addr.pci); + ret = qemuMonitorRemovePCIDevice(priv->mon, &detach->info->addr.pci); } qemuDomainObjExitMonitor(driver, vm); - virDomainAuditHostdev(vm, detach, "detach", rv == 0); - if (rv < 0) - goto cleanup; - - /* - * For SRIOV net host devices, unset mac and port profile before - * reset and reattach device - */ - if (detach->parent.data.net) - qemuDomainHostdevNetConfigRestore(detach, cfg->stateDir); - - virObjectLock(driver->activePciHostdevs); - virObjectLock(driver->inactivePciHostdevs); - pci = virPCIDeviceNew(subsys->u.pci.addr.domain, subsys->u.pci.addr.bus, - subsys->u.pci.addr.slot, subsys->u.pci.addr.function); - if (pci) { - activePci = virPCIDeviceListSteal(driver->activePciHostdevs, pci); - if (activePci && - virPCIDeviceReset(activePci, driver->activePciHostdevs, - driver->inactivePciHostdevs) == 0) { - qemuReattachPciDevice(activePci, driver); - ret = 0; - } else { - /* reset of the device failed, treat it as if it was returned */ - virPCIDeviceFree(activePci); - } - virPCIDeviceFree(pci); - } - virObjectUnlock(driver->activePciHostdevs); - virObjectUnlock(driver->inactivePciHostdevs); - - qemuDomainReleaseDeviceAddress(vm, detach->info, NULL); -cleanup: - virObjectUnref(cfg); return ret; } @@ -2528,8 +2621,6 @@ qemuDomainDetachHostUsbDevice(virQEMUDriverPtr driver, virDomainHostdevDefPtr detach) { qemuDomainObjPrivatePtr priv = vm->privateData; - virDomainHostdevSubsysPtr subsys = &detach->source.subsys; - virUSBDevicePtr usb; int ret; if (!detach->info->alias) { @@ -2547,20 +2638,7 @@ qemuDomainDetachHostUsbDevice(virQEMUDriverPtr driver, qemuDomainObjEnterMonitor(driver, vm); ret = qemuMonitorDelDevice(priv->mon, detach->info->alias); qemuDomainObjExitMonitor(driver, vm); - virDomainAuditHostdev(vm, detach, "detach", ret == 0); - if (ret < 0) - return -1; - usb = virUSBDeviceNew(subsys->u.usb.bus, subsys->u.usb.device, NULL); - if (usb) { - virObjectLock(driver->activeUsbHostdevs); - virUSBDeviceListDel(driver->activeUsbHostdevs, usb); - virObjectUnlock(driver->activeUsbHostdevs); - virUSBDeviceFree(usb); - } else { - VIR_WARN("Unable to find device %03d.%03d in list of used USB devices", - subsys->u.usb.bus, subsys->u.usb.device); - } return ret; } @@ -2608,11 +2686,6 @@ qemuDomainDetachHostScsiDevice(virQEMUDriverPtr driver, } qemuDomainObjExitMonitor(driver, vm); - virDomainAuditHostdev(vm, detach, "detach", ret == 0); - - if (ret == 0) - qemuDomainReAttachHostScsiDevices(driver, vm->def->name, &detach, 1); - cleanup: VIR_FREE(drvstr); VIR_FREE(devstr); @@ -2622,27 +2695,10 @@ cleanup: static int qemuDomainDetachThisHostDevice(virQEMUDriverPtr driver, virDomainObjPtr vm, - virDomainHostdevDefPtr detach, - int idx) + virDomainHostdevDefPtr detach) { int ret = -1; - if (idx < 0) { - /* caller didn't know index of hostdev in hostdevs list, so we - * need to find it. - */ - for (idx = 0; idx < vm->def->nhostdevs; idx++) { - if (vm->def->hostdevs[idx] == detach) - break; - } - if (idx >= vm->def->nhostdevs) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("device not found in hostdevs list (%zu entries)"), - vm->def->nhostdevs); - return ret; - } - } - switch (detach->source.subsys.type) { case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI: ret = qemuDomainDetachHostPciDevice(driver, vm, detach); @@ -2660,17 +2716,11 @@ qemuDomainDetachThisHostDevice(virQEMUDriverPtr driver, return -1; } - if (!ret) { - if (qemuTeardownHostdevCgroup(vm, detach) < 0) - VIR_WARN("Failed to remove host device cgroup ACL"); + if (ret < 0) + virDomainAuditHostdev(vm, detach, "detach", false); + else + qemuDomainRemoveHostDevice(driver, vm, detach); - if (virSecurityManagerRestoreHostdevLabel(driver->securityManager, - vm->def, detach, NULL) < 0) { - VIR_WARN("Failed to restore host device labelling"); - } - virDomainHostdevRemove(vm->def, idx); - virDomainHostdevDefFree(detach); - } return ret; } @@ -2732,7 +2782,7 @@ int qemuDomainDetachHostDevice(virQEMUDriverPtr driver, if (detach->parent.type == VIR_DOMAIN_DEVICE_NET) return qemuDomainDetachNetDevice(driver, vm, &detach->parent); else - return qemuDomainDetachThisHostDevice(driver, vm, detach, idx); + return qemuDomainDetachThisHostDevice(driver, vm, detach); } int @@ -2765,13 +2815,7 @@ qemuDomainDetachNetDevice(virQEMUDriverPtr driver, if (virDomainNetGetActualType(detach) == VIR_DOMAIN_NET_TYPE_HOSTDEV) { /* coverity[negative_returns] */ ret = qemuDomainDetachThisHostDevice(driver, vm, - virDomainNetGetActualHostdev(detach), - -1); - if (!ret) { - networkReleaseActualDevice(detach); - virDomainNetRemove(vm->def, detachidx); - virDomainNetDefFree(detach); - } + virDomainNetGetActualHostdev(detach)); goto cleanup; } if (STREQLEN(vm->def->os.machine, "s390-ccw", 8) && -- 1.8.3.2
On Mon, Jul 15, 2013 at 07:11:09PM +0200, Jiri Denemark wrote:
--- src/qemu/qemu_hotplug.c | 240 ++++++++++++++++++++++++++++-------------------- 1 file changed, 142 insertions(+), 98 deletions(-)
ACK Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On Tue, Jul 16, 2013 at 10:58:11 +0100, Daniel Berrange wrote:
On Mon, Jul 15, 2013 at 07:11:09PM +0200, Jiri Denemark wrote:
--- src/qemu/qemu_hotplug.c | 240 ++++++++++++++++++++++++++++-------------------- 1 file changed, 142 insertions(+), 98 deletions(-)
ACK
Pushed, thanks. Jirka
--- src/conf/domain_conf.c | 41 +++++++++++++++++++++++++++++++++++++++++ src/conf/domain_conf.h | 4 ++++ src/libvirt_private.syms | 1 + 3 files changed, 46 insertions(+) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 602c9a6..1520ec5 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -18252,3 +18252,44 @@ virDomainDiskDefGenSecurityLabelDef(const char *model) return seclabel; } + + +typedef struct { + const char *devAlias; + virDomainDeviceDefPtr dev; +} virDomainDefFindDeviceCallbackData; + +static int +virDomainDefFindDeviceCallback(virDomainDefPtr def ATTRIBUTE_UNUSED, + virDomainDeviceDefPtr dev, + virDomainDeviceInfoPtr info, + void *opaque) +{ + virDomainDefFindDeviceCallbackData *data = opaque; + + if (STREQ_NULLABLE(info->alias, data->devAlias)) { + *data->dev = *dev; + return -1; + } + return 0; +} + +int +virDomainDefFindDevice(virDomainDefPtr def, + const char *devAlias, + virDomainDeviceDefPtr dev) +{ + virDomainDefFindDeviceCallbackData data = { devAlias, dev }; + + dev->type = VIR_DOMAIN_DEVICE_NONE; + virDomainDeviceInfoIterateInternal(def, virDomainDefFindDeviceCallback, + true, &data); + + if (dev->type == VIR_DOMAIN_DEVICE_NONE) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("no device found with alias %s"), devAlias); + return -1; + } + + return 0; +} diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index ef72d24..f2a78fa 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2684,4 +2684,8 @@ virDomainDefMaybeAddController(virDomainDefPtr def, char *virDomainDefGetDefaultEmulator(virDomainDefPtr def, virCapsPtr caps); +int virDomainDefFindDevice(virDomainDefPtr def, + const char *devAlias, + virDomainDeviceDefPtr dev); + #endif /* __DOMAIN_CONF_H */ diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 49f07c1..ae6f7a2 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -151,6 +151,7 @@ virDomainDefClearDeviceAliases; virDomainDefClearPCIAddresses; virDomainDefCompatibleDevice; virDomainDefCopy; +virDomainDefFindDevice; virDomainDefFormat; virDomainDefFormatInternal; virDomainDefFree; -- 1.8.3.2
On Mon, Jul 15, 2013 at 07:11:10PM +0200, Jiri Denemark wrote:
--- src/conf/domain_conf.c | 41 +++++++++++++++++++++++++++++++++++++++++ src/conf/domain_conf.h | 4 ++++ src/libvirt_private.syms | 1 + 3 files changed, 46 insertions(+)
Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
--- src/qemu/qemu_capabilities.c | 2 ++ src/qemu/qemu_capabilities.h | 1 + src/qemu/qemu_monitor.c | 13 +++++++++++++ src/qemu/qemu_monitor.h | 5 +++++ src/qemu/qemu_monitor_json.c | 15 +++++++++++++++ 5 files changed, 36 insertions(+) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index baaaefe..16a3664 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -233,6 +233,7 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST, "mlock", "vnc-share-policy", /* 150 */ + "device-del-event", ); struct _virQEMUCaps { @@ -1335,6 +1336,7 @@ struct virQEMUCapsStringFlags virQEMUCapsCommands[] = { struct virQEMUCapsStringFlags virQEMUCapsEvents[] = { { "BALLOON_CHANGE", QEMU_CAPS_BALLOON_EVENT }, { "SPICE_MIGRATE_COMPLETED", QEMU_CAPS_SEAMLESS_MIGRATION }, + { "DEVICE_DELETED", QEMU_CAPS_DEVICE_DEL_EVENT }, }; struct virQEMUCapsStringFlags virQEMUCapsObjectTypes[] = { diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index 7088747..f5f685d 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -189,6 +189,7 @@ enum virQEMUCapsFlags { QEMU_CAPS_DRIVE_DISCARD = 148, /* -drive discard=off(ignore)|on(unmap) */ QEMU_CAPS_MLOCK = 149, /* -realtime mlock=on|off */ QEMU_CAPS_VNC_SHARE_POLICY = 150, /* set display sharing policy */ + QEMU_CAPS_DEVICE_DEL_EVENT = 151, /* DEVICE_DELETED event */ QEMU_CAPS_LAST, /* this must always be the last item */ }; diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index c1ac493..566c648 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -1162,6 +1162,19 @@ int qemuMonitorEmitBalloonChange(qemuMonitorPtr mon, } +int +qemuMonitorEmitDeviceDeleted(qemuMonitorPtr mon, + const char *devAlias) +{ + int ret = -1; + VIR_DEBUG("mon=%p", mon); + + QEMU_MONITOR_CALLBACK(mon, ret, domainDeviceDeleted, mon->vm, devAlias); + + return ret; +} + + int qemuMonitorSetCapabilities(qemuMonitorPtr mon) { int ret; diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index 4d83bef..a985fec 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -142,6 +142,9 @@ struct _qemuMonitorCallbacks { virDomainObjPtr vm); int (*domainGuestPanic)(qemuMonitorPtr mon, virDomainObjPtr vm); + int (*domainDeviceDeleted)(qemuMonitorPtr mon, + virDomainObjPtr vm, + const char *devAlias); }; char *qemuMonitorEscapeArg(const char *in); @@ -223,6 +226,8 @@ int qemuMonitorEmitBalloonChange(qemuMonitorPtr mon, unsigned long long actual); int qemuMonitorEmitPMSuspendDisk(qemuMonitorPtr mon); int qemuMonitorEmitGuestPanic(qemuMonitorPtr mon); +int qemuMonitorEmitDeviceDeleted(qemuMonitorPtr mon, + const char *devAlias); int qemuMonitorStartCPUs(qemuMonitorPtr mon, virConnectPtr conn); diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 81af4f3..ec4030c 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -75,6 +75,7 @@ static void qemuMonitorJSONHandleBlockJobReady(qemuMonitorPtr mon, virJSONValueP static void qemuMonitorJSONHandleBalloonChange(qemuMonitorPtr mon, virJSONValuePtr data); static void qemuMonitorJSONHandlePMSuspendDisk(qemuMonitorPtr mon, virJSONValuePtr data); static void qemuMonitorJSONHandleGuestPanic(qemuMonitorPtr mon, virJSONValuePtr data); +static void qemuMonitorJSONHandleDeviceDeleted(qemuMonitorPtr mon, virJSONValuePtr data); typedef struct { const char *type; @@ -87,6 +88,7 @@ static qemuEventHandler eventHandlers[] = { { "BLOCK_JOB_CANCELLED", qemuMonitorJSONHandleBlockJobCanceled, }, { "BLOCK_JOB_COMPLETED", qemuMonitorJSONHandleBlockJobCompleted, }, { "BLOCK_JOB_READY", qemuMonitorJSONHandleBlockJobReady, }, + { "DEVICE_DELETED", qemuMonitorJSONHandleDeviceDeleted, }, { "DEVICE_TRAY_MOVED", qemuMonitorJSONHandleTrayChange, }, { "GUEST_PANICKED", qemuMonitorJSONHandleGuestPanic, }, { "POWERDOWN", qemuMonitorJSONHandlePowerdown, }, @@ -920,6 +922,19 @@ qemuMonitorJSONHandlePMSuspendDisk(qemuMonitorPtr mon, qemuMonitorEmitPMSuspendDisk(mon); } +static void +qemuMonitorJSONHandleDeviceDeleted(qemuMonitorPtr mon, virJSONValuePtr data) +{ + const char *device; + + if (!(device = virJSONValueObjectGetString(data, "device"))) { + VIR_WARN("missing device in device deleted event"); + return; + } + + qemuMonitorEmitDeviceDeleted(mon, device); +} + int qemuMonitorJSONHumanCommandWithFd(qemuMonitorPtr mon, const char *cmd_str, -- 1.8.3.2
On Mon, Jul 15, 2013 at 07:11:11PM +0200, Jiri Denemark wrote:
--- src/qemu/qemu_capabilities.c | 2 ++ src/qemu/qemu_capabilities.h | 1 + src/qemu/qemu_monitor.c | 13 +++++++++++++ src/qemu/qemu_monitor.h | 5 +++++ src/qemu/qemu_monitor_json.c | 15 +++++++++++++++ 5 files changed, 36 insertions(+)
ACK Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
--- src/qemu/qemu_domain.h | 2 + src/qemu/qemu_hotplug.c | 101 +++++++++++++++++++++++++++++++++++++++++++++--- src/qemu/qemu_hotplug.h | 3 ++ src/qemu/qemu_process.c | 41 ++++++++++++++++++++ 4 files changed, 142 insertions(+), 5 deletions(-) diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 750841f..ba273ed 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -168,6 +168,8 @@ struct _qemuDomainObjPrivate { size_t ncleanupCallbacks_max; virCgroupPtr cgroup; + + const char *deletingDevice; /* alias of the device that is being deleted */ }; typedef enum { diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 169cce2..8c2f756 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -2347,6 +2347,49 @@ qemuDomainRemoveHostDevice(virQEMUDriverPtr driver, } +void +qemuDomainRemoveDevice(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainDeviceDefPtr dev) +{ + switch ((virDomainDeviceType) dev->type) { + case VIR_DOMAIN_DEVICE_DISK: + qemuDomainRemoveDiskDevice(driver, vm, dev->data.disk); + break; + case VIR_DOMAIN_DEVICE_CONTROLLER: + qemuDomainRemoveControllerDevice(driver, vm, dev->data.controller); + break; + case VIR_DOMAIN_DEVICE_NET: + qemuDomainRemoveNetDevice(driver, vm, dev->data.net); + break; + case VIR_DOMAIN_DEVICE_HOSTDEV: + qemuDomainRemoveHostDevice(driver, vm, dev->data.hostdev); + break; + + case VIR_DOMAIN_DEVICE_NONE: + case VIR_DOMAIN_DEVICE_LEASE: + case VIR_DOMAIN_DEVICE_FS: + case VIR_DOMAIN_DEVICE_INPUT: + case VIR_DOMAIN_DEVICE_SOUND: + case VIR_DOMAIN_DEVICE_VIDEO: + case VIR_DOMAIN_DEVICE_WATCHDOG: + case VIR_DOMAIN_DEVICE_GRAPHICS: + case VIR_DOMAIN_DEVICE_HUB: + case VIR_DOMAIN_DEVICE_REDIRDEV: + case VIR_DOMAIN_DEVICE_SMARTCARD: + case VIR_DOMAIN_DEVICE_CHR: + case VIR_DOMAIN_DEVICE_MEMBALLOON: + case VIR_DOMAIN_DEVICE_NVRAM: + case VIR_DOMAIN_DEVICE_RNG: + case VIR_DOMAIN_DEVICE_LAST: + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, + _("don't know how to remove a %s device"), + virDomainDeviceTypeToString(dev->type)); + break; + } +} + + int qemuDomainDetachVirtioDiskDevice(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainDiskDefPtr detach) @@ -2385,6 +2428,11 @@ int qemuDomainDetachVirtioDiskDevice(virQEMUDriverPtr driver, QEMU_DRIVE_HOST_PREFIX, detach->info.alias) < 0) goto cleanup; + if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE_DEL_EVENT)) + priv->deletingDevice = detach->info.alias; + else + priv->deletingDevice = NULL; + qemuDomainObjEnterMonitor(driver, vm); if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) { if (qemuMonitorDelDevice(priv->mon, detach->info.alias) < 0) { @@ -2406,10 +2454,12 @@ int qemuDomainDetachVirtioDiskDevice(virQEMUDriverPtr driver, qemuDomainObjExitMonitor(driver, vm); - qemuDomainRemoveDiskDevice(driver, vm, detach); + if (!priv->deletingDevice) + qemuDomainRemoveDiskDevice(driver, vm, detach); ret = 0; cleanup: + priv->deletingDevice = NULL; VIR_FREE(drivestr); return ret; } @@ -2442,6 +2492,11 @@ int qemuDomainDetachDiskDevice(virQEMUDriverPtr driver, QEMU_DRIVE_HOST_PREFIX, detach->info.alias) < 0) goto cleanup; + if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE_DEL_EVENT)) + priv->deletingDevice = detach->info.alias; + else + priv->deletingDevice = NULL; + qemuDomainObjEnterMonitor(driver, vm); if (qemuMonitorDelDevice(priv->mon, detach->info.alias) < 0) { qemuDomainObjExitMonitor(driver, vm); @@ -2454,10 +2509,12 @@ int qemuDomainDetachDiskDevice(virQEMUDriverPtr driver, qemuDomainObjExitMonitor(driver, vm); - qemuDomainRemoveDiskDevice(driver, vm, detach); + if (!priv->deletingDevice) + qemuDomainRemoveDiskDevice(driver, vm, detach); ret = 0; cleanup: + priv->deletingDevice = NULL; VIR_FREE(drivestr); return ret; } @@ -2557,6 +2614,11 @@ int qemuDomainDetachPciControllerDevice(virQEMUDriverPtr driver, goto cleanup; } + if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE_DEL_EVENT)) + priv->deletingDevice = detach->info.alias; + else + priv->deletingDevice = NULL; + qemuDomainObjEnterMonitor(driver, vm); if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) { if (qemuMonitorDelDevice(priv->mon, detach->info.alias)) { @@ -2572,11 +2634,13 @@ int qemuDomainDetachPciControllerDevice(virQEMUDriverPtr driver, } qemuDomainObjExitMonitor(driver, vm); - qemuDomainRemoveControllerDevice(driver, vm, detach); + if (!priv->deletingDevice) + qemuDomainRemoveControllerDevice(driver, vm, detach); ret = 0; cleanup: + priv->deletingDevice = NULL; return ret; } @@ -2604,6 +2668,11 @@ qemuDomainDetachHostPciDevice(virQEMUDriverPtr driver, return -1; } + if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE_DEL_EVENT)) + priv->deletingDevice = detach->info->alias; + else + priv->deletingDevice = NULL; + qemuDomainObjEnterMonitor(driver, vm); if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) { ret = qemuMonitorDelDevice(priv->mon, detach->info->alias); @@ -2635,6 +2704,11 @@ qemuDomainDetachHostUsbDevice(virQEMUDriverPtr driver, return -1; } + if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE_DEL_EVENT)) + priv->deletingDevice = detach->info->alias; + else + priv->deletingDevice = NULL; + qemuDomainObjEnterMonitor(driver, vm); ret = qemuMonitorDelDevice(priv->mon, detach->info->alias); qemuDomainObjExitMonitor(driver, vm); @@ -2670,6 +2744,11 @@ qemuDomainDetachHostScsiDevice(virQEMUDriverPtr driver, if (!(devstr = qemuBuildSCSIHostdevDevStr(vm->def, detach, priv->qemuCaps))) goto cleanup; + if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE_DEL_EVENT)) + priv->deletingDevice = detach->info->alias; + else + priv->deletingDevice = NULL; + qemuDomainObjEnterMonitor(driver, vm); if ((ret = qemuMonitorDelDevice(priv->mon, detach->info->alias)) == 0) { if ((ret = qemuMonitorDriveDel(priv->mon, drvstr)) < 0) { @@ -2697,6 +2776,7 @@ qemuDomainDetachThisHostDevice(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainHostdevDefPtr detach) { + qemuDomainObjPrivatePtr priv = vm->privateData; int ret = -1; switch (detach->source.subsys.type) { @@ -2718,9 +2798,11 @@ qemuDomainDetachThisHostDevice(virQEMUDriverPtr driver, if (ret < 0) virDomainAuditHostdev(vm, detach, "detach", false); - else + else if (!priv->deletingDevice) qemuDomainRemoveHostDevice(driver, vm, detach); + priv->deletingDevice = NULL; + return ret; } @@ -2851,6 +2933,11 @@ qemuDomainDetachNetDevice(virQEMUDriverPtr driver, if (virAsprintf(&hostnet_name, "host%s", detach->info.alias) < 0) goto cleanup; + if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE_DEL_EVENT)) + priv->deletingDevice = detach->info.alias; + else + priv->deletingDevice = NULL; + qemuDomainObjEnterMonitor(driver, vm); if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) { if (qemuMonitorDelDevice(priv->mon, detach->info.alias) < 0) { @@ -2883,9 +2970,13 @@ qemuDomainDetachNetDevice(virQEMUDriverPtr driver, } qemuDomainObjExitMonitor(driver, vm); - qemuDomainRemoveNetDevice(driver, vm, detach); + if (!priv->deletingDevice) + qemuDomainRemoveNetDevice(driver, vm, detach); + ret = 0; + cleanup: + priv->deletingDevice = NULL; VIR_FREE(hostnet_name); return ret; } diff --git a/src/qemu/qemu_hotplug.h b/src/qemu/qemu_hotplug.h index da802ee..56acb1a 100644 --- a/src/qemu/qemu_hotplug.h +++ b/src/qemu/qemu_hotplug.h @@ -105,5 +105,8 @@ int qemuDomainDetachLease(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainLeaseDefPtr lease); +void qemuDomainRemoveDevice(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainDeviceDefPtr dev); #endif /* __QEMU_HOTPLUG_H__ */ diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 574abf2..d73bc70 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -1313,6 +1313,46 @@ cleanup: } +static int +qemuProcessHandleDeviceDeleted(qemuMonitorPtr mon ATTRIBUTE_UNUSED, + virDomainObjPtr vm, + const char *devAlias) +{ + virQEMUDriverPtr driver = qemu_driver; + virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); + qemuDomainObjPrivatePtr priv; + virDomainDeviceDef dev; + + virObjectLock(vm); + + VIR_DEBUG("Device %s removed from domain %p %s", + devAlias, vm, vm->def->name); + + priv = vm->privateData; + + if (STREQ_NULLABLE(priv->deletingDevice, devAlias)) { + /* We got this event before the command that requested removal of + * devAlias returned. Clear deletingDevice so that the unplugging + * code know it has to remove the device. + */ + priv->deletingDevice = NULL; + } else { + if (virDomainDefFindDevice(vm->def, devAlias, &dev) < 0) + goto cleanup; + + qemuDomainRemoveDevice(driver, vm, &dev); + + if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm) < 0) + VIR_WARN("unable to save domain status with balloon change"); + } + +cleanup: + virObjectUnlock(vm); + virObjectUnref(cfg); + return 0; +} + + static qemuMonitorCallbacks monitorCallbacks = { .destroy = qemuProcessHandleMonitorDestroy, .eofNotify = qemuProcessHandleMonitorEOF, @@ -1333,6 +1373,7 @@ static qemuMonitorCallbacks monitorCallbacks = { .domainBalloonChange = qemuProcessHandleBalloonChange, .domainPMSuspendDisk = qemuProcessHandlePMSuspendDisk, .domainGuestPanic = qemuProcessHandleGuestPanic, + .domainDeviceDeleted = qemuProcessHandleDeviceDeleted, }; static int -- 1.8.3.2
On Mon, Jul 15, 2013 at 07:11:12PM +0200, Jiri Denemark wrote:
--- src/qemu/qemu_domain.h | 2 + src/qemu/qemu_hotplug.c | 101 +++++++++++++++++++++++++++++++++++++++++++++--- src/qemu/qemu_hotplug.h | 3 ++ src/qemu/qemu_process.c | 41 ++++++++++++++++++++ 4 files changed, 142 insertions(+), 5 deletions(-)
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 750841f..ba273ed 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -168,6 +168,8 @@ struct _qemuDomainObjPrivate { size_t ncleanupCallbacks_max;
virCgroupPtr cgroup; + + const char *deletingDevice; /* alias of the device that is being deleted */
This field just feels very wrong to me. It is storing state related to a single method call, outside the execution lifetime of the method call.
@@ -2385,6 +2428,11 @@ int qemuDomainDetachVirtioDiskDevice(virQEMUDriverPtr driver, QEMU_DRIVE_HOST_PREFIX, detach->info.alias) < 0) goto cleanup;
+ if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE_DEL_EVENT)) + priv->deletingDevice = detach->info.alias; + else + priv->deletingDevice = NULL; + qemuDomainObjEnterMonitor(driver, vm); if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) { if (qemuMonitorDelDevice(priv->mon, detach->info.alias) < 0) {
Can't we get rid of this bit here...
@@ -2406,10 +2454,12 @@ int qemuDomainDetachVirtioDiskDevice(virQEMUDriverPtr driver,
qemuDomainObjExitMonitor(driver, vm);
- qemuDomainRemoveDiskDevice(driver, vm, detach); + if (!priv->deletingDevice) + qemuDomainRemoveDiskDevice(driver, vm, detach); ret = 0;
And change this to if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE_DEL_EVENT)) qemuDomainRemoveDiskDevice(driver, vm, detach);
+static int +qemuProcessHandleDeviceDeleted(qemuMonitorPtr mon ATTRIBUTE_UNUSED, + virDomainObjPtr vm, + const char *devAlias) +{ + virQEMUDriverPtr driver = qemu_driver; + virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); + qemuDomainObjPrivatePtr priv; + virDomainDeviceDef dev; + + virObjectLock(vm); + + VIR_DEBUG("Device %s removed from domain %p %s", + devAlias, vm, vm->def->name); + + priv = vm->privateData; + + if (STREQ_NULLABLE(priv->deletingDevice, devAlias)) { + /* We got this event before the command that requested removal of + * devAlias returned. Clear deletingDevice so that the unplugging + * code know it has to remove the device. + */ + priv->deletingDevice = NULL; + } else { + if (virDomainDefFindDevice(vm->def, devAlias, &dev) < 0) + goto cleanup; + + qemuDomainRemoveDevice(driver, vm, &dev); + + if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm) < 0) + VIR_WARN("unable to save domain status with balloon change"); + }
And kill the first part of this if() block, leaving only the else(), so removal of the device is fully delegated to this callback when the event is known to exist. Also, if we have the event, I think we ought to make the virDomainDetachDevice() API block waiting for arrival of the event, for some reasonable finite period of time. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On Mon, Jul 15, 2013 at 18:25:58 +0100, Daniel Berrange wrote:
On Mon, Jul 15, 2013 at 07:11:12PM +0200, Jiri Denemark wrote:
--- src/qemu/qemu_domain.h | 2 + src/qemu/qemu_hotplug.c | 101 +++++++++++++++++++++++++++++++++++++++++++++--- src/qemu/qemu_hotplug.h | 3 ++ src/qemu/qemu_process.c | 41 ++++++++++++++++++++ 4 files changed, 142 insertions(+), 5 deletions(-)
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 750841f..ba273ed 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -168,6 +168,8 @@ struct _qemuDomainObjPrivate { size_t ncleanupCallbacks_max;
virCgroupPtr cgroup; + + const char *deletingDevice; /* alias of the device that is being deleted */
This field just feels very wrong to me. It is storing state related to a single method call, outside the execution lifetime of the method call.
No, actually it's storing state for only a part of the method call. It is set before the method enters monitor and it is checked and reset when the method returns from monitor. This is all because DEVICE_DELETED may be delivered before the monitor command that caused it returns, in which case we don't want the event handler to remove that device but rather let the method itself finish its removal. Alternatively, we could remove the device directly in DEVICE_DELETED handler everytime but we would still need a way to notify the method that its device has already been removed. Although we could change it into a condition and move it to the monitor internal data structure. ...
+static int +qemuProcessHandleDeviceDeleted(qemuMonitorPtr mon ATTRIBUTE_UNUSED, + virDomainObjPtr vm, + const char *devAlias) +{ + virQEMUDriverPtr driver = qemu_driver; + virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); + qemuDomainObjPrivatePtr priv; + virDomainDeviceDef dev; + + virObjectLock(vm); + + VIR_DEBUG("Device %s removed from domain %p %s", + devAlias, vm, vm->def->name); + + priv = vm->privateData; + + if (STREQ_NULLABLE(priv->deletingDevice, devAlias)) { + /* We got this event before the command that requested removal of + * devAlias returned. Clear deletingDevice so that the unplugging + * code know it has to remove the device. + */ + priv->deletingDevice = NULL; + } else { + if (virDomainDefFindDevice(vm->def, devAlias, &dev) < 0) + goto cleanup; + + qemuDomainRemoveDevice(driver, vm, &dev); + + if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm) < 0) + VIR_WARN("unable to save domain status with balloon change"); + }
And kill the first part of this if() block, leaving only the else(), so removal of the device is fully delegated to this callback when the event is known to exist.
Yes, that would be an alternative approach.
Also, if we have the event, I think we ought to make the virDomainDetachDevice() API block waiting for arrival of the event, for some reasonable finite period of time.
Hmm, that would work too and would probably provide a better user experience esp. for interactive usage. In other words, we would pretend that any device which finished unplug in 5 or 10 seconds was removed synchronously. Sounds reasonable. Jirka
--- src/qemu/qemu_hotplug.c | 22 +++++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 8c2f756..ea62e71 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -2105,6 +2105,7 @@ qemuDomainRemoveDiskDevice(virQEMUDriverPtr driver, virDomainDiskDefPtr disk) { virDomainDeviceDef dev; + virDomainEventPtr event; size_t i; VIR_DEBUG("Removing disk %s from domain %p %s", @@ -2112,6 +2113,10 @@ qemuDomainRemoveDiskDevice(virQEMUDriverPtr driver, virDomainAuditDisk(vm, disk->src, NULL, "detach", true); + event = virDomainEventDeviceRemovedNewFromObj(vm, disk->info.alias); + if (event) + qemuDomainEventQueue(driver, event); + for (i = 0; i < vm->def->ndisks; i++) { if (vm->def->disks[i] == disk) { virDomainDiskRemove(vm->def, i); @@ -2140,15 +2145,20 @@ qemuDomainRemoveDiskDevice(virQEMUDriverPtr driver, static void -qemuDomainRemoveControllerDevice(virQEMUDriverPtr driver ATTRIBUTE_UNUSED, +qemuDomainRemoveControllerDevice(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainControllerDefPtr controller) { + virDomainEventPtr event; size_t i; VIR_DEBUG("Removing controller %s from domain %p %s", controller->info.alias, vm, vm->def->name); + event = virDomainEventDeviceRemovedNewFromObj(vm, controller->info.alias); + if (event) + qemuDomainEventQueue(driver, event); + for (i = 0; i < vm->def->ncontrollers; i++) { if (vm->def->controllers[i] == controller) { virDomainControllerRemove(vm->def, i); @@ -2168,6 +2178,7 @@ qemuDomainRemoveNetDevice(virQEMUDriverPtr driver, { virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); virNetDevVPortProfilePtr vport; + virDomainEventPtr event; size_t i; VIR_DEBUG("Removing network interface %s from domain %p %s", @@ -2175,6 +2186,10 @@ qemuDomainRemoveNetDevice(virQEMUDriverPtr driver, virDomainAuditNet(vm, net, NULL, "detach", true); + event = virDomainEventDeviceRemovedNewFromObj(vm, net->info.alias); + if (event) + qemuDomainEventQueue(driver, event); + for (i = 0; i < vm->def->nnets; i++) { if (vm->def->nets[i] == net) { virDomainNetRemove(vm->def, i); @@ -2291,11 +2306,16 @@ qemuDomainRemoveHostDevice(virQEMUDriverPtr driver, virDomainHostdevDefPtr hostdev) { virDomainNetDefPtr net = NULL; + virDomainEventPtr event; size_t i; VIR_DEBUG("Removing host device %s from domain %p %s", hostdev->info->alias, vm, vm->def->name); + event = virDomainEventDeviceRemovedNewFromObj(vm, hostdev->info->alias); + if (event) + qemuDomainEventQueue(driver, event); + if (hostdev->parent.type == VIR_DOMAIN_DEVICE_NET) { net = hostdev->parent.data.net; -- 1.8.3.2
participants (2)
-
Daniel P. Berrange -
Jiri Denemark