Definition of public API for virDomain{Attach,Detach}DeviceFlags. --- include/libvirt/libvirt.h.in | 13 +++++++++++++ src/libvirt_public.syms | 6 ++++++ 2 files changed, 19 insertions(+), 0 deletions(-) diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index f192fb1..99a5c45 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -845,9 +845,22 @@ int virDomainGetVcpus (virDomainPtr domain, */ #define VIR_GET_CPUMAP(cpumaps,maplen,vcpu) &(cpumaps[(vcpu)*(maplen)]) + +typedef enum { + + VIR_DOMAIN_DEVICE_MODIFY_CURRENT = 0, /* Modify device allocation based on current domain state */ + VIR_DOMAIN_DEVICE_MODIFY_LIVE = (1 << 0), /* Modify live device allocation */ + VIR_DOMAIN_DEVICE_MODIFY_CONFIG = (1 << 1), /* Modify persisted device allocation */ +} virDomainDeviceModifyFlags; + int virDomainAttachDevice(virDomainPtr domain, const char *xml); int virDomainDetachDevice(virDomainPtr domain, const char *xml); +int virDomainAttachDeviceFlags(virDomainPtr domain, + const char *xml, unsigned int flags); +int virDomainDetachDeviceFlags(virDomainPtr domain, + const char *xml, unsigned int flags); + /* * NUMA support */ diff --git a/src/libvirt_public.syms b/src/libvirt_public.syms index 0521158..e190d83 100644 --- a/src/libvirt_public.syms +++ b/src/libvirt_public.syms @@ -349,4 +349,10 @@ LIBVIRT_0.7.5 { virDomainMemoryStats; } LIBVIRT_0.7.3; +LIBVIRT_0.7.6 { + global: + virDomainAttachDeviceFlags; + virDomainDetachDeviceFlags; +} LIBVIRT_0.7.5; + # .... define new API here using predicted next version number .... -- 1.6.0.2

Implementation of public API for virDomain{Attach,Detach}DeviceFlags. --- src/libvirt.c | 106 ++++++++++++++++++++++++++++++++++++++++++++++++++++---- 1 files changed, 98 insertions(+), 8 deletions(-) diff --git a/src/libvirt.c b/src/libvirt.c index 1145561..77f76bc 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -5129,7 +5129,6 @@ error: int virDomainAttachDevice(virDomainPtr domain, const char *xml) { - virConnectPtr conn; DEBUG("domain=%p, xml=%s", domain, xml); virResetLastError(); @@ -5147,17 +5146,63 @@ virDomainAttachDevice(virDomainPtr domain, const char *xml) virLibDomainError(domain, VIR_ERR_OPERATION_INVALID, __FUNCTION__); goto error; } + + return virDomainAttachDeviceFlags(domain, xml, + VIR_DOMAIN_DEVICE_MODIFY_LIVE); + +error: + virDispatchError(domain->conn); + return -1; +} + +/** + * virDomainAttachDeviceFlags: + * @domain: pointer to domain object + * @xml: pointer to XML description of one device + * @flags: an OR'ed set of virDomainDeviceModifyFlags + * + * Attach a virtual device to a domain, using the flags parameter + * to control how the device is attached. VIR_DOMAIN_DEVICE_MODIFY_CURRENT + * specifies that the device allocation is made based on current domain + * state. VIR_DOMAIN_DEVICE_MODIFY_LIVE specifies that the device shall be + * allocated to the active domain instance only and is not added to the + * persisted domain configuration. VIR_DOMAIN_DEVICE_MODIFY_CONFIG + * specifies that the device shall be allocated to the persisted domain + * configuration only. Note that the target hypervisor must return an + * error if unable to satisfy flags. E.g. the hypervisor driver will + * return failure if LIVE is specified but it only supports modifying the + * persisted device allocation. + * + * Returns 0 in case of success, -1 in case of failure. + */ +int +virDomainAttachDeviceFlags(virDomainPtr domain, + const char *xml, unsigned int flags) +{ + virConnectPtr conn; + DEBUG("domain=%p, xml=%s, flags=%d", domain, xml, flags); + + virResetLastError(); + + if (!VIR_IS_CONNECTED_DOMAIN(domain)) { + virLibDomainError(NULL, VIR_ERR_INVALID_DOMAIN, __FUNCTION__); + return (-1); + } + if (domain->conn->flags & VIR_CONNECT_RO) { + virLibDomainError(domain, VIR_ERR_OPERATION_DENIED, __FUNCTION__); + goto error; + } conn = domain->conn; - if (conn->driver->domainAttachDevice) { + if (conn->driver->domainAttachDeviceFlags) { int ret; - ret = conn->driver->domainAttachDevice (domain, xml); + ret = conn->driver->domainAttachDeviceFlags(domain, xml, flags); if (ret < 0) goto error; return ret; } - virLibConnError (conn, VIR_ERR_NO_SUPPORT, __FUNCTION__); + virLibConnError(conn, VIR_ERR_NO_SUPPORT, __FUNCTION__); error: virDispatchError(domain->conn); @@ -5177,7 +5222,6 @@ error: int virDomainDetachDevice(virDomainPtr domain, const char *xml) { - virConnectPtr conn; DEBUG("domain=%p, xml=%s", domain, xml); virResetLastError(); @@ -5195,17 +5239,63 @@ virDomainDetachDevice(virDomainPtr domain, const char *xml) virLibDomainError(domain, VIR_ERR_OPERATION_INVALID, __FUNCTION__); goto error; } + + return virDomainDetachDeviceFlags(domain, xml, + VIR_DOMAIN_DEVICE_MODIFY_LIVE); + +error: + virDispatchError(domain->conn); + return -1; +} + +/** + * virDomainDetachDeviceFlags: + * @domain: pointer to domain object + * @xml: pointer to XML description of one device + * @flags: an OR'ed set of virDomainDeviceModifyFlags + * + * Detach a virtual device from a domain, using the flags parameter + * to control how the device is detached. VIR_DOMAIN_DEVICE_MODIFY_CURRENT + * specifies that the device allocation is removed based on current domain + * state. VIR_DOMAIN_DEVICE_MODIFY_LIVE specifies that the device shall be + * deallocated from the active domain instance only and is not from the + * persisted domain configuration. VIR_DOMAIN_DEVICE_MODIFY_CONFIG + * specifies that the device shall be deallocated from the persisted domain + * configuration only. Note that the target hypervisor must return an + * error if unable to satisfy flags. E.g. the hypervisor driver will + * return failure if LIVE is specified but it only supports removing the + * persisted device allocation. + * + * Returns 0 in case of success, -1 in case of failure. + */ +int +virDomainDetachDeviceFlags(virDomainPtr domain, + const char *xml, unsigned int flags) +{ + virConnectPtr conn; + DEBUG("domain=%p, xml=%s, flags=%d", domain, xml, flags); + + virResetLastError(); + + if (!VIR_IS_CONNECTED_DOMAIN(domain)) { + virLibDomainError(NULL, VIR_ERR_INVALID_DOMAIN, __FUNCTION__); + return (-1); + } + if (domain->conn->flags & VIR_CONNECT_RO) { + virLibDomainError(domain, VIR_ERR_OPERATION_DENIED, __FUNCTION__); + goto error; + } conn = domain->conn; - if (conn->driver->domainDetachDevice) { + if (conn->driver->domainDetachDeviceFlags) { int ret; - ret = conn->driver->domainDetachDevice (domain, xml); + ret = conn->driver->domainDetachDeviceFlags(domain, xml, flags); if (ret < 0) goto error; return ret; } - virLibConnError (conn, VIR_ERR_NO_SUPPORT, __FUNCTION__); + virLibConnError(conn, VIR_ERR_NO_SUPPORT, __FUNCTION__); error: virDispatchError(domain->conn); -- 1.6.0.2

Implementation of Domain{Attach,Detach}DeviceFlags in remote driver. --- src/remote/remote_driver.c | 54 ++++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 54 insertions(+), 0 deletions(-) diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index d6f5fce..eb16f62 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -3057,6 +3057,32 @@ done: } static int +remoteDomainAttachDeviceFlags (virDomainPtr domain, const char *xml, + unsigned int flags) +{ + int rv = -1; + remote_domain_attach_device_flags_args args; + struct private_data *priv = domain->conn->privateData; + + remoteDriverLock(priv); + + make_nonnull_domain (&args.dom, domain); + args.xml = (char *) xml; + args.flags = flags; + + if (call (domain->conn, priv, 0, REMOTE_PROC_DOMAIN_ATTACH_DEVICE_FLAGS, + (xdrproc_t) xdr_remote_domain_attach_device_flags_args, (char *) &args, + (xdrproc_t) xdr_void, (char *) NULL) == -1) + goto done; + + rv = 0; + +done: + remoteDriverUnlock(priv); + return rv; +} + +static int remoteDomainDetachDevice (virDomainPtr domain, const char *xml) { int rv = -1; @@ -3081,6 +3107,32 @@ done: } static int +remoteDomainDetachDeviceFlags (virDomainPtr domain, const char *xml, + unsigned int flags) +{ + int rv = -1; + remote_domain_detach_device_flags_args args; + struct private_data *priv = domain->conn->privateData; + + remoteDriverLock(priv); + + make_nonnull_domain (&args.dom, domain); + args.xml = (char *) xml; + args.flags = flags; + + if (call (domain->conn, priv, 0, REMOTE_PROC_DOMAIN_DETACH_DEVICE_FLAGS, + (xdrproc_t) xdr_remote_domain_detach_device_flags_args, (char *) &args, + (xdrproc_t) xdr_void, (char *) NULL) == -1) + goto done; + + rv = 0; + +done: + remoteDriverUnlock(priv); + return rv; +} + +static int remoteDomainGetAutostart (virDomainPtr domain, int *autostart) { int rv = -1; @@ -8894,7 +8946,9 @@ static virDriver remote_driver = { remoteDomainDefineXML, /* domainDefineXML */ remoteDomainUndefine, /* domainUndefine */ remoteDomainAttachDevice, /* domainAttachDevice */ + remoteDomainAttachDeviceFlags, /* domainAttachDeviceFlags */ remoteDomainDetachDevice, /* domainDetachDevice */ + remoteDomainDetachDeviceFlags, /* domainDetachDeviceFlags */ remoteDomainGetAutostart, /* domainGetAutostart */ remoteDomainSetAutostart, /* domainSetAutostart */ remoteDomainGetSchedulerType, /* domainGetSchedulerType */ -- 1.6.0.2

Implementation of domain{Attach,Detach}DeviceFlags handlers in the drivers. --- src/esx/esx_driver.c | 2 + src/lxc/lxc_driver.c | 2 + src/opennebula/one_driver.c | 2 + src/openvz/openvz_driver.c | 2 + src/phyp/phyp_driver.c | 2 + src/qemu/qemu_driver.c | 26 +++++++++++ src/test/test_driver.c | 2 + src/uml/uml_driver.c | 2 + src/vbox/vbox_tmpl.c | 24 ++++++++++ src/xen/proxy_internal.c | 4 +- src/xen/xen_driver.c | 42 +++++++++++++++++-- src/xen/xen_driver.h | 4 +- src/xen/xen_hypervisor.c | 4 +- src/xen/xen_inotify.c | 4 +- src/xen/xend_internal.c | 98 +++++++++++++++++++++++++++++++++++-------- src/xen/xm_internal.c | 30 +++++++++---- src/xen/xs_internal.c | 4 +- 17 files changed, 212 insertions(+), 42 deletions(-) diff --git a/src/esx/esx_driver.c b/src/esx/esx_driver.c index ddda66e..577c1c7 100644 --- a/src/esx/esx_driver.c +++ b/src/esx/esx_driver.c @@ -3443,7 +3443,9 @@ static virDriver esxDriver = { esxDomainDefineXML, /* domainDefineXML */ esxDomainUndefine, /* domainUndefine */ NULL, /* domainAttachDevice */ + NULL, /* domainAttachDeviceFlags */ NULL, /* domainDetachDevice */ + NULL, /* domainDetachDeviceFlags */ NULL, /* domainGetAutostart */ NULL, /* domainSetAutostart */ esxDomainGetSchedulerType, /* domainGetSchedulerType */ diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index 86606c7..d80f20c 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -2427,7 +2427,9 @@ static virDriver lxcDriver = { lxcDomainDefine, /* domainDefineXML */ lxcDomainUndefine, /* domainUndefine */ NULL, /* domainAttachDevice */ + NULL, /* domainAttachDeviceFlags */ NULL, /* domainDetachDevice */ + NULL, /* domainDetachDeviceFlags */ lxcDomainGetAutostart, /* domainGetAutostart */ lxcDomainSetAutostart, /* domainSetAutostart */ lxcGetSchedulerType, /* domainGetSchedulerType */ diff --git a/src/opennebula/one_driver.c b/src/opennebula/one_driver.c index ad7faca..509cbc3 100644 --- a/src/opennebula/one_driver.c +++ b/src/opennebula/one_driver.c @@ -754,7 +754,9 @@ static virDriver oneDriver = { oneDomainDefine, /* domainDefineXML */ oneDomainUndefine, /* domainUndefine */ NULL, /* domainAttachDevice */ + NULL, /* domainAttachDeviceFlags */ NULL, /* domainDetachDevice */ + NULL, /* domainDetachDeviceFlags */ oneGetAutostart, /* domainGetAutostart */ NULL, /* domainSetAutostart */ NULL, /* domainGetSchedulerType */ diff --git a/src/openvz/openvz_driver.c b/src/openvz/openvz_driver.c index 196fd8c..b16efef 100644 --- a/src/openvz/openvz_driver.c +++ b/src/openvz/openvz_driver.c @@ -1506,7 +1506,9 @@ static virDriver openvzDriver = { openvzDomainDefineXML, /* domainDefineXML */ openvzDomainUndefine, /* domainUndefine */ NULL, /* domainAttachDevice */ + NULL, /* domainAttachDeviceFlags */ NULL, /* domainDetachDevice */ + NULL, /* domainDetachDeviceFlags */ openvzDomainGetAutostart, /* domainGetAutostart */ openvzDomainSetAutostart, /* domainSetAutostart */ NULL, /* domainGetSchedulerType */ diff --git a/src/phyp/phyp_driver.c b/src/phyp/phyp_driver.c index bd5cfc7..5474bda 100644 --- a/src/phyp/phyp_driver.c +++ b/src/phyp/phyp_driver.c @@ -1622,7 +1622,9 @@ virDriver phypDriver = { NULL, /* domainDefineXML */ NULL, /* domainUndefine */ NULL, /* domainAttachDevice */ + NULL, /* domainAttachDeviceFlags */ NULL, /* domainDetachDevice */ + NULL, /* domainDetachDeviceFlags */ NULL, /* domainGetAutostart */ NULL, /* domainSetAutostart */ NULL, /* domainGetSchedulerType */ diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index deb8adc..02a6212 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5544,6 +5544,18 @@ cleanup: return ret; } +static int qemudDomainAttachDeviceFlags(virDomainPtr dom, + const char *xml, + unsigned int flags) { + if (flags & VIR_DOMAIN_DEVICE_MODIFY_CONFIG) { + qemudReportError(dom->conn, dom, NULL, VIR_ERR_OPERATION_INVALID, + "%s", _("cannot modify the persistent configuration of a domain")); + return -1; + } + + return qemudDomainAttachDevice(dom, xml); +} + static int qemudDomainDetachPciDiskDevice(virConnectPtr conn, struct qemud_driver *driver, virDomainObjPtr vm, @@ -5872,6 +5884,18 @@ cleanup: return ret; } +static int qemudDomainDetachDeviceFlags(virDomainPtr dom, + const char *xml, + unsigned int flags) { + if (flags & VIR_DOMAIN_DEVICE_MODIFY_CONFIG) { + qemudReportError(dom->conn, dom, NULL, VIR_ERR_OPERATION_INVALID, + "%s", _("cannot modify the persistent configuration of a domain")); + return -1; + } + + return qemudDomainDetachDevice(dom, xml); +} + static int qemudDomainGetAutostart(virDomainPtr dom, int *autostart) { struct qemud_driver *driver = dom->conn->privateData; @@ -7985,7 +8009,9 @@ static virDriver qemuDriver = { qemudDomainDefine, /* domainDefineXML */ qemudDomainUndefine, /* domainUndefine */ qemudDomainAttachDevice, /* domainAttachDevice */ + qemudDomainAttachDeviceFlags, /* domainAttachDeviceFlags */ qemudDomainDetachDevice, /* domainDetachDevice */ + qemudDomainDetachDeviceFlags, /* domainDetachDeviceFlags */ qemudDomainGetAutostart, /* domainGetAutostart */ qemudDomainSetAutostart, /* domainSetAutostart */ qemuGetSchedulerType, /* domainGetSchedulerType */ diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 2122a1b..1feca1c 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -5209,7 +5209,9 @@ static virDriver testDriver = { testDomainDefineXML, /* domainDefineXML */ testDomainUndefine, /* domainUndefine */ NULL, /* domainAttachDevice */ + NULL, /* domainAttachDeviceFlags */ NULL, /* domainDetachDevice */ + NULL, /* domainDetachDeviceFlags */ testDomainGetAutostart, /* domainGetAutostart */ testDomainSetAutostart, /* domainSetAutostart */ testDomainGetSchedulerType, /* domainGetSchedulerType */ diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c index b808090..6adaf9f 100644 --- a/src/uml/uml_driver.c +++ b/src/uml/uml_driver.c @@ -1895,7 +1895,9 @@ static virDriver umlDriver = { umlDomainDefine, /* domainDefineXML */ umlDomainUndefine, /* domainUndefine */ NULL, /* domainAttachDevice */ + NULL, /* domainAttachDeviceFlags */ NULL, /* domainDetachDevice */ + NULL, /* domainDetachDeviceFlags */ umlDomainGetAutostart, /* domainGetAutostart */ umlDomainSetAutostart, /* domainSetAutostart */ NULL, /* domainGetSchedulerType */ diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c index 07696c0..79915c2 100644 --- a/src/vbox/vbox_tmpl.c +++ b/src/vbox/vbox_tmpl.c @@ -4837,6 +4837,17 @@ cleanup: return ret; } +static int vboxDomainAttachDeviceFlags(virDomainPtr dom, const char *xml, + unsigned int flags) { + if (flags & VIR_DOMAIN_DEVICE_MODIFY_CONFIG) { + vboxError(dom->conn, VIR_ERR_OPERATION_INVALID, "%s", + _("cannot modify the persistent configuration of a domain")); + return -1; + } + + return vboxDomainAttachDevice(dom, xml); +} + static int vboxDomainDetachDevice(virDomainPtr dom, const char *xml) { VBOX_OBJECT_CHECK(dom->conn, int, -1); IMachine *machine = NULL; @@ -4967,6 +4978,17 @@ cleanup: return ret; } +static int vboxDomainDetachDeviceFlags(virDomainPtr dom, const char *xml, + unsigned int flags) { + if (flags & VIR_DOMAIN_DEVICE_MODIFY_CONFIG) { + vboxError(dom->conn, VIR_ERR_OPERATION_INVALID, "%s", + _("cannot modify the persistent configuration of a domain")); + return -1; + } + + return vboxDomainDetachDevice(dom, xml); +} + #if VBOX_API_VERSION == 2002 /* No Callback support for VirtualBox 2.2.* series */ #else /* !(VBOX_API_VERSION == 2002) */ @@ -7013,7 +7035,9 @@ virDriver NAME(Driver) = { vboxDomainDefineXML, /* domainDefineXML */ vboxDomainUndefine, /* domainUndefine */ vboxDomainAttachDevice, /* domainAttachDevice */ + vboxDomainAttachDeviceFlags, /* domainAttachDeviceFlags */ vboxDomainDetachDevice, /* domainDetachDevice */ + vboxDomainDetachDeviceFlags, /* domainDetachDeviceFlags */ NULL, /* domainGetAutostart */ NULL, /* domainSetAutostart */ NULL, /* domainGetSchedulerType */ diff --git a/src/xen/proxy_internal.c b/src/xen/proxy_internal.c index 73a0469..33f82c2 100644 --- a/src/xen/proxy_internal.c +++ b/src/xen/proxy_internal.c @@ -76,8 +76,8 @@ struct xenUnifiedDriver xenProxyDriver = { NULL, /* domainCreate */ NULL, /* domainDefineXML */ NULL, /* domainUndefine */ - NULL, /* domainAttachDevice */ - NULL, /* domainDetachDevice */ + NULL, /* domainAttachDeviceFlags */ + NULL, /* domainDetachDeviceFlags */ NULL, /* domainGetAutostart */ NULL, /* domainSetAutostart */ NULL, /* domainGetSchedulerType */ diff --git a/src/xen/xen_driver.c b/src/xen/xen_driver.c index 4911c9e..141e415 100644 --- a/src/xen/xen_driver.c +++ b/src/xen/xen_driver.c @@ -1426,10 +1426,26 @@ xenUnifiedDomainAttachDevice (virDomainPtr dom, const char *xml) { GET_PRIVATE(dom->conn); int i; + unsigned int flags = VIR_DOMAIN_DEVICE_MODIFY_LIVE; for (i = 0; i < XEN_UNIFIED_NR_DRIVERS; ++i) - if (priv->opened[i] && drivers[i]->domainAttachDevice && - drivers[i]->domainAttachDevice (dom, xml) == 0) + if (priv->opened[i] && drivers[i]->domainAttachDeviceFlags && + drivers[i]->domainAttachDeviceFlags(dom, xml, flags) == 0) + return 0; + + return -1; +} + +static int +xenUnifiedDomainAttachDeviceFlags (virDomainPtr dom, const char *xml, + unsigned int flags) +{ + GET_PRIVATE(dom->conn); + int i; + + for (i = 0; i < XEN_UNIFIED_NR_DRIVERS; ++i) + if (priv->opened[i] && drivers[i]->domainAttachDeviceFlags && + drivers[i]->domainAttachDeviceFlags(dom, xml, flags) == 0) return 0; return -1; @@ -1440,10 +1456,26 @@ xenUnifiedDomainDetachDevice (virDomainPtr dom, const char *xml) { GET_PRIVATE(dom->conn); int i; + unsigned int flags = VIR_DOMAIN_DEVICE_MODIFY_LIVE; + + for (i = 0; i < XEN_UNIFIED_NR_DRIVERS; ++i) + if (priv->opened[i] && drivers[i]->domainDetachDeviceFlags && + drivers[i]->domainDetachDeviceFlags(dom, xml, flags) == 0) + return 0; + + return -1; +} + +static int +xenUnifiedDomainDetachDeviceFlags (virDomainPtr dom, const char *xml, + unsigned int flags) +{ + GET_PRIVATE(dom->conn); + int i; for (i = 0; i < XEN_UNIFIED_NR_DRIVERS; ++i) - if (priv->opened[i] && drivers[i]->domainDetachDevice && - drivers[i]->domainDetachDevice (dom, xml) == 0) + if (priv->opened[i] && drivers[i]->domainDetachDeviceFlags && + drivers[i]->domainDetachDeviceFlags(dom, xml, flags) == 0) return 0; return -1; @@ -1833,7 +1865,9 @@ static virDriver xenUnifiedDriver = { xenUnifiedDomainDefineXML, /* domainDefineXML */ xenUnifiedDomainUndefine, /* domainUndefine */ xenUnifiedDomainAttachDevice, /* domainAttachDevice */ + xenUnifiedDomainAttachDeviceFlags, /* domainAttachDeviceFlags */ xenUnifiedDomainDetachDevice, /* domainDetachDevice */ + xenUnifiedDomainDetachDeviceFlags, /* domainDetachDeviceFlags */ xenUnifiedDomainGetAutostart, /* domainGetAutostart */ xenUnifiedDomainSetAutostart, /* domainSetAutostart */ xenUnifiedDomainGetSchedulerType, /* domainGetSchedulerType */ diff --git a/src/xen/xen_driver.h b/src/xen/xen_driver.h index 185eb2b..defe6e2 100644 --- a/src/xen/xen_driver.h +++ b/src/xen/xen_driver.h @@ -93,8 +93,8 @@ struct xenUnifiedDriver { virDrvDomainCreate domainCreate; virDrvDomainDefineXML domainDefineXML; virDrvDomainUndefine domainUndefine; - virDrvDomainAttachDevice domainAttachDevice; - virDrvDomainDetachDevice domainDetachDevice; + virDrvDomainAttachDeviceFlags domainAttachDeviceFlags; + virDrvDomainDetachDeviceFlags domainDetachDeviceFlags; virDrvDomainGetAutostart domainGetAutostart; virDrvDomainSetAutostart domainSetAutostart; virDrvDomainGetSchedulerType domainGetSchedulerType; diff --git a/src/xen/xen_hypervisor.c b/src/xen/xen_hypervisor.c index 6d8accc..baa626f 100644 --- a/src/xen/xen_hypervisor.c +++ b/src/xen/xen_hypervisor.c @@ -793,8 +793,8 @@ struct xenUnifiedDriver xenHypervisorDriver = { NULL, /* domainCreate */ NULL, /* domainDefineXML */ NULL, /* domainUndefine */ - NULL, /* domainAttachDevice */ - NULL, /* domainDetachDevice */ + NULL, /* domainAttachDeviceFlags */ + NULL, /* domainDetachDeviceFlags */ NULL, /* domainGetAutostart */ NULL, /* domainSetAutostart */ xenHypervisorGetSchedulerType, /* domainGetSchedulerType */ diff --git a/src/xen/xen_inotify.c b/src/xen/xen_inotify.c index d9dfbb7..e3845d8 100644 --- a/src/xen/xen_inotify.c +++ b/src/xen/xen_inotify.c @@ -79,8 +79,8 @@ struct xenUnifiedDriver xenInotifyDriver = { NULL, /* domainCreate */ NULL, /* domainDefineXML */ NULL, /* domainUndefine */ - NULL, /* domainAttachDevice */ - NULL, /* domainDetachDevice */ + NULL, /* domainAttachDeviceFlags */ + NULL, /* domainDetachDeviceFlags */ NULL, /* domainGetAutostart */ NULL, /* domainSetAutostart */ NULL, /* domainGetSchedulerType */ diff --git a/src/xen/xend_internal.c b/src/xen/xend_internal.c index d9bfa15..665bc47 100644 --- a/src/xen/xend_internal.c +++ b/src/xen/xend_internal.c @@ -4096,9 +4096,10 @@ xenDaemonCreateXML(virConnectPtr conn, const char *xmlDesc, } /** - * xenDaemonAttachDevice: + * xenDaemonAttachDeviceFlags: * @domain: pointer to domain object * @xml: pointer to XML description of device + * @flags: an OR'ed set of virDomainDeviceModifyFlags * * Create a virtual device attachment to backend. * XML description is translated into S-expression. @@ -4106,7 +4107,8 @@ xenDaemonCreateXML(virConnectPtr conn, const char *xmlDesc, * Returns 0 in case of success, -1 in case of failure. */ static int -xenDaemonAttachDevice(virDomainPtr domain, const char *xml) +xenDaemonAttachDeviceFlags(virDomainPtr domain, const char *xml, + unsigned int flags) { xenUnifiedPrivatePtr priv; char *sexpr = NULL; @@ -4124,12 +4126,41 @@ xenDaemonAttachDevice(virDomainPtr domain, const char *xml) priv = (xenUnifiedPrivatePtr) domain->conn->privateData; - /* - * on older Xen without the inactive guests management - * avoid doing this on inactive guests - */ - if ((domain->id < 0) && (priv->xendConfigVersion < 3)) - return -1; + if (domain->id < 0) { + /* If xendConfigVersion < 3 only live config can be changed */ + if (priv->xendConfigVersion < 3) { + virXendError(domain->conn, VIR_ERR_OPERATION_INVALID, "%s", + _("Xend version does not support modifying " + "persisted config")); + return -1; + } + /* Cannot modify live config if domain is inactive */ + if (flags & VIR_DOMAIN_DEVICE_MODIFY_LIVE) { + virXendError(domain->conn, VIR_ERR_OPERATION_INVALID, "%s", + _("Cannot modify live config if domain is inactive")); + return -1; + } + } else { + /* Only live config can be changed if xendConfigVersion < 3 */ + if (priv->xendConfigVersion < 3 && + (flags != VIR_DOMAIN_DEVICE_MODIFY_CURRENT || + flags != VIR_DOMAIN_DEVICE_MODIFY_LIVE)) { + virXendError(domain->conn, VIR_ERR_OPERATION_INVALID, "%s", + _("Xend version does not support modifying " + "persisted config")); + return -1; + } + /* Xen only supports modifying both live and persisted config if + * xendConfigVersion >= 3 + */ + if (flags != (VIR_DOMAIN_DEVICE_MODIFY_LIVE | + VIR_DOMAIN_DEVICE_MODIFY_CONFIG)) { + virXendError(domain->conn, VIR_ERR_OPERATION_INVALID, "%s", + _("Xend only supports modifying both live and " + "persisted config")); + return -1; + } + } if (!(def = xenDaemonDomainFetch(domain->conn, domain->id, @@ -4203,16 +4234,18 @@ cleanup: } /** - * xenDaemonDetachDevice: + * xenDaemonDetachDeviceFlags: * @domain: pointer to domain object * @xml: pointer to XML description of device + * @flags: an OR'ed set of virDomainDeviceModifyFlags * * Destroy a virtual device attachment to backend. * * Returns 0 in case of success, -1 in case of failure. */ static int -xenDaemonDetachDevice(virDomainPtr domain, const char *xml) +xenDaemonDetachDeviceFlags(virDomainPtr domain, const char *xml, + unsigned int flags) { xenUnifiedPrivatePtr priv; char class[8], ref[80]; @@ -4230,12 +4263,41 @@ xenDaemonDetachDevice(virDomainPtr domain, const char *xml) priv = (xenUnifiedPrivatePtr) domain->conn->privateData; - /* - * on older Xen without the inactive guests management - * avoid doing this on inactive guests - */ - if ((domain->id < 0) && (priv->xendConfigVersion < 3)) - return -1; + if (domain->id < 0) { + /* If xendConfigVersion < 3 only live config can be changed */ + if (priv->xendConfigVersion < 3) { + virXendError(domain->conn, VIR_ERR_OPERATION_INVALID, "%s", + _("Xend version does not support modifying " + "persisted config")); + return -1; + } + /* Cannot modify live config if domain is inactive */ + if (flags & VIR_DOMAIN_DEVICE_MODIFY_LIVE) { + virXendError(domain->conn, VIR_ERR_OPERATION_INVALID, "%s", + _("Cannot modify live config if domain is inactive")); + return -1; + } + } else { + /* Only live config can be changed if xendConfigVersion < 3 */ + if (priv->xendConfigVersion < 3 && + (flags != VIR_DOMAIN_DEVICE_MODIFY_CURRENT || + flags != VIR_DOMAIN_DEVICE_MODIFY_LIVE)) { + virXendError(domain->conn, VIR_ERR_OPERATION_INVALID, "%s", + _("Xend version does not support modifying " + "persisted config")); + return -1; + } + /* Xen only supports modifying both live and persisted config if + * xendConfigVersion >= 3 + */ + if (flags != (VIR_DOMAIN_DEVICE_MODIFY_LIVE | + VIR_DOMAIN_DEVICE_MODIFY_CONFIG)) { + virXendError(domain->conn, VIR_ERR_OPERATION_INVALID, "%s", + _("Xend only supports modifying both live and " + "persisted config")); + return -1; + } + } if (!(def = xenDaemonDomainFetch(domain->conn, domain->id, @@ -5165,8 +5227,8 @@ struct xenUnifiedDriver xenDaemonDriver = { xenDaemonDomainCreate, /* domainCreate */ xenDaemonDomainDefineXML, /* domainDefineXML */ xenDaemonDomainUndefine, /* domainUndefine */ - xenDaemonAttachDevice, /* domainAttachDevice */ - xenDaemonDetachDevice, /* domainDetachDevice */ + xenDaemonAttachDeviceFlags, /* domainAttachDeviceFlags */ + xenDaemonDetachDeviceFlags, /* domainDetachDeviceFlags */ xenDaemonDomainGetAutostart, /* domainGetAutostart */ xenDaemonDomainSetAutostart, /* domainSetAutostart */ xenDaemonGetSchedulerType, /* domainGetSchedulerType */ diff --git a/src/xen/xm_internal.c b/src/xen/xm_internal.c index 944d5d5..43efe08 100644 --- a/src/xen/xm_internal.c +++ b/src/xen/xm_internal.c @@ -65,8 +65,10 @@ static int xenXMConfigSetString(virConfPtr conf, const char *setting, const char *str); char * xenXMAutoAssignMac(void); -static int xenXMDomainAttachDevice(virDomainPtr domain, const char *xml); -static int xenXMDomainDetachDevice(virDomainPtr domain, const char *xml); +static int xenXMDomainAttachDeviceFlags(virDomainPtr domain, const char *xml, + unsigned int flags); +static int xenXMDomainDetachDeviceFlags(virDomainPtr domain, const char *xml, + unsigned int flags); #define XM_REFRESH_INTERVAL 10 @@ -109,8 +111,8 @@ struct xenUnifiedDriver xenXMDriver = { xenXMDomainCreate, /* domainCreate */ xenXMDomainDefineXML, /* domainDefineXML */ xenXMDomainUndefine, /* domainUndefine */ - xenXMDomainAttachDevice, /* domainAttachDevice */ - xenXMDomainDetachDevice, /* domainDetachDevice */ + xenXMDomainAttachDeviceFlags, /* domainAttachDeviceFlags */ + xenXMDomainDetachDeviceFlags, /* domainDetachDeviceFlags */ NULL, /* domainGetAutostart */ NULL, /* domainSetAutostart */ NULL, /* domainGetSchedulerType */ @@ -2914,17 +2916,21 @@ cleanup: /** - * xenXMDomainAttachDevice: + * xenXMDomainAttachDeviceFlags: * @domain: pointer to domain object * @xml: pointer to XML description of device + * @flags: an OR'ed set of virDomainDeviceModifyFlags * * Create a virtual device attachment to backend. * XML description is translated into config file. + * This driver only supports device allocation to + * persisted config. * * Returns 0 in case of success, -1 in case of failure. */ static int -xenXMDomainAttachDevice(virDomainPtr domain, const char *xml) { +xenXMDomainAttachDeviceFlags(virDomainPtr domain, const char *xml, + unsigned int flags) { const char *filename = NULL; xenXMConfCachePtr entry = NULL; int ret = -1; @@ -2940,7 +2946,7 @@ xenXMDomainAttachDevice(virDomainPtr domain, const char *xml) { if (domain->conn->flags & VIR_CONNECT_RO) return -1; - if (domain->id != -1) + if (domain->id != -1 && !(flags & VIR_DOMAIN_DEVICE_MODIFY_CONFIG)) return -1; priv = (xenUnifiedPrivatePtr) domain->conn->privateData; @@ -3002,16 +3008,20 @@ xenXMDomainAttachDevice(virDomainPtr domain, const char *xml) { /** - * xenXMDomainDetachDevice: + * xenXMDomainDetachDeviceFlags: * @domain: pointer to domain object * @xml: pointer to XML description of device + * @flags: an OR'ed set of virDomainDeviceModifyFlags * * Destroy a virtual device attachment to backend. + * This driver only supports device deallocation from + * persisted config. * * Returns 0 in case of success, -1 in case of failure. */ static int -xenXMDomainDetachDevice(virDomainPtr domain, const char *xml) { +xenXMDomainDetachDeviceFlags(virDomainPtr domain, const char *xml, + unsigned int flags) { const char *filename = NULL; xenXMConfCachePtr entry = NULL; virDomainDeviceDefPtr dev = NULL; @@ -3029,7 +3039,7 @@ xenXMDomainDetachDevice(virDomainPtr domain, const char *xml) { if (domain->conn->flags & VIR_CONNECT_RO) return -1; - if (domain->id != -1) + if (domain->id != -1 && !(flags & VIR_DOMAIN_DEVICE_MODIFY_CONFIG)) return -1; priv = (xenUnifiedPrivatePtr) domain->conn->privateData; diff --git a/src/xen/xs_internal.c b/src/xen/xs_internal.c index 8a64d4e..1db0397 100644 --- a/src/xen/xs_internal.c +++ b/src/xen/xs_internal.c @@ -76,8 +76,8 @@ struct xenUnifiedDriver xenStoreDriver = { NULL, /* domainCreate */ NULL, /* domainDefineXML */ NULL, /* domainUndefine */ - NULL, /* domainAttachDevice */ - NULL, /* domainDetachDevice */ + NULL, /* domainAttachDeviceFlags */ + NULL, /* domainDetachDeviceFlags */ NULL, /* domainGetAutostart */ NULL, /* domainSetAutostart */ NULL, /* domainGetSchedulerType */ -- 1.6.0.2

On Thu, Jan 14, 2010 at 10:42:46AM -0700, Jim Fehlig wrote:

Change all virsh commands that invoke virDomain{Attach,Detach}Device() to use virDomain{Attach,Detach}DeviceFlags() instead.

Add a "--persistent" flag to these virsh commands, allowing user to specify that the domain persisted config be modified as well.

--- tools/virsh.c | 55 +++++++++++++++++++++++++++++++++++++++++++++++++------ 1 files changed, 49 insertions(+), 6 deletions(-)

diff --git a/tools/virsh.c b/tools/virsh.c index 1fae5e6..a082b89 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -6285,6 +6285,7 @@ static const vshCmdInfo info_attach_device[] = { static const vshCmdOptDef opts_attach_device[] = { {"domain", VSH_OT_DATA, VSH_OFLAG_REQ, gettext_noop("domain name, id or uuid")}, {"file", VSH_OT_DATA, VSH_OFLAG_REQ, gettext_noop("XML file")}, + {"persistent", VSH_OT_BOOL, 0, gettext_noop("persist device attachment")}, {NULL, 0, 0, NULL} };

@@ -6296,6 +6297,7 @@ cmdAttachDevice(vshControl *ctl, const vshCmd *cmd) char *buffer; int ret = TRUE; int found; + int flags = VIR_DOMAIN_DEVICE_MODIFY_CURRENT;

if (!vshConnectionUsability(ctl, ctl->conn, TRUE)) return FALSE; @@ -6309,13 +6311,18 @@ cmdAttachDevice(vshControl *ctl, const vshCmd *cmd) virDomainFree(dom); return FALSE; } + if (vshCommandOptBool(cmd, "persistent")) + flags |= VIR_DOMAIN_DEVICE_MODIFY_CONFIG;

if (virFileReadAll(from, VIRSH_MAX_XML_FILE, &buffer) < 0) { virDomainFree(dom); return FALSE; }

- ret = virDomainAttachDevice(dom, buffer); + if (virDomainIsActive(dom)) + flags |= VIR_DOMAIN_DEVICE_MODIFY_LIVE; + + ret = virDomainAttachDeviceFlags(dom, buffer, flags); VIR_FREE(buffer);

if (ret < 0) {

This has the same subtle compatability problem that the public API entry point suffers from. New virsh won't be able to modify guests from an existing libvirtd. I think that if flags == 0, then we should use the existing API method, and only use the new virDomainAttachDeviceFlags if flags != 0. I think we probably want to default to 0, and only set the VIR_DOMAIN_DEVICE_MODIFY_LIVE flag if a '--live' flag is given. Basically we need to try & ensure compatability with existing operation if at all possible Regards, Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

On Thu, Jan 14, 2010 at 10:42:45AM -0700, Jim Fehlig wrote:

Implementation of domain{Attach,Detach}DeviceFlags handlers in the drivers. --- src/esx/esx_driver.c | 2 + src/lxc/lxc_driver.c | 2 + src/opennebula/one_driver.c | 2 + src/openvz/openvz_driver.c | 2 + src/phyp/phyp_driver.c | 2 + src/qemu/qemu_driver.c | 26 +++++++++++ src/test/test_driver.c | 2 + src/uml/uml_driver.c | 2 + src/vbox/vbox_tmpl.c | 24 ++++++++++ src/xen/proxy_internal.c | 4 +- src/xen/xen_driver.c | 42 +++++++++++++++++-- src/xen/xen_driver.h | 4 +- src/xen/xen_hypervisor.c | 4 +- src/xen/xen_inotify.c | 4 +- src/xen/xend_internal.c | 98 +++++++++++++++++++++++++++++++++++-------- src/xen/xm_internal.c | 30 +++++++++---- src/xen/xs_internal.c | 4 +- 17 files changed, 212 insertions(+), 42 deletions(-)

diff --git a/src/esx/esx_driver.c b/src/esx/esx_driver.c index ddda66e..577c1c7 100644 --- a/src/esx/esx_driver.c +++ b/src/esx/esx_driver.c @@ -3443,7 +3443,9 @@ static virDriver esxDriver = { esxDomainDefineXML, /* domainDefineXML */ esxDomainUndefine, /* domainUndefine */ NULL, /* domainAttachDevice */ + NULL, /* domainAttachDeviceFlags */ NULL, /* domainDetachDevice */ + NULL, /* domainDetachDeviceFlags */ NULL, /* domainGetAutostart */ NULL, /* domainSetAutostart */ esxDomainGetSchedulerType, /* domainGetSchedulerType */ diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index 86606c7..d80f20c 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -2427,7 +2427,9 @@ static virDriver lxcDriver = { lxcDomainDefine, /* domainDefineXML */ lxcDomainUndefine, /* domainUndefine */ NULL, /* domainAttachDevice */ + NULL, /* domainAttachDeviceFlags */ NULL, /* domainDetachDevice */ + NULL, /* domainDetachDeviceFlags */ lxcDomainGetAutostart, /* domainGetAutostart */ lxcDomainSetAutostart, /* domainSetAutostart */ lxcGetSchedulerType, /* domainGetSchedulerType */ diff --git a/src/opennebula/one_driver.c b/src/opennebula/one_driver.c index ad7faca..509cbc3 100644 --- a/src/opennebula/one_driver.c +++ b/src/opennebula/one_driver.c @@ -754,7 +754,9 @@ static virDriver oneDriver = { oneDomainDefine, /* domainDefineXML */ oneDomainUndefine, /* domainUndefine */ NULL, /* domainAttachDevice */ + NULL, /* domainAttachDeviceFlags */ NULL, /* domainDetachDevice */ + NULL, /* domainDetachDeviceFlags */ oneGetAutostart, /* domainGetAutostart */ NULL, /* domainSetAutostart */ NULL, /* domainGetSchedulerType */ diff --git a/src/openvz/openvz_driver.c b/src/openvz/openvz_driver.c index 196fd8c..b16efef 100644 --- a/src/openvz/openvz_driver.c +++ b/src/openvz/openvz_driver.c @@ -1506,7 +1506,9 @@ static virDriver openvzDriver = { openvzDomainDefineXML, /* domainDefineXML */ openvzDomainUndefine, /* domainUndefine */ NULL, /* domainAttachDevice */ + NULL, /* domainAttachDeviceFlags */ NULL, /* domainDetachDevice */ + NULL, /* domainDetachDeviceFlags */ openvzDomainGetAutostart, /* domainGetAutostart */ openvzDomainSetAutostart, /* domainSetAutostart */ NULL, /* domainGetSchedulerType */ diff --git a/src/phyp/phyp_driver.c b/src/phyp/phyp_driver.c index bd5cfc7..5474bda 100644 --- a/src/phyp/phyp_driver.c +++ b/src/phyp/phyp_driver.c @@ -1622,7 +1622,9 @@ virDriver phypDriver = { NULL, /* domainDefineXML */ NULL, /* domainUndefine */ NULL, /* domainAttachDevice */ + NULL, /* domainAttachDeviceFlags */ NULL, /* domainDetachDevice */ + NULL, /* domainDetachDeviceFlags */ NULL, /* domainGetAutostart */ NULL, /* domainSetAutostart */ NULL, /* domainGetSchedulerType */ diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index deb8adc..02a6212 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5544,6 +5544,18 @@ cleanup: return ret; }

+static int qemudDomainAttachDeviceFlags(virDomainPtr dom, + const char *xml, + unsigned int flags) { + if (flags & VIR_DOMAIN_DEVICE_MODIFY_CONFIG) { + qemudReportError(dom->conn, dom, NULL, VIR_ERR_OPERATION_INVALID, + "%s", _("cannot modify the persistent configuration of a domain")); + return -1; + } + + return qemudDomainAttachDevice(dom, xml); +} + static int qemudDomainDetachPciDiskDevice(virConnectPtr conn, struct qemud_driver *driver, virDomainObjPtr vm, @@ -5872,6 +5884,18 @@ cleanup: return ret; }

+static int qemudDomainDetachDeviceFlags(virDomainPtr dom, + const char *xml, + unsigned int flags) { + if (flags & VIR_DOMAIN_DEVICE_MODIFY_CONFIG) { + qemudReportError(dom->conn, dom, NULL, VIR_ERR_OPERATION_INVALID, + "%s", _("cannot modify the persistent configuration of a domain")); + return -1; + } + + return qemudDomainDetachDevice(dom, xml); +} + static int qemudDomainGetAutostart(virDomainPtr dom, int *autostart) { struct qemud_driver *driver = dom->conn->privateData; @@ -7985,7 +8009,9 @@ static virDriver qemuDriver = { qemudDomainDefine, /* domainDefineXML */ qemudDomainUndefine, /* domainUndefine */ qemudDomainAttachDevice, /* domainAttachDevice */ + qemudDomainAttachDeviceFlags, /* domainAttachDeviceFlags */ qemudDomainDetachDevice, /* domainDetachDevice */ + qemudDomainDetachDeviceFlags, /* domainDetachDeviceFlags */ qemudDomainGetAutostart, /* domainGetAutostart */ qemudDomainSetAutostart, /* domainSetAutostart */ qemuGetSchedulerType, /* domainGetSchedulerType */ diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 2122a1b..1feca1c 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -5209,7 +5209,9 @@ static virDriver testDriver = { testDomainDefineXML, /* domainDefineXML */ testDomainUndefine, /* domainUndefine */ NULL, /* domainAttachDevice */ + NULL, /* domainAttachDeviceFlags */ NULL, /* domainDetachDevice */ + NULL, /* domainDetachDeviceFlags */ testDomainGetAutostart, /* domainGetAutostart */ testDomainSetAutostart, /* domainSetAutostart */ testDomainGetSchedulerType, /* domainGetSchedulerType */ diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c index b808090..6adaf9f 100644 --- a/src/uml/uml_driver.c +++ b/src/uml/uml_driver.c @@ -1895,7 +1895,9 @@ static virDriver umlDriver = { umlDomainDefine, /* domainDefineXML */ umlDomainUndefine, /* domainUndefine */ NULL, /* domainAttachDevice */ + NULL, /* domainAttachDeviceFlags */ NULL, /* domainDetachDevice */ + NULL, /* domainDetachDeviceFlags */ umlDomainGetAutostart, /* domainGetAutostart */ umlDomainSetAutostart, /* domainSetAutostart */ NULL, /* domainGetSchedulerType */ diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c index 07696c0..79915c2 100644 --- a/src/vbox/vbox_tmpl.c +++ b/src/vbox/vbox_tmpl.c @@ -4837,6 +4837,17 @@ cleanup: return ret; }

+static int vboxDomainAttachDeviceFlags(virDomainPtr dom, const char *xml, + unsigned int flags) { + if (flags & VIR_DOMAIN_DEVICE_MODIFY_CONFIG) { + vboxError(dom->conn, VIR_ERR_OPERATION_INVALID, "%s", + _("cannot modify the persistent configuration of a domain")); + return -1; + } + + return vboxDomainAttachDevice(dom, xml); +} + static int vboxDomainDetachDevice(virDomainPtr dom, const char *xml) { VBOX_OBJECT_CHECK(dom->conn, int, -1); IMachine *machine = NULL; @@ -4967,6 +4978,17 @@ cleanup: return ret; }

+static int vboxDomainDetachDeviceFlags(virDomainPtr dom, const char *xml, + unsigned int flags) { + if (flags & VIR_DOMAIN_DEVICE_MODIFY_CONFIG) { + vboxError(dom->conn, VIR_ERR_OPERATION_INVALID, "%s", + _("cannot modify the persistent configuration of a domain")); + return -1; + } + + return vboxDomainDetachDevice(dom, xml); +} + #if VBOX_API_VERSION == 2002 /* No Callback support for VirtualBox 2.2.* series */ #else /* !(VBOX_API_VERSION == 2002) */ @@ -7013,7 +7035,9 @@ virDriver NAME(Driver) = { vboxDomainDefineXML, /* domainDefineXML */ vboxDomainUndefine, /* domainUndefine */ vboxDomainAttachDevice, /* domainAttachDevice */ + vboxDomainAttachDeviceFlags, /* domainAttachDeviceFlags */ vboxDomainDetachDevice, /* domainDetachDevice */ + vboxDomainDetachDeviceFlags, /* domainDetachDeviceFlags */ NULL, /* domainGetAutostart */ NULL, /* domainSetAutostart */ NULL, /* domainGetSchedulerType */ diff --git a/src/xen/proxy_internal.c b/src/xen/proxy_internal.c index 73a0469..33f82c2 100644 --- a/src/xen/proxy_internal.c +++ b/src/xen/proxy_internal.c @@ -76,8 +76,8 @@ struct xenUnifiedDriver xenProxyDriver = { NULL, /* domainCreate */ NULL, /* domainDefineXML */ NULL, /* domainUndefine */ - NULL, /* domainAttachDevice */ - NULL, /* domainDetachDevice */ + NULL, /* domainAttachDeviceFlags */ + NULL, /* domainDetachDeviceFlags */ NULL, /* domainGetAutostart */ NULL, /* domainSetAutostart */ NULL, /* domainGetSchedulerType */ diff --git a/src/xen/xen_driver.c b/src/xen/xen_driver.c index 4911c9e..141e415 100644 --- a/src/xen/xen_driver.c +++ b/src/xen/xen_driver.c @@ -1426,10 +1426,26 @@ xenUnifiedDomainAttachDevice (virDomainPtr dom, const char *xml) { GET_PRIVATE(dom->conn); int i; + unsigned int flags = VIR_DOMAIN_DEVICE_MODIFY_LIVE;

for (i = 0; i < XEN_UNIFIED_NR_DRIVERS; ++i) - if (priv->opened[i] && drivers[i]->domainAttachDevice && - drivers[i]->domainAttachDevice (dom, xml) == 0) + if (priv->opened[i] && drivers[i]->domainAttachDeviceFlags && + drivers[i]->domainAttachDeviceFlags(dom, xml, flags) == 0) + return 0; + + return -1; +} + +static int +xenUnifiedDomainAttachDeviceFlags (virDomainPtr dom, const char *xml, + unsigned int flags) +{ + GET_PRIVATE(dom->conn); + int i; + + for (i = 0; i < XEN_UNIFIED_NR_DRIVERS; ++i) + if (priv->opened[i] && drivers[i]->domainAttachDeviceFlags && + drivers[i]->domainAttachDeviceFlags(dom, xml, flags) == 0) return 0;

return -1; @@ -1440,10 +1456,26 @@ xenUnifiedDomainDetachDevice (virDomainPtr dom, const char *xml) { GET_PRIVATE(dom->conn); int i; + unsigned int flags = VIR_DOMAIN_DEVICE_MODIFY_LIVE; + + for (i = 0; i < XEN_UNIFIED_NR_DRIVERS; ++i) + if (priv->opened[i] && drivers[i]->domainDetachDeviceFlags && + drivers[i]->domainDetachDeviceFlags(dom, xml, flags) == 0) + return 0; + + return -1; +} + +static int +xenUnifiedDomainDetachDeviceFlags (virDomainPtr dom, const char *xml, + unsigned int flags) +{ + GET_PRIVATE(dom->conn); + int i;

for (i = 0; i < XEN_UNIFIED_NR_DRIVERS; ++i) - if (priv->opened[i] && drivers[i]->domainDetachDevice && - drivers[i]->domainDetachDevice (dom, xml) == 0) + if (priv->opened[i] && drivers[i]->domainDetachDeviceFlags && + drivers[i]->domainDetachDeviceFlags(dom, xml, flags) == 0) return 0;

return -1; @@ -1833,7 +1865,9 @@ static virDriver xenUnifiedDriver = { xenUnifiedDomainDefineXML, /* domainDefineXML */ xenUnifiedDomainUndefine, /* domainUndefine */ xenUnifiedDomainAttachDevice, /* domainAttachDevice */ + xenUnifiedDomainAttachDeviceFlags, /* domainAttachDeviceFlags */ xenUnifiedDomainDetachDevice, /* domainDetachDevice */ + xenUnifiedDomainDetachDeviceFlags, /* domainDetachDeviceFlags */ xenUnifiedDomainGetAutostart, /* domainGetAutostart */ xenUnifiedDomainSetAutostart, /* domainSetAutostart */ xenUnifiedDomainGetSchedulerType, /* domainGetSchedulerType */ diff --git a/src/xen/xen_driver.h b/src/xen/xen_driver.h index 185eb2b..defe6e2 100644 --- a/src/xen/xen_driver.h +++ b/src/xen/xen_driver.h @@ -93,8 +93,8 @@ struct xenUnifiedDriver { virDrvDomainCreate domainCreate; virDrvDomainDefineXML domainDefineXML; virDrvDomainUndefine domainUndefine; - virDrvDomainAttachDevice domainAttachDevice; - virDrvDomainDetachDevice domainDetachDevice; + virDrvDomainAttachDeviceFlags domainAttachDeviceFlags; + virDrvDomainDetachDeviceFlags domainDetachDeviceFlags; virDrvDomainGetAutostart domainGetAutostart; virDrvDomainSetAutostart domainSetAutostart; virDrvDomainGetSchedulerType domainGetSchedulerType; diff --git a/src/xen/xen_hypervisor.c b/src/xen/xen_hypervisor.c index 6d8accc..baa626f 100644 --- a/src/xen/xen_hypervisor.c +++ b/src/xen/xen_hypervisor.c @@ -793,8 +793,8 @@ struct xenUnifiedDriver xenHypervisorDriver = { NULL, /* domainCreate */ NULL, /* domainDefineXML */ NULL, /* domainUndefine */ - NULL, /* domainAttachDevice */ - NULL, /* domainDetachDevice */ + NULL, /* domainAttachDeviceFlags */ + NULL, /* domainDetachDeviceFlags */ NULL, /* domainGetAutostart */ NULL, /* domainSetAutostart */ xenHypervisorGetSchedulerType, /* domainGetSchedulerType */ diff --git a/src/xen/xen_inotify.c b/src/xen/xen_inotify.c index d9dfbb7..e3845d8 100644 --- a/src/xen/xen_inotify.c +++ b/src/xen/xen_inotify.c @@ -79,8 +79,8 @@ struct xenUnifiedDriver xenInotifyDriver = { NULL, /* domainCreate */ NULL, /* domainDefineXML */ NULL, /* domainUndefine */ - NULL, /* domainAttachDevice */ - NULL, /* domainDetachDevice */ + NULL, /* domainAttachDeviceFlags */ + NULL, /* domainDetachDeviceFlags */ NULL, /* domainGetAutostart */ NULL, /* domainSetAutostart */ NULL, /* domainGetSchedulerType */ diff --git a/src/xen/xend_internal.c b/src/xen/xend_internal.c index d9bfa15..665bc47 100644 --- a/src/xen/xend_internal.c +++ b/src/xen/xend_internal.c @@ -4096,9 +4096,10 @@ xenDaemonCreateXML(virConnectPtr conn, const char *xmlDesc, }

/** - * xenDaemonAttachDevice: + * xenDaemonAttachDeviceFlags: * @domain: pointer to domain object * @xml: pointer to XML description of device + * @flags: an OR'ed set of virDomainDeviceModifyFlags * * Create a virtual device attachment to backend. * XML description is translated into S-expression. @@ -4106,7 +4107,8 @@ xenDaemonCreateXML(virConnectPtr conn, const char *xmlDesc, * Returns 0 in case of success, -1 in case of failure. */ static int -xenDaemonAttachDevice(virDomainPtr domain, const char *xml) +xenDaemonAttachDeviceFlags(virDomainPtr domain, const char *xml, + unsigned int flags) { xenUnifiedPrivatePtr priv; char *sexpr = NULL; @@ -4124,12 +4126,41 @@ xenDaemonAttachDevice(virDomainPtr domain, const char *xml)

priv = (xenUnifiedPrivatePtr) domain->conn->privateData;

- /* - * on older Xen without the inactive guests management - * avoid doing this on inactive guests - */ - if ((domain->id < 0) && (priv->xendConfigVersion < 3)) - return -1; + if (domain->id < 0) { + /* If xendConfigVersion < 3 only live config can be changed */ + if (priv->xendConfigVersion < 3) { + virXendError(domain->conn, VIR_ERR_OPERATION_INVALID, "%s", + _("Xend version does not support modifying " + "persisted config")); + return -1; + } + /* Cannot modify live config if domain is inactive */ + if (flags & VIR_DOMAIN_DEVICE_MODIFY_LIVE) { + virXendError(domain->conn, VIR_ERR_OPERATION_INVALID, "%s", + _("Cannot modify live config if domain is inactive")); + return -1; + } + } else { + /* Only live config can be changed if xendConfigVersion < 3 */ + if (priv->xendConfigVersion < 3 && + (flags != VIR_DOMAIN_DEVICE_MODIFY_CURRENT || + flags != VIR_DOMAIN_DEVICE_MODIFY_LIVE)) { + virXendError(domain->conn, VIR_ERR_OPERATION_INVALID, "%s", + _("Xend version does not support modifying " + "persisted config")); + return -1; + } + /* Xen only supports modifying both live and persisted config if + * xendConfigVersion >= 3 + */ + if (flags != (VIR_DOMAIN_DEVICE_MODIFY_LIVE | + VIR_DOMAIN_DEVICE_MODIFY_CONFIG)) { + virXendError(domain->conn, VIR_ERR_OPERATION_INVALID, "%s", + _("Xend only supports modifying both live and " + "persisted config")); + return -1; + } + }

if (!(def = xenDaemonDomainFetch(domain->conn, domain->id, @@ -4203,16 +4234,18 @@ cleanup: }

/** - * xenDaemonDetachDevice: + * xenDaemonDetachDeviceFlags: * @domain: pointer to domain object * @xml: pointer to XML description of device + * @flags: an OR'ed set of virDomainDeviceModifyFlags * * Destroy a virtual device attachment to backend. * * Returns 0 in case of success, -1 in case of failure. */ static int -xenDaemonDetachDevice(virDomainPtr domain, const char *xml) +xenDaemonDetachDeviceFlags(virDomainPtr domain, const char *xml, + unsigned int flags) { xenUnifiedPrivatePtr priv; char class[8], ref[80]; @@ -4230,12 +4263,41 @@ xenDaemonDetachDevice(virDomainPtr domain, const char *xml)

priv = (xenUnifiedPrivatePtr) domain->conn->privateData;

- /* - * on older Xen without the inactive guests management - * avoid doing this on inactive guests - */ - if ((domain->id < 0) && (priv->xendConfigVersion < 3)) - return -1; + if (domain->id < 0) { + /* If xendConfigVersion < 3 only live config can be changed */ + if (priv->xendConfigVersion < 3) { + virXendError(domain->conn, VIR_ERR_OPERATION_INVALID, "%s", + _("Xend version does not support modifying " + "persisted config")); + return -1; + } + /* Cannot modify live config if domain is inactive */ + if (flags & VIR_DOMAIN_DEVICE_MODIFY_LIVE) { + virXendError(domain->conn, VIR_ERR_OPERATION_INVALID, "%s", + _("Cannot modify live config if domain is inactive")); + return -1; + } + } else { + /* Only live config can be changed if xendConfigVersion < 3 */ + if (priv->xendConfigVersion < 3 && + (flags != VIR_DOMAIN_DEVICE_MODIFY_CURRENT || + flags != VIR_DOMAIN_DEVICE_MODIFY_LIVE)) { + virXendError(domain->conn, VIR_ERR_OPERATION_INVALID, "%s", + _("Xend version does not support modifying " + "persisted config")); + return -1; + } + /* Xen only supports modifying both live and persisted config if + * xendConfigVersion >= 3 + */ + if (flags != (VIR_DOMAIN_DEVICE_MODIFY_LIVE | + VIR_DOMAIN_DEVICE_MODIFY_CONFIG)) { + virXendError(domain->conn, VIR_ERR_OPERATION_INVALID, "%s", + _("Xend only supports modifying both live and " + "persisted config")); + return -1; + } + }

if (!(def = xenDaemonDomainFetch(domain->conn, domain->id, @@ -5165,8 +5227,8 @@ struct xenUnifiedDriver xenDaemonDriver = { xenDaemonDomainCreate, /* domainCreate */ xenDaemonDomainDefineXML, /* domainDefineXML */ xenDaemonDomainUndefine, /* domainUndefine */ - xenDaemonAttachDevice, /* domainAttachDevice */ - xenDaemonDetachDevice, /* domainDetachDevice */ + xenDaemonAttachDeviceFlags, /* domainAttachDeviceFlags */ + xenDaemonDetachDeviceFlags, /* domainDetachDeviceFlags */ xenDaemonDomainGetAutostart, /* domainGetAutostart */ xenDaemonDomainSetAutostart, /* domainSetAutostart */ xenDaemonGetSchedulerType, /* domainGetSchedulerType */ diff --git a/src/xen/xm_internal.c b/src/xen/xm_internal.c index 944d5d5..43efe08 100644 --- a/src/xen/xm_internal.c +++ b/src/xen/xm_internal.c @@ -65,8 +65,10 @@ static int xenXMConfigSetString(virConfPtr conf, const char *setting, const char *str); char * xenXMAutoAssignMac(void); -static int xenXMDomainAttachDevice(virDomainPtr domain, const char *xml); -static int xenXMDomainDetachDevice(virDomainPtr domain, const char *xml); +static int xenXMDomainAttachDeviceFlags(virDomainPtr domain, const char *xml, + unsigned int flags); +static int xenXMDomainDetachDeviceFlags(virDomainPtr domain, const char *xml, + unsigned int flags);

#define XM_REFRESH_INTERVAL 10

@@ -109,8 +111,8 @@ struct xenUnifiedDriver xenXMDriver = { xenXMDomainCreate, /* domainCreate */ xenXMDomainDefineXML, /* domainDefineXML */ xenXMDomainUndefine, /* domainUndefine */ - xenXMDomainAttachDevice, /* domainAttachDevice */ - xenXMDomainDetachDevice, /* domainDetachDevice */ + xenXMDomainAttachDeviceFlags, /* domainAttachDeviceFlags */ + xenXMDomainDetachDeviceFlags, /* domainDetachDeviceFlags */ NULL, /* domainGetAutostart */ NULL, /* domainSetAutostart */ NULL, /* domainGetSchedulerType */ @@ -2914,17 +2916,21 @@ cleanup:

/** - * xenXMDomainAttachDevice: + * xenXMDomainAttachDeviceFlags: * @domain: pointer to domain object * @xml: pointer to XML description of device + * @flags: an OR'ed set of virDomainDeviceModifyFlags * * Create a virtual device attachment to backend. * XML description is translated into config file. + * This driver only supports device allocation to + * persisted config. * * Returns 0 in case of success, -1 in case of failure. */ static int -xenXMDomainAttachDevice(virDomainPtr domain, const char *xml) { +xenXMDomainAttachDeviceFlags(virDomainPtr domain, const char *xml, + unsigned int flags) { const char *filename = NULL; xenXMConfCachePtr entry = NULL; int ret = -1; @@ -2940,7 +2946,7 @@ xenXMDomainAttachDevice(virDomainPtr domain, const char *xml) {

if (domain->conn->flags & VIR_CONNECT_RO) return -1; - if (domain->id != -1) + if (domain->id != -1 && !(flags & VIR_DOMAIN_DEVICE_MODIFY_CONFIG)) return -1;

priv = (xenUnifiedPrivatePtr) domain->conn->privateData; @@ -3002,16 +3008,20 @@ xenXMDomainAttachDevice(virDomainPtr domain, const char *xml) {

/** - * xenXMDomainDetachDevice: + * xenXMDomainDetachDeviceFlags: * @domain: pointer to domain object * @xml: pointer to XML description of device + * @flags: an OR'ed set of virDomainDeviceModifyFlags * * Destroy a virtual device attachment to backend. + * This driver only supports device deallocation from + * persisted config. * * Returns 0 in case of success, -1 in case of failure. */ static int -xenXMDomainDetachDevice(virDomainPtr domain, const char *xml) { +xenXMDomainDetachDeviceFlags(virDomainPtr domain, const char *xml, + unsigned int flags) { const char *filename = NULL; xenXMConfCachePtr entry = NULL; virDomainDeviceDefPtr dev = NULL; @@ -3029,7 +3039,7 @@ xenXMDomainDetachDevice(virDomainPtr domain, const char *xml) {

if (domain->conn->flags & VIR_CONNECT_RO) return -1; - if (domain->id != -1) + if (domain->id != -1 && !(flags & VIR_DOMAIN_DEVICE_MODIFY_CONFIG)) return -1;

priv = (xenUnifiedPrivatePtr) domain->conn->privateData; diff --git a/src/xen/xs_internal.c b/src/xen/xs_internal.c index 8a64d4e..1db0397 100644 --- a/src/xen/xs_internal.c +++ b/src/xen/xs_internal.c @@ -76,8 +76,8 @@ struct xenUnifiedDriver xenStoreDriver = { NULL, /* domainCreate */ NULL, /* domainDefineXML */ NULL, /* domainUndefine */ - NULL, /* domainAttachDevice */ - NULL, /* domainDetachDevice */ + NULL, /* domainAttachDeviceFlags */ + NULL, /* domainDetachDeviceFlags */ NULL, /* domainGetAutostart */ NULL, /* domainSetAutostart */ NULL, /* domainGetSchedulerType */ --

ACK, all looks good. Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

On Thu, Jan 14, 2010 at 10:42:43AM -0700, Jim Fehlig wrote:

Implementation of Domain{Attach,Detach}DeviceFlags in remote driver. --- src/remote/remote_driver.c | 54 ++++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 54 insertions(+), 0 deletions(-)

diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index d6f5fce..eb16f62 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -3057,6 +3057,32 @@ done: }

static int +remoteDomainAttachDeviceFlags (virDomainPtr domain, const char *xml, + unsigned int flags) +{ + int rv = -1; + remote_domain_attach_device_flags_args args; + struct private_data *priv = domain->conn->privateData; + + remoteDriverLock(priv); + + make_nonnull_domain (&args.dom, domain); + args.xml = (char *) xml; + args.flags = flags; + + if (call (domain->conn, priv, 0, REMOTE_PROC_DOMAIN_ATTACH_DEVICE_FLAGS, + (xdrproc_t) xdr_remote_domain_attach_device_flags_args, (char *) &args, + (xdrproc_t) xdr_void, (char *) NULL) == -1) + goto done; + + rv = 0; + +done: + remoteDriverUnlock(priv); + return rv; +} + +static int remoteDomainDetachDevice (virDomainPtr domain, const char *xml) { int rv = -1; @@ -3081,6 +3107,32 @@ done: }

static int +remoteDomainDetachDeviceFlags (virDomainPtr domain, const char *xml, + unsigned int flags) +{ + int rv = -1; + remote_domain_detach_device_flags_args args; + struct private_data *priv = domain->conn->privateData; + + remoteDriverLock(priv); + + make_nonnull_domain (&args.dom, domain); + args.xml = (char *) xml; + args.flags = flags; + + if (call (domain->conn, priv, 0, REMOTE_PROC_DOMAIN_DETACH_DEVICE_FLAGS, + (xdrproc_t) xdr_remote_domain_detach_device_flags_args, (char *) &args, + (xdrproc_t) xdr_void, (char *) NULL) == -1) + goto done; + + rv = 0; + +done: + remoteDriverUnlock(priv); + return rv; +} + +static int remoteDomainGetAutostart (virDomainPtr domain, int *autostart) { int rv = -1; @@ -8894,7 +8946,9 @@ static virDriver remote_driver = { remoteDomainDefineXML, /* domainDefineXML */ remoteDomainUndefine, /* domainUndefine */ remoteDomainAttachDevice, /* domainAttachDevice */ + remoteDomainAttachDeviceFlags, /* domainAttachDeviceFlags */ remoteDomainDetachDevice, /* domainDetachDevice */ + remoteDomainDetachDeviceFlags, /* domainDetachDeviceFlags */ remoteDomainGetAutostart, /* domainGetAutostart */ remoteDomainSetAutostart, /* domainSetAutostart */ remoteDomainGetSchedulerType, /* domainGetSchedulerType */

ACK Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

On Thu, Jan 14, 2010 at 10:42:41AM -0700, Jim Fehlig wrote:

Implementation of public API for virDomain{Attach,Detach}DeviceFlags. --- src/libvirt.c | 106 ++++++++++++++++++++++++++++++++++++++++++++++++++++---- 1 files changed, 98 insertions(+), 8 deletions(-)

diff --git a/src/libvirt.c b/src/libvirt.c index 1145561..77f76bc 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -5129,7 +5129,6 @@ error: int virDomainAttachDevice(virDomainPtr domain, const char *xml) { - virConnectPtr conn; DEBUG("domain=%p, xml=%s", domain, xml);

virResetLastError(); @@ -5147,17 +5146,63 @@ virDomainAttachDevice(virDomainPtr domain, const char *xml) virLibDomainError(domain, VIR_ERR_OPERATION_INVALID, __FUNCTION__); goto error; } + + return virDomainAttachDeviceFlags(domain, xml, + VIR_DOMAIN_DEVICE_MODIFY_LIVE); + +error: + virDispatchError(domain->conn); + return -1; +}

This looks safe, but there's a subtle problem with changing the existing virDomainAttachDevice() entry point to call virDomainAttachDeviceFlags(). It will break compatability with old libvirtd, even though the old libvirtd supports the virDomainAttachDevice() code. So we need to keep the distinct paths in the public API & driver definitions. The eventual low level hypervisor drivers can of course just turn their existing impl into a thin wrapper to the new method..

+ +/** + * virDomainAttachDeviceFlags: + * @domain: pointer to domain object + * @xml: pointer to XML description of one device + * @flags: an OR'ed set of virDomainDeviceModifyFlags + * + * Attach a virtual device to a domain, using the flags parameter + * to control how the device is attached. VIR_DOMAIN_DEVICE_MODIFY_CURRENT + * specifies that the device allocation is made based on current domain + * state. VIR_DOMAIN_DEVICE_MODIFY_LIVE specifies that the device shall be + * allocated to the active domain instance only and is not added to the + * persisted domain configuration. VIR_DOMAIN_DEVICE_MODIFY_CONFIG + * specifies that the device shall be allocated to the persisted domain + * configuration only. Note that the target hypervisor must return an + * error if unable to satisfy flags. E.g. the hypervisor driver will + * return failure if LIVE is specified but it only supports modifying the + * persisted device allocation. + * + * Returns 0 in case of success, -1 in case of failure. + */ +int +virDomainAttachDeviceFlags(virDomainPtr domain, + const char *xml, unsigned int flags) +{ + virConnectPtr conn; + DEBUG("domain=%p, xml=%s, flags=%d", domain, xml, flags); + + virResetLastError(); + + if (!VIR_IS_CONNECTED_DOMAIN(domain)) { + virLibDomainError(NULL, VIR_ERR_INVALID_DOMAIN, __FUNCTION__); + return (-1); + } + if (domain->conn->flags & VIR_CONNECT_RO) { + virLibDomainError(domain, VIR_ERR_OPERATION_DENIED, __FUNCTION__); + goto error; + } conn = domain->conn;

- if (conn->driver->domainAttachDevice) { + if (conn->driver->domainAttachDeviceFlags) { int ret; - ret = conn->driver->domainAttachDevice (domain, xml); + ret = conn->driver->domainAttachDeviceFlags(domain, xml, flags); if (ret < 0) goto error; return ret; }

- virLibConnError (conn, VIR_ERR_NO_SUPPORT, __FUNCTION__); + virLibConnError(conn, VIR_ERR_NO_SUPPORT, __FUNCTION__);

error: virDispatchError(domain->conn); @@ -5177,7 +5222,6 @@ error: int virDomainDetachDevice(virDomainPtr domain, const char *xml) { - virConnectPtr conn; DEBUG("domain=%p, xml=%s", domain, xml);

virResetLastError(); @@ -5195,17 +5239,63 @@ virDomainDetachDevice(virDomainPtr domain, const char *xml) virLibDomainError(domain, VIR_ERR_OPERATION_INVALID, __FUNCTION__); goto error; } + + return virDomainDetachDeviceFlags(domain, xml, + VIR_DOMAIN_DEVICE_MODIFY_LIVE); + +error: + virDispatchError(domain->conn); + return -1; +} + +/** + * virDomainDetachDeviceFlags: + * @domain: pointer to domain object + * @xml: pointer to XML description of one device + * @flags: an OR'ed set of virDomainDeviceModifyFlags + * + * Detach a virtual device from a domain, using the flags parameter + * to control how the device is detached. VIR_DOMAIN_DEVICE_MODIFY_CURRENT + * specifies that the device allocation is removed based on current domain + * state. VIR_DOMAIN_DEVICE_MODIFY_LIVE specifies that the device shall be + * deallocated from the active domain instance only and is not from the + * persisted domain configuration. VIR_DOMAIN_DEVICE_MODIFY_CONFIG + * specifies that the device shall be deallocated from the persisted domain + * configuration only. Note that the target hypervisor must return an + * error if unable to satisfy flags. E.g. the hypervisor driver will + * return failure if LIVE is specified but it only supports removing the + * persisted device allocation. + * + * Returns 0 in case of success, -1 in case of failure. + */ +int +virDomainDetachDeviceFlags(virDomainPtr domain, + const char *xml, unsigned int flags) +{ + virConnectPtr conn; + DEBUG("domain=%p, xml=%s, flags=%d", domain, xml, flags); + + virResetLastError(); + + if (!VIR_IS_CONNECTED_DOMAIN(domain)) { + virLibDomainError(NULL, VIR_ERR_INVALID_DOMAIN, __FUNCTION__); + return (-1); + } + if (domain->conn->flags & VIR_CONNECT_RO) { + virLibDomainError(domain, VIR_ERR_OPERATION_DENIED, __FUNCTION__); + goto error; + } conn = domain->conn;

- if (conn->driver->domainDetachDevice) { + if (conn->driver->domainDetachDeviceFlags) { int ret; - ret = conn->driver->domainDetachDevice (domain, xml); + ret = conn->driver->domainDetachDeviceFlags(domain, xml, flags); if (ret < 0) goto error; return ret; }

- virLibConnError (conn, VIR_ERR_NO_SUPPORT, __FUNCTION__); + virLibConnError(conn, VIR_ERR_NO_SUPPORT, __FUNCTION__);

error: virDispatchError(domain->conn); --

The same with the detach operation. Can you re-post this one such that it just adds the new methods, but leaves the current ones alone. Regards, Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

Daniel P. Berrange wrote:

On Tue, Jan 19, 2010 at 07:40:14PM +0000, Daniel P. Berrange wrote:

...

On Thu, Jan 14, 2010 at 10:42:41AM -0700, Jim Fehlig wrote:

...

Implementation of public API for virDomain{Attach,Detach}DeviceFlags. --- src/libvirt.c | 106 ++++++++++++++++++++++++++++++++++++++++++++++++++++---- 1 files changed, 98 insertions(+), 8 deletions(-)

diff --git a/src/libvirt.c b/src/libvirt.c index 1145561..77f76bc 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -5129,7 +5129,6 @@ error: int virDomainAttachDevice(virDomainPtr domain, const char *xml) { - virConnectPtr conn; DEBUG("domain=%p, xml=%s", domain, xml);

virResetLastError(); @@ -5147,17 +5146,63 @@ virDomainAttachDevice(virDomainPtr domain, const char *xml) virLibDomainError(domain, VIR_ERR_OPERATION_INVALID, __FUNCTION__); goto error; } + + return virDomainAttachDeviceFlags(domain, xml, + VIR_DOMAIN_DEVICE_MODIFY_LIVE); + +error: + virDispatchError(domain->conn); + return -1; +}

This looks safe, but there's a subtle problem with changing the existing virDomainAttachDevice() entry point to call virDomainAttachDeviceFlags(). It will break compatability with old libvirtd, even though the old libvirtd supports the virDomainAttachDevice() code.

Ah, yes - good catch. Thanks.

...

So we need to keep the distinct paths in the public API & driver definitions. The eventual low level hypervisor drivers can of course just turn their existing impl into a thin wrapper to the new method..

There's one other option actually - we could put compatability code in the remote driver client instead. Either, make it always invoke the old RPC call if flags == VIR_DOMAIN_DEVICE_MODIFY_LIVE, or have it invoke the new RPC call & fallback to the old RPC call if it gets an error indicating the new one doesn't exist.

Do you prefer the latter option? After a quick look, I didn't spot any existing compatibility code in the remote driver client. The first option might be slightly better wrt maintenance. Thanks, Jim

On Thu, Jan 14, 2010 at 10:42:40AM -0700, Jim Fehlig wrote:

Definition of internal API for virDomain{Attach,Detach}DeviceFlags. --- src/driver.h | 10 ++++++++++ 1 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/src/driver.h b/src/driver.h index c7e4fbf..08fe816 100644 --- a/src/driver.h +++ b/src/driver.h @@ -192,9 +192,17 @@ typedef int (*virDrvDomainAttachDevice) (virDomainPtr domain, const char *xml); typedef int + (*virDrvDomainAttachDeviceFlags) (virDomainPtr domain, + const char *xml, + unsigned int flags); +typedef int (*virDrvDomainDetachDevice) (virDomainPtr domain, const char *xml); typedef int + (*virDrvDomainDetachDeviceFlags) (virDomainPtr domain, + const char *xml, + unsigned int flags); +typedef int (*virDrvDomainGetAutostart) (virDomainPtr domain, int *autostart); typedef int @@ -419,7 +427,9 @@ struct _virDriver { virDrvDomainDefineXML domainDefineXML; virDrvDomainUndefine domainUndefine; virDrvDomainAttachDevice domainAttachDevice; + virDrvDomainAttachDeviceFlags domainAttachDeviceFlags; virDrvDomainDetachDevice domainDetachDevice; + virDrvDomainDetachDeviceFlags domainDetachDeviceFlags; virDrvDomainGetAutostart domainGetAutostart; virDrvDomainSetAutostart domainSetAutostart; virDrvDomainGetSchedulerType domainGetSchedulerType; --

ACK Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

On Thu, Jan 14, 2010 at 10:42:38AM -0700, Jim Fehlig wrote:

virDomain{Attach,Detach}Device is now only permitted on active domains. Explicitly state this restriction in the API documentation and enforce it in libvirt frontend. --- src/libvirt.c | 14 ++++++++++++-- 1 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/src/libvirt.c b/src/libvirt.c index d973907..1145561 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -5121,7 +5121,8 @@ error: * @domain: pointer to domain object * @xml: pointer to XML description of one device * - * Create a virtual device attachment to backend. + * Create a virtual device attachment to backend. This function, + * having hotplug semantics, is only allowed on an active domain. * * Returns 0 in case of success, -1 in case of failure. */ @@ -5142,6 +5143,10 @@ virDomainAttachDevice(virDomainPtr domain, const char *xml) virLibDomainError(domain, VIR_ERR_OPERATION_DENIED, __FUNCTION__); goto error; } + if (!virDomainIsActive(domain)) { + virLibDomainError(domain, VIR_ERR_OPERATION_INVALID, __FUNCTION__); + goto error; + } conn = domain->conn;

if (conn->driver->domainAttachDevice) { @@ -5164,7 +5169,8 @@ error: * @domain: pointer to domain object * @xml: pointer to XML description of one device * - * Destroy a virtual device attachment to backend. + * Destroy a virtual device attachment to backend. This function, + * having hot-unplug semantics, is only allowed on an active domain. * * Returns 0 in case of success, -1 in case of failure. */ @@ -5185,6 +5191,10 @@ virDomainDetachDevice(virDomainPtr domain, const char *xml) virLibDomainError(domain, VIR_ERR_OPERATION_DENIED, __FUNCTION__); goto error; } + if (!virDomainIsActive(domain)) { + virLibDomainError(domain, VIR_ERR_OPERATION_INVALID, __FUNCTION__); + goto error; + } conn = domain->conn;

if (conn->driver->domainDetachDevice) { --

Agreed with the added doc comments, but I think I prefer that we do the check for active in the drivers themselves, at time of use. Doing the check at the top level is open to race conditions, since no locks are held on the objects in question at this point. Drivers will thus have to do extra checks themselves, making this top level one redundant. So I think we should just add the docs here. Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

On Mon, Jan 25, 2010 at 04:16:59PM -0700, Jim Fehlig wrote:

Daniel P. Berrange wrote:

...

On Thu, Jan 14, 2010 at 10:42:38AM -0700, Jim Fehlig wrote:

...

virDomain{Attach,Detach}Device is now only permitted on active domains. Explicitly state this restriction in the API documentation and enforce it in libvirt frontend. --- src/libvirt.c | 14 ++++++++++++-- 1 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/src/libvirt.c b/src/libvirt.c index d973907..1145561 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -5121,7 +5121,8 @@ error: * @domain: pointer to domain object * @xml: pointer to XML description of one device * - * Create a virtual device attachment to backend. + * Create a virtual device attachment to backend. This function, + * having hotplug semantics, is only allowed on an active domain. * * Returns 0 in case of success, -1 in case of failure. */ @@ -5142,6 +5143,10 @@ virDomainAttachDevice(virDomainPtr domain, const char *xml) virLibDomainError(domain, VIR_ERR_OPERATION_DENIED, __FUNCTION__); goto error; } + if (!virDomainIsActive(domain)) { + virLibDomainError(domain, VIR_ERR_OPERATION_INVALID, __FUNCTION__); + goto error; + } conn = domain->conn;

if (conn->driver->domainAttachDevice) { @@ -5164,7 +5169,8 @@ error: * @domain: pointer to domain object * @xml: pointer to XML description of one device * - * Destroy a virtual device attachment to backend. + * Destroy a virtual device attachment to backend. This function, + * having hot-unplug semantics, is only allowed on an active domain. * * Returns 0 in case of success, -1 in case of failure. */ @@ -5185,6 +5191,10 @@ virDomainDetachDevice(virDomainPtr domain, const char *xml) virLibDomainError(domain, VIR_ERR_OPERATION_DENIED, __FUNCTION__); goto error; } + if (!virDomainIsActive(domain)) { + virLibDomainError(domain, VIR_ERR_OPERATION_INVALID, __FUNCTION__); + goto error; + } conn = domain->conn;

if (conn->driver->domainDetachDevice) { --

Agreed with the added doc comments, but I think I prefer that we do the check for active in the drivers themselves, at time of use. Doing the check at the top level is open to race conditions, since no locks are held on the objects in question at this point. Drivers will thus have to do extra checks themselves, making this top level one redundant. So I think we should just add the docs here.

Ok. Modified patch below.

Thanks, Jim

commit d8ec244c6513b7c44956a547e56c228a4c38fbbe Author: Jim Fehlig <jfehlig@novell.com> Date: Wed Jan 13 18:24:51 2010 -0700

doc: restrict virDomain{Attach,Detach}Device to active domains

virDomain{Attach,Detach}Device is now only permitted on active domains. Explicitly state this restriction in the API documentation.

V2: Only change doc, dropping the hunk that forced the restriction in libvirt frontend.

diff --git a/src/libvirt.c b/src/libvirt.c index d973907..188b991 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -5121,7 +5121,8 @@ error: * @domain: pointer to domain object * @xml: pointer to XML description of one device * - * Create a virtual device attachment to backend. + * Create a virtual device attachment to backend. This function, + * having hotplug semantics, is only allowed on an active domain. * * Returns 0 in case of success, -1 in case of failure. */ @@ -5164,7 +5165,8 @@ error: * @domain: pointer to domain object * @xml: pointer to XML description of one device * - * Destroy a virtual device attachment to backend. + * Destroy a virtual device attachment to backend. This function, + * having hot-unplug semantics, is only allowed on an active domain. * * Returns 0 in case of success, -1 in case of failure. */

ACK Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|