5:06 p.m.
This small patch series improves virDomain{Attach,Detach}Device
documentation and disallows these operation on inactive domains
in Xen driver as discussed here
http://www.redhat.com/archives/libvir-list/2009-November/msg00513.html
Jim Fehlig (2):
Improve virDomain{Attach,Detach}Device documentation
Disallow attach/detach device in Xen driver
src/libvirt.c | 6 ++++--
src/xen/xend_internal.c | 22 ++++++++++------------
2 files changed, 14 insertions(+), 14 deletions(-)
5:06 p.m.
New subject: [libvirt] [PATCH 1/2] Improve virDomain{Attach, Detach}Device documentation
virDomain{Attach,Detach}Device is only permitted on active
domains. Explicitly state this restriction in the API
documentation.
---
src/libvirt.c | 6 ++++--
1 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/src/libvirt.c b/src/libvirt.c
index 05e45f3..918dd4f 100644
--- a/src/libvirt.c
+++ b/src/libvirt.c
@@ -4919,7 +4919,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.
*/
@@ -4962,7 +4963,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.
*/
--
1.6.0.2
5:06 p.m.
New subject: [libvirt] [PATCH 2/2] Disallow attach/detach device in Xen driver
The API documentation now explicitly states that attaching
and detaching devices is only permitted on active domains.
This is already the case in the qemu driver, make it so in the
Xen driver.
---
src/xen/xend_internal.c | 22 ++++++++++------------
1 files changed, 10 insertions(+), 12 deletions(-)
diff --git a/src/xen/xend_internal.c b/src/xen/xend_internal.c
index d61e9e6..67d011e 100644
--- a/src/xen/xend_internal.c
+++ b/src/xen/xend_internal.c
@@ -4109,12 +4109,11 @@ 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) {
+ virXendError(domain->conn, VIR_ERR_OPERATION_INVALID,
+ "%s", _("cannot attach device on inactive
domain"));
+ return -1;
+ }
if (!(def = xenDaemonDomainFetch(domain->conn,
domain->id,
@@ -4213,12 +4212,11 @@ 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) {
+ virXendError(domain->conn, VIR_ERR_OPERATION_INVALID,
+ "%s", _("cannot detach device on inactive
domain"));
+ return -1;
+ }
if (!(def = xenDaemonDomainFetch(domain->conn,
domain->id,
--
1.6.0.2
9:40 a.m.
New subject: [libvirt] [PATCH 1/2] Improve virDomain{Attach, Detach}Device documentation
On Mon, Nov 16, 2009 at 04:06:41PM -0700, Jim Fehlig wrote:
virDomain{Attach,Detach}Device is only permitted on active
domains. Explicitly state this restriction in the API
documentation.
Well, actually I'm not sure it's true. For exemple the
XML xen driver has an implementation for inactive Xen domains,
and if I look at the VirtualBox driver it seems to take care
of domains which are not currently running (or paused).
Also if we were to implement that restriction it would be
better done in the libvirt.c front-end by asking the driver
if the domin is active and returning directly from there.
Daniel
--
Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/
daniel(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
12:39 p.m.
New subject: [libvirt] [PATCH 1/2] Improve virDomain{Attach, Detach}Device documentation
Daniel Veillard wrote:
On Mon, Nov 16, 2009 at 04:06:41PM -0700, Jim Fehlig wrote:
> virDomain{Attach,Detach}Device is only permitted on active
> domains. Explicitly state this restriction in the API
> documentation.
>
Well, actually I'm not sure it's true. For exemple the
XML xen driver has an implementation for inactive Xen domains,
and if I look at the VirtualBox driver it seems to take care
of domains which are not currently running (or paused).
So what do folks prefer? Allow the individual drivers to restrict
attach/detach device or enforce restriction in the front-end? IMO, it
should be delegated to the individual drivers, with a comment in the API
description that some hypervisors may not support this operation on
inactive domains. Why restrict a hypervisor's management functionality
in the libvirt front-end?
Also if we were to implement that restriction it would be
better done in the libvirt.c front-end by asking the driver
if the domin is active and returning directly from there.
Agreed. I had already thought about this after submitting the patch.
I'll rework the patch depending on consensus answer to above question.
Thanks,
Jim
2:42 p.m.
New subject: [libvirt] [PATCH 1/2] Improve virDomain{Attach, Detach}Device documentation
On 11/20/2009 01:39 PM, Jim Fehlig wrote:
Daniel Veillard wrote:
> On Mon, Nov 16, 2009 at 04:06:41PM -0700, Jim Fehlig wrote:
>
>> virDomain{Attach,Detach}Device is only permitted on active
>> domains. Explicitly state this restriction in the API
>> documentation.
>>
>
> Well, actually I'm not sure it's true. For exemple the
> XML xen driver has an implementation for inactive Xen domains,
> and if I look at the VirtualBox driver it seems to take care
> of domains which are not currently running (or paused).
>
So what do folks prefer? Allow the individual drivers to restrict
attach/detach device or enforce restriction in the front-end? IMO, it
should be delegated to the individual drivers, with a comment in the API
description that some hypervisors may not support this operation on
inactive domains. Why restrict a hypervisor's management functionality
in the libvirt front-end?
Because it introduces hypervisor dependent API differences that aren't
programmatically discoverable. This makes life difficult for apps that
want to support multiple hypervisors (like virt-manager, where we have
already bumped up against this inconsistency between xen and qemu
drivers). In this case, if xen allows offline device addition but qemu
does not, virt-manager will have to use the safe subset and never
attempt attach device for an offline VM, or hardcode HV differences into
the app.
For that reason, I think we should either always allow offline
AttachDevice or never allow it. And of the two, I highly favor the
latter, since the alternative would force every driver to reimplement
something that can easily be accomplished with DefineXML (unless the HV
supports it natively, which in Xen's case sounds like it isn't handled
consistently for all device types).
Maybe a way forward here is to clarify the virsh commands: we can alter
attach/detach-device to manually edit the VM xml for inactive VMs (but
also attempt hotplug if the VM is running), and introduce new commands
hotplug-device and hotunplug-device which map straight through to the
API. This way virsh won't regress, and indeed will improve: only
straight API users will see a change in behavior.
Thanks,
Cole
4:54 p.m.
New subject: [libvirt] [PATCH 1/2] Improve virDomain{Attach, Detach}Device documentation
Cole Robinson wrote:
On 11/20/2009 01:39 PM, Jim Fehlig wrote:
> Daniel Veillard wrote:
>
>> On Mon, Nov 16, 2009 at 04:06:41PM -0700, Jim Fehlig wrote:
>>
>>
>>> virDomain{Attach,Detach}Device is only permitted on active
>>> domains. Explicitly state this restriction in the API
>>> documentation.
>>>
>>>
>> Well, actually I'm not sure it's true. For exemple the
>> XML xen driver has an implementation for inactive Xen domains,
>> and if I look at the VirtualBox driver it seems to take care
>> of domains which are not currently running (or paused).
>>
>>
> So what do folks prefer? Allow the individual drivers to restrict
> attach/detach device or enforce restriction in the front-end? IMO, it
> should be delegated to the individual drivers, with a comment in the API
> description that some hypervisors may not support this operation on
> inactive domains. Why restrict a hypervisor's management functionality
> in the libvirt front-end?
>
>
Because it introduces hypervisor dependent API differences that aren't
programmatically discoverable.
They are discoverable if you consider receiving a "not supported" error
after making the call :-). But yes, it would be nice if apps knew these
capabilities up front to enable/disable widgets and such. Perhaps
libvirt will need a "hypervisor management service" capabilities element
some day.
This makes life difficult for apps that
want to support multiple hypervisors (like virt-manager, where we have
already bumped up against this inconsistency between xen and qemu
drivers). In this case, if xen allows offline device addition but qemu
does not, virt-manager will have to use the safe subset and never
attempt attach device for an offline VM, or hardcode HV differences into
the app.
Yep, understood.
For that reason, I think we should either always allow offline
AttachDevice or never allow it. And of the two, I highly favor the
latter, since the alternative would force every driver to reimplement
something that can easily be accomplished with DefineXML (unless the HV
supports it natively, which in Xen's case sounds like it isn't handled
consistently for all device types).
Maybe a way forward here is to clarify the virsh commands: we can
alter
attach/detach-device to manually edit the VM xml for inactive VMs (but
also attempt hotplug if the VM is running), and introduce new commands
hotplug-device and hotunplug-device which map straight through to the
API. This way virsh won't regress, and indeed will improve: only
straight API users will see a change in behavior.
Ok, IIUC you prefer to never allow offline {Attach,Detach}Device in the
API. Do others concur? If so I can spin a patch that
- Improves virDomain(Attach,Detach}Device documentation
- Disallows these operations in libvirt front-end as DV suggested
- Improves virsh app as Cole suggested
Thanks,
Jim
5516
days inactive
5520
days old
6 comments
3 participants
participants (3)
-
Cole Robinson -
Daniel Veillard -
Jim Fehlig