[Libvir] State of driver backend infrastructure
I'm looking to get more of the test driver backend working, and so decided it was time to audit the state of the driver backend infrastructure. There are a couple of areas which could do with some work: * Driver specific data. The virConnect & virDomain structs have explicit fields for Xen related data: virConnect: int handle; /* internal handle used for hypercall */ struct xs_handle *xshandle; /* handle to talk to the xenstore */ virDomain: int handle; /* internal handle for the domnain ID */ One idea for resolving this is to have an array of slots, one per drivers in which drivers can store data they need. void **handles Or perhaps, to allow simple int type too void union { int num; void *ptr } *handles * Connect URIs. The 'virDomainOpen' and 'virDomainOpenReadOnly' methods have an optional parameter 'name'. The Xen drivers currently ignore this, or expect it to be NULL / the string "Xen". For sake of back-compat we'll ned to preserve existing behaviour for NULL, but we should hook up URI parsing for the XenD backend to allow non local connections to work. This gives the question of what format should the URI be for Xen domains ? Although 'http://' is the protocol XenD uses, this isn't likely a good idea, because a later VMWare driver might also use a http based protocol. So, perhaps we should use generic URIs:? xend://somehost:port/ * Driver method hookup. The libvirt.c file has alot of methods which are either not hooked up to the driver backend, or hooked up, but still have duplicated (redundant?) Xen specific calls. Here is the complete status: virGetVersion: - Hardcodes support for Xen HV. No virConnectPtr handle, so how would we call out to driver backends ? virConnectOpen - OK virConnectOpenReadOnly - OK virConnectClose: - Does not call out to driver backends, simply free's struct virConnectGetType: - Does not call out to driver backends, hardcodes Xen HV virConnectGetVersion - Does not call out to driver backends, hardcodes Xen HV virConnectListDomains - Calls out to drivers, but also has duplicated code for XenD & XenStore virConnectNumOfDomains - Calls out to drivers, but also has duplicated code for XenD & XenStore virDomainCreateLinux - Does not call out to driver backends, hardcodes XenD virDomainLookupByID - Calls out to drivers, but also has duplicated code for XenD & XenStore virDomainLookupByUUID - Calls out to drivers, but also has duplicated code for XenD & XenStore virDomainLookupByUUIDString - OK (but delegates to virDomainLookupByUUID) virDomainLookupByName - Calls out to drivers, but also has duplicated code for XenD & XenStore virDomainDestroy - Does not call out to driver backends, hardcodes XenD & Xen HV virDomainFree - Does not call out to driver backends, simply free's struct virDomainSuspend - Does not call out to driver backends, hardcodes XenD & Xen HV virDomainResume - Does not call out to driver backends, hardcodes XenD & Xen HV virDomainSave - Does not call out to driver backends, hardcodes XenD virDomainRestore - Does not call out to driver backends, hardcodes XenD virDomainShutdown - Does not call out to driver backends, hardcodes XenD virDomainReboot - Does not call out to driver backends, hardcodes XenD virDomainGetName - OK virDomainGetUUID - Does not call out to driver backends, hardcodes XenD virDomainGetUUIDString - OK (but delegates to virDomainGetUUID) virDomainGetOSType - Does not call out to driver backends, calls function in XenStore driver virDomainGetMaxMemory - Does not call out to driver backends, hardcodes XenD, Xen HV, XenStore virDomainSetMaxMemory - OK virDomainSetMemory - OK virDomainGetInfo - Calls out to drivers, but also has duplicated code for XenD & XenStore virDomainGetXMLDesc - Does not call out to driver backends, hardcodes XenD virNodeGetInfo - OK virDomainDefineXML - OK virDomainUndefineXML - OK virDomainListDefinedDomains - No implemented virDomainCreate - No implemented My immediate needs for testing, are to hook up more of the methods which aren't connected to the driver backends. Regards, Dan. -- |=- Red Hat, Engineering, Emerging Technologies, Boston. +1 978 392 2496 -=| |=- Perl modules: http://search.cpan.org/~danberr/ -=| |=- Projects: http://freshmeat.net/~danielpb/ -=| |=- GnuPG: 7D3B9505 F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 -=|
On Mon, Jun 12, 2006 at 11:19:00PM +0100, Daniel P. Berrange wrote:
I'm looking to get more of the test driver backend working, and so decided it was time to audit the state of the driver backend infrastructure. There are a couple of areas which could do with some work:
* Driver specific data. The virConnect & virDomain structs have explicit fields for Xen related data:
virConnect:
int handle; /* internal handle used for hypercall */ struct xs_handle *xshandle; /* handle to talk to the xenstore */
virDomain:
int handle; /* internal handle for the domnain ID */
One idea for resolving this is to have an array of slots, one per drivers in which drivers can store data they need.
void **handles
Or perhaps, to allow simple int type too
void union { int num; void *ptr } *handles
Hum, I think I would worry when we have half a dozen back-ends, virConnect content is opaque, allocated/freed only by the library so changing that later won't generate any kind of API or ABI issues. Having explicit pointers at worse may loose a few bytes per connections, and on the other hand having the explicit types there ease debugging. So at this point I think there isn't really to worry about it.
* Connect URIs. The 'virDomainOpen' and 'virDomainOpenReadOnly' methods have an optional parameter 'name'. The Xen drivers currently ignore this, or expect it to be NULL / the string "Xen". For sake of back-compat we'll ned to preserve existing behaviour for NULL, but we should hook up URI parsing for the XenD backend to allow non local connections to work.
yes, that's an area which need refining.
This gives the question of what format should the URI be for Xen domains ? Although 'http://' is the protocol XenD uses, this isn't likely a good idea, because a later VMWare driver might also use a http based protocol. So, perhaps we should use generic URIs:?
xend://somehost:port/
yes I was thinking of somthing like this. The only annoying point is that xend isn't really a protocol i.e. it deviates a bit from URI real semantic and one could think of multiple xend access types (see Anthony's patch for Xend XML-RPC though ssh on xen-devel), but we could do things like xend:///var/run/xend/socket xendssh://user@somehost:port while being relatively clean w.r.t RFC 2396 and Co.
* Driver method hookup. The libvirt.c file has alot of methods which are either not hooked up to the driver backend, or hooked up, but still have duplicated (redundant?) Xen specific calls. Here is the complete status:
Okay, I need to clean them up, please bugzilla this I will fix this soonish.
virGetVersion:
- Hardcodes support for Xen HV. No virConnectPtr handle, so how would we call out to driver backends ?
yes that's an API bug :-\ , I'm afraid that mean I will have to make a release breaking compatibility, annoying, better do that as soon as possible.
virConnectClose:
- Does not call out to driver backends, simply free's struct
okay need to call the virDrvClose if non NULL.
virConnectGetType:
- Does not call out to driver backends, hardcodes Xen HV
Should return the value of virDrvGetType() or the string identifying the driver if NULL
virConnectGetVersion
- Does not call out to driver backends, hardcodes Xen HV
Should return value of virDrvGetVersion()
virConnectListDomains
- Calls out to drivers, but also has duplicated code for XenD & XenStore
Should remove the fallback, that should just work
virConnectNumOfDomains
- Calls out to drivers, but also has duplicated code for XenD & XenStore
same as previous
virDomainLookupByID
- Calls out to drivers, but also has duplicated code for XenD & XenStore
same as previous
virDomainLookupByUUID
- Calls out to drivers, but also has duplicated code for XenD & XenStore
same as previous
virDomainLookupByName
- Calls out to drivers, but also has duplicated code for XenD & XenStore
same as previous
virDomainCreateLinux
- Does not call out to driver backends, hardcodes XenD
that one is a bit complex because that's multistep, but yeah if needed the meat of the routine should just be moved in the xend driver.
virDomainLookupByUUIDString
- OK (but delegates to virDomainLookupByUUID)
virDomainDestroy
- Does not call out to driver backends, hardcodes XenD & Xen HV
bad need fixing to call the driver entry point.
virDomainFree
- Does not call out to driver backends, simply free's struct
same as previous
virDomainSuspend
- Does not call out to driver backends, hardcodes XenD & Xen HV
same as previous
virDomainResume
- Does not call out to driver backends, hardcodes XenD & Xen HV
same as previous
virDomainSave
- Does not call out to driver backends, hardcodes XenD
same as previous
virDomainRestore
- Does not call out to driver backends, hardcodes XenD
same as previous
virDomainShutdown
- Does not call out to driver backends, hardcodes XenD
same as previous
virDomainReboot
- Does not call out to driver backends, hardcodes XenD
same as previous
virDomainGetUUID
- Does not call out to driver backends, hardcodes XenD
virDomainGetName
- OK
virDomainGetUUIDString
- OK (but delegates to virDomainGetUUID)
virDomainGetOSType
- Does not call out to driver backends, calls function in XenStore driver
virDomainGetMaxMemory
- Does not call out to driver backends, hardcodes XenD, Xen HV, XenStore
virDomainGetXMLDesc
- Does not call out to driver backends, hardcodes XenD
Those 3 need to call the corresponding driver entries.
virDomainSetMaxMemory
- OK
virDomainSetMemory
- OK
virDomainGetInfo
- Calls out to drivers, but also has duplicated code for XenD & XenStore
fall back need to be removed
virNodeGetInfo
- OK
virDomainDefineXML
- OK
virDomainUndefineXML
- OK
virDomainListDefinedDomains
- No implemented
virDomainCreate
- No implemented
I need to implement the entry point but I'm waiting on Xend changes to support defined but not running domains.
My immediate needs for testing, are to hook up more of the methods which aren't connected to the driver backends.
Okay I need to clean this up, should not be that hard except the Version API change needed, :-\ maybe I can avoid this by using the version extraction from the first activated driver found. Thanks a lot for going though this ! Daniel -- Daniel Veillard | Red Hat http://redhat.com/ veillard@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
On Mon, Jun 12, 2006 at 06:52:57PM -0400, Daniel Veillard wrote:
On Mon, Jun 12, 2006 at 11:19:00PM +0100, Daniel P. Berrange wrote:
virGetVersion:
- Hardcodes support for Xen HV. No virConnectPtr handle, so how would we call out to driver backends ?
yes that's an API bug :-\ , I'm afraid that mean I will have to make a release breaking compatibility, annoying, better do that as soon as possible.
Actually no :-) no need to change the function signature I think: http://libvirt.org/html/libvirt-libvirt.html#virGetVersion Provides two information back, @libVer is the version of the library while @typeVer will be the version of the hypervisor type @type against which the library was compiled. If @type is NULL, "Xen" is assumed, if @type is unknown or not availble, an error code will be returned and @typeVer will be 0. this is a static compile time check. What we need is to provide a version info from the driver, if possible provided in the source, but that should not require a change of the User API. On the other hand adding a version field from struct _virDriver make sense, but it's not impacting user code at all, I will do that. Daniel -- Daniel Veillard | Red Hat http://redhat.com/ veillard@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
On Mon, Jun 12, 2006 at 06:52:57PM -0400, Daniel Veillard wrote:
On Mon, Jun 12, 2006 at 11:19:00PM +0100, Daniel P. Berrange wrote:
* Driver method hookup. The libvirt.c file has alot of methods which are either not hooked up to the driver backend, or hooked up, but still have duplicated (redundant?) Xen specific calls. Here is the complete status:
Okay, I need to clean them up, please bugzilla this I will fix this soonish.
virDomainDestroy
- Does not call out to driver backends, hardcodes XenD & Xen HV
bad need fixing to call the driver entry point.
virDomainSuspend
- Does not call out to driver backends, hardcodes XenD & Xen HV
same as previous
virDomainResume
- Does not call out to driver backends, hardcodes XenD & Xen HV
same as previous
virDomainShutdown
- Does not call out to driver backends, hardcodes XenD
same as previous
virDomainReboot
- Does not call out to driver backends, hardcodes XenD
same as previous
I'm attaching an initial patch which implements the driver backend calls for these five methods. I've tested that suspend & resume work, but my hardware died before I fully tested the other methods. I've got a couple questions before proceeding in any case: 1. In all of these methods I followed example from virDomainSetMemory and put in if (domain->conn->flags & VIR_CONNECT_RO) return (-1); This prevents these methods working with a 'read only' connection, however, this is a deviation from previous semantics. Even with a read only connection, XenD will let arbitrary unprivileged user shutdown/suspend/resume/etc a domain, so by putting this VIR_CONNECT_RO check in we'd be preventing an operation which used to work. However, IMHO this is a good thing, because it can be considered a *HUGE* security hole / denial of service attack, that unprivileged users can shutdown reboot, suspend, etc domains via XenD with no authentication. When this hole is eventually plugged, these methods would cease to work with a read only connection, so long term we'd end up in the same situation. Thus this patch could be considered a 'pre-emptive' solution. 2. The current implementation of shutdown & reboot methods calls the same code twice in a succession: /* * try first with the xend daemon */ ret = xenDaemonDomainShutdown(domain); if (ret == 0) { domain->flags |= DOMAIN_IS_SHUTDOWN; return (0); } /* * this is very hackish, the domU kernel probes for a special * node in the xenstore and launch the shutdown command if found. */ ret = xenDaemonDomainShutdown(domain); if (ret == 0) { domain->flags |= DOMAIN_IS_SHUTDOWN; } What was the reason to call xenDaemonDomainShutdown twice ? With my change to use the driver backends, it will only be called once. So I want to make sure I'm not missing an edge case here. Dan. -- |=- Red Hat, Engineering, Emerging Technologies, Boston. +1 978 392 2496 -=| |=- Perl modules: http://search.cpan.org/~danberr/ -=| |=- Projects: http://freshmeat.net/~danielpb/ -=| |=- GnuPG: 7D3B9505 F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 -=|
On Wed, Jun 14, 2006 at 01:44:36PM +0100, Daniel P. Berrange wrote:
On Mon, Jun 12, 2006 at 06:52:57PM -0400, Daniel Veillard wrote:
I'm attaching an initial patch which implements the driver backend calls for these five methods.
Okay,
I've tested that suspend & resume work, but my hardware died before I fully tested the other methods. I've got a couple questions before proceeding in any case:
1. In all of these methods I followed example from virDomainSetMemory and put in
if (domain->conn->flags & VIR_CONNECT_RO) return (-1);
This prevents these methods working with a 'read only' connection, however, this is a deviation from previous semantics. Even with a read only connection, XenD will let arbitrary unprivileged user shutdown/suspend/resume/etc a domain, so by putting this VIR_CONNECT_RO check in we'd be preventing an operation which used to work.
Hum, there is pros and cons. Pro is obviously cleaness and long term maintainance/expectations (this will have to be fixed). Cons is the fact it is allowed and putting the limitation in libvirt does not fix anything and we don't know yet what the final security policy will be... Also it blocks regression tests from running as an user and force to su before running 'make tests' which is a bit inconvenient...
However, IMHO this is a good thing, because it can be considered a *HUGE* security hole / denial of service attack, that unprivileged users can shutdown reboot, suspend, etc domains via XenD with no authentication. When this hole is eventually plugged, these methods would cease to work with a read only connection, so long term we'd end up in the same situation. Thus this patch could be considered a 'pre-emptive' solution.
pre-emptive but shooting a bit in the dark. I'm balanced, others may have an opinion there, please express yourselve :-)
2. The current implementation of shutdown & reboot methods calls the same code twice in a succession:
/* * try first with the xend daemon */ ret = xenDaemonDomainShutdown(domain); if (ret == 0) { domain->flags |= DOMAIN_IS_SHUTDOWN; return (0); }
/* * this is very hackish, the domU kernel probes for a special * node in the xenstore and launch the shutdown command if found. */ ret = xenDaemonDomainShutdown(domain); if (ret == 0) { domain->flags |= DOMAIN_IS_SHUTDOWN; }
What was the reason to call xenDaemonDomainShutdown twice ? With my
my guess is that's just an error due to a factoring remains from when the xenDaemonDomainShutdown() code was directly inlined in that routine.
change to use the driver backends, it will only be called once. So I want to make sure I'm not missing an edge case here.
I think that's fine :-)
Index: src/libvirt.c =================================================================== RCS file: /data/cvs/libvirt/src/libvirt.c,v retrieving revision 1.29 diff -u -r1.29 libvirt.c
I need to remember to not modify those 5 functions. thanks ! Daniel -- Daniel Veillard | Red Hat http://redhat.com/ veillard@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
On Wed, Jun 14, 2006 at 09:22:04AM -0400, Daniel Veillard wrote:
On Wed, Jun 14, 2006 at 01:44:36PM +0100, Daniel P. Berrange wrote:
1. In all of these methods I followed example from virDomainSetMemory and put in
if (domain->conn->flags & VIR_CONNECT_RO) return (-1);
This prevents these methods working with a 'read only' connection, however, this is a deviation from previous semantics. Even with a read only connection, XenD will let arbitrary unprivileged user shutdown/suspend/resume/etc a domain, so by putting this VIR_CONNECT_RO check in we'd be preventing an operation which used to work.
Hum, there is pros and cons. Pro is obviously cleaness and long term maintainance/expectations (this will have to be fixed). Cons is the fact it is allowed and putting the limitation in libvirt does not fix anything and we don't know yet what the final security policy will be... Also it blocks regression tests from running as an user and force to su before running 'make tests' which is a bit inconvenient...
When I commit this I'll wrap the VIR_CONNECT_RO flag test in a '#ifdef PEDANTIC' conditional. So the default semantics of these methods will be unchanged for now, unless you explicitly add -DPEDANDIC to the compiler flags. We can re-visit it at a later day whe XenD gets a sensible security / authentication model.
What was the reason to call xenDaemonDomainShutdown twice ? With my
my guess is that's just an error due to a factoring remains from when the xenDaemonDomainShutdown() code was directly inlined in that routine.
Ok, so I've commited the patch and not worried about the duplicated calls. Dan. -- |=- Red Hat, Engineering, Emerging Technologies, Boston. +1 978 392 2496 -=| |=- Perl modules: http://search.cpan.org/~danberr/ -=| |=- Projects: http://freshmeat.net/~danielpb/ -=| |=- GnuPG: 7D3B9505 F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 -=|
participants (3)
-
Daniel P. Berrange -
Daniel Veillard -
Karel Zak