[Libvir] [PATCH 0/6] Inactive domain support
The following series of patches is an update of my previous patches for supporting inactive domains in libvirt. A rough highlight of changes since last time: - Can override /etc/xen by setting LIBVIRT_XM_CONFIG_DIR env variable for people who move it to non-standard location - Several memory corruption bugs in config file refresh were solved - Fixed booting with paravirt framebuffer + grub - Added support for new APIs to python binding - Removed logic for supressing 404 errors (already merged earlier) - More complete device handling support (now creates network devices) Previous posting was here: http://www.redhat.com/archives/libvir-list/2006-September/msg00003.html 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 -=|
The attached patch adds two new APIs to the virConf object needed for creating new inactive domains: virConfNew virConfSetValue Docs for these methods are inline with the patch. The semantics of the virConfSetValue method are changed based on previous feedback - the 'value' object passed in is now always owned by the vifConf object upon return - even upon failure. This actually simplified the calling code quite alot too. 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, Nov 15, 2006 at 02:15:07AM +0000, Daniel P. Berrange wrote:
The attached patch adds two new APIs to the virConf object needed for creating new inactive domains:
virConfNew virConfSetValue
Docs for these methods are inline with the patch. The semantics of the virConfSetValue method are changed based on previous feedback - the 'value' object passed in is now always owned by the vifConf object upon return - even upon failure. This actually simplified the calling code quite alot too.
I think I reviewed this already in September, I could not spot anything wrong, this is infrastructure and APIs are good so please push it :-) Daniel -- Red Hat Virtualization group http://redhat.com/virtualization/ Daniel Veillard | virtualization library http://libvirt.org/ veillard@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
On Wed, Nov 15, 2006 at 04:32:21AM -0500, Daniel Veillard wrote:
On Wed, Nov 15, 2006 at 02:15:07AM +0000, Daniel P. Berrange wrote:
The attached patch adds two new APIs to the virConf object needed for creating new inactive domains:
virConfNew virConfSetValue
Docs for these methods are inline with the patch. The semantics of the virConfSetValue method are changed based on previous feedback - the 'value' object passed in is now always owned by the vifConf object upon return - even upon failure. This actually simplified the calling code quite alot too.
I think I reviewed this already in September, I could not spot anything wrong, this is infrastructure and APIs are good so please push it :-)
Now committed. 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 -=|
The attached patch adds a couple of new APIs to the hash table object to allow various different ways of iterating over the contents of the hash table. The methods are: virHashForEach virHashRemoveSet virHashSearch Docs for these methods are all inline. Compared to previous patch a logic flaw in the virHashRemoveSet method was fixed prevently some severe memory corruption! 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 Wed, Nov 15, 2006 at 02:18:48AM +0000, Daniel P. Berrange wrote:
The attached patch adds a couple of new APIs to the hash table object to allow various different ways of iterating over the contents of the hash table. The methods are:
virHashForEach virHashRemoveSet virHashSearch
Docs for these methods are all inline. Compared to previous patch a logic flaw in the virHashRemoveSet method was fixed prevently some severe memory corruption!
The APIs are okay, I'm just wondering if the iterator should not return an int allowing to break the iteration, but admitedly that would make it close to the search. So it's fine as-is.
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 -=|
Index: src/hash.c =================================================================== RCS file: /data/cvs/libvirt/src/hash.c,v retrieving revision 1.10 diff -c -r1.10 hash.c *** src/hash.c 21 Sep 2006 15:24:37 -0000 1.10 --- src/hash.c 15 Nov 2006 02:34:00 -0000 *************** *** 473,478 **** --- 473,597 ---- } }
+ + /** + * virHashForEach + * @table: the hash table to process + * @iter: callback to process each element + * @data: opaque data to pass to the iterator + * + * Iterates over every element in the hash table, invoking the + * 'iter' callback. The callback must not call any other virHash* + * functions, and in particular must not attempt to remove the + * element. + * + * Returns 0 on completion, -1 on failure + */
It could be useful and cheap to return the number of item traversed if there was no failure.
+ int virHashForEach(virHashTablePtr table, virHashIterator iter, const void *data) { + int i; + + if (table == NULL || iter == NULL) + return (-1); + + for (i = 0 ; i < table->size ; i++) { + virHashEntryPtr entry = table->table + i; + while (entry) { + if (entry->valid) { + iter(entry->payload, entry->name, data); + } + entry = entry->next; + } + } + return 0; + } + + /** + * virHashRemoveSet + * @table: the hash table to process + * @iter: callback to identify elements for removal + * @f: callback to free memory from element payload + * @data: opaque data to pass to the iterator + * + * Iterates over all elements in the hash table, invoking the 'iter' + * callback. If the callback returns a non-zero value, the element + * will be removed from the hash table & its payload passed to the + * callback 'f' for de-allocation. + * + * Returns 0 on success, -1 on failure + */
Same thing, we could return the number of item deallocated, callbacks make hard to keep the count.
+ int virHashRemoveSet(virHashTablePtr table, virHashSearcher iter, virHashDeallocator f, const void *data) { + int i; + + if (table == NULL || iter == NULL) + return (-1); + + for (i = 0 ; i < table->size ; i++) { + virHashEntryPtr prev = NULL; + virHashEntryPtr entry = &(table->table[i]); + + while (entry && entry->valid) { + if (iter(entry->payload, entry->name, data)) { + f(entry->payload, entry->name); + if (entry->name) + free(entry->name); + if (prev) { + prev->next = entry->next; + free(entry); + } else { + if (entry->next == NULL) { + entry->valid = 0; + entry->name = NULL; + } else { + entry = entry->next; + memcpy(&(table->table[i]), entry, + sizeof(virHashEntry)); + free(entry); + entry = NULL; + } + } + table->nbElems--; + } + prev = entry; + if (entry) { + entry = entry->next; + } else { + entry = NULL; + } + } + } + return 0; + }
Iterating when removing entries which are first in the list is a bit tricky but that's looks fine.
+ /** + * virHashSearch: + * @table: the hash table to search + * @iter: an iterator to identify the desired element + * @data: extra opaque information passed to the iter + * + * Iterates over the hash table calling the 'iter' callback + * for each element. The first element for which the iter + * returns non-zero will be returned by this function. + * The elements are processed in a undefined order + */ + void *virHashSearch(virHashTablePtr table, virHashSearcher iter, const void *data) { + int i; + + if (table == NULL || iter == NULL) + return (NULL); + + for (i = 0 ; i < table->size ; i++) { + virHashEntryPtr entry = table->table + i; + while (entry) { + if (entry->valid) { + if (iter(entry->payload, entry->name, data)) + return entry->payload; + } + entry = entry->next; + } + } + return (NULL); + }
Minor stylistic issue I tend to use return(value); and you use sometime return (value); or return value; but it's really not a serious problem :-)
/************************************************************************ * * * Domain and Connections allocations * *************** *** 770,772 **** --- 889,899 ---- xmlMutexUnlock(conn->domains_mux); return(ret); } + /* + * Local variables: + * indent-tabs-mode: nil + * c-indent-level: 4 + * c-basic-offset: 4 + * tab-width: 4 + * End: + */ Index: src/hash.h =================================================================== RCS file: /data/cvs/libvirt/src/hash.h,v retrieving revision 1.5 diff -c -r1.5 hash.h *** src/hash.h 9 Apr 2006 13:11:22 -0000 1.5 --- src/hash.h 15 Nov 2006 02:34:00 -0000 *************** *** 33,39 **** * * Callback to free data from a hash. */ ! typedef void (*virHashDeallocator) (void *payload, char *name);
/* * Constructor and destructor. --- 33,41 ---- * * Callback to free data from a hash.
maybe update the comment
*/ ! typedef void (*virHashDeallocator) (void *payload, const char *name); ! typedef void (*virHashIterator) (const void *payload, const char *name, const void *data); ! typedef int (*virHashSearcher) (const void *payload, const char *name, const void *data);
/* * Constructor and destructor. *************** *** 62,67 **** --- 64,75 ---- */ void *virHashLookup(virHashTablePtr table, const char *name);
/* * Iterators */
+ + + int virHashForEach(virHashTablePtr table, virHashIterator iter, const void *data); + int virHashRemoveSet(virHashTablePtr table, virHashSearcher iter, virHashDeallocator f, const void *data); + void *virHashSearch(virHashTablePtr table, virHashSearcher iter, const void *data); + #ifdef __cplusplus } #endif
All minor comments, please commit once reviewed :-) Daniel -- Red Hat Virtualization group http://redhat.com/virtualization/ Daniel Veillard | virtualization library http://libvirt.org/ veillard@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
On Wed, Nov 15, 2006 at 04:50:55AM -0500, Daniel Veillard wrote:
On Wed, Nov 15, 2006 at 02:18:48AM +0000, Daniel P. Berrange wrote:
The attached patch adds a couple of new APIs to the hash table object to allow various different ways of iterating over the contents of the hash table. The methods are:
virHashForEach virHashRemoveSet virHashSearch
Docs for these methods are all inline. Compared to previous patch a logic flaw in the virHashRemoveSet method was fixed prevently some severe memory corruption!
The APIs are okay, I'm just wondering if the iterator should not return an int allowing to break the iteration, but admitedly that would make it close to the search. So it's fine as-is.
Yes, that's a good idea - I'll make it return number of elements - I think I could acutaly make use of that elsewhere already.
+ int virHashRemoveSet(virHashTablePtr table, virHashSearcher iter, virHashDeallocator f, const void *data) { + int i; + + }
Iterating when removing entries which are first in the list is a bit tricky but that's looks fine.
Yeah, that's why i wrote a dedicated method for iterating & removing in one go - calling 'virHashRemove' from the normal iterator just caused very bad things to happen :-)
+ void *virHashSearch(virHashTablePtr table, virHashSearcher iter, const void *data) { + int i; + + if (table == NULL || iter == NULL) + return (NULL); + + for (i = 0 ; i < table->size ; i++) { + virHashEntryPtr entry = table->table + i; + while (entry) { + if (entry->valid) { + if (iter(entry->payload, entry->name, data)) + return entry->payload; + } + entry = entry->next; + } + } + return (NULL); + }
Minor stylistic issue I tend to use return(value); and you use sometime return (value); or return value; but it's really not a serious problem :-)
My own style is normally 'return value', but I try to match your style when working on libvirt - sometimes i forget though, so I'll fix up the bits where I forget the (). 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 Wed, Nov 15, 2006 at 04:25:55PM +0000, Daniel P. Berrange wrote:
On Wed, Nov 15, 2006 at 04:50:55AM -0500, Daniel Veillard wrote:
On Wed, Nov 15, 2006 at 02:18:48AM +0000, Daniel P. Berrange wrote:
The attached patch adds a couple of new APIs to the hash table object to allow various different ways of iterating over the contents of the hash table. The methods are:
virHashForEach virHashRemoveSet virHashSearch
Docs for these methods are all inline. Compared to previous patch a logic flaw in the virHashRemoveSet method was fixed prevently some severe memory corruption!
The APIs are okay, I'm just wondering if the iterator should not return an int allowing to break the iteration, but admitedly that would make it close to the search. So it's fine as-is.
Yes, that's a good idea - I'll make it return number of elements - I think I could acutaly make use of that elsewhere already.
+ int virHashRemoveSet(virHashTablePtr table, virHashSearcher iter, virHashDeallocator f, const void *data) { + int i; + + }
Iterating when removing entries which are first in the list is a bit tricky but that's looks fine.
Yeah, that's why i wrote a dedicated method for iterating & removing in one go - calling 'virHashRemove' from the normal iterator just caused very bad things to happen :-)
Compute next first, then process to test, usually that works :-) Daniel -- Red Hat Virtualization group http://redhat.com/virtualization/ Daniel Veillard | virtualization library http://libvirt.org/ veillard@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
On Wed, Nov 15, 2006 at 12:39:01PM -0500, Daniel Veillard wrote:
On Wed, Nov 15, 2006 at 04:25:55PM +0000, Daniel P. Berrange wrote:
On Wed, Nov 15, 2006 at 04:50:55AM -0500, Daniel Veillard wrote:
On Wed, Nov 15, 2006 at 02:18:48AM +0000, Daniel P. Berrange wrote:
The attached patch adds a couple of new APIs to the hash table object to allow various different ways of iterating over the contents of the hash table. The methods are:
virHashForEach virHashRemoveSet virHashSearch
Docs for these methods are all inline. Compared to previous patch a logic flaw in the virHashRemoveSet method was fixed prevently some severe memory corruption!
The APIs are okay, I'm just wondering if the iterator should not return an int allowing to break the iteration, but admitedly that would make it close to the search. So it's fine as-is.
Yes, that's a good idea - I'll make it return number of elements - I think I could acutaly make use of that elsewhere already.
+ int virHashRemoveSet(virHashTablePtr table, virHashSearcher iter, virHashDeallocator f, const void *data) { + int i; + + }
Iterating when removing entries which are first in the list is a bit tricky but that's looks fine.
Yeah, that's why i wrote a dedicated method for iterating & removing in one go - calling 'virHashRemove' from the normal iterator just caused very bad things to happen :-)
Compute next first, then process to test, usually that works :-)
Comitted, with the addition of the logic to return the number of elements iterated over. 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 -=|
The attached patch implements the virListDefinedDomains method in the python bindings, since it wasn't being automatically generated by the generator.py script. 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 Wed, Nov 15, 2006 at 02:20:33AM +0000, Daniel P. Berrange wrote:
The attached patch implements the virListDefinedDomains method in the python bindings, since it wasn't being automatically generated by the generator.py script.
+ c_retval = virConnectNumOfDefinedDomains(conn); + if (c_retval < 0) { + Py_INCREF(Py_None); + return (Py_None); + } + + if (c_retval) { + names = malloc(sizeof(char *) * c_retval);
Miss NULL return check here I think.
+ c_retval = virConnectListDefinedDomains(conn, names, c_retval); + if (c_retval < 0) { + free(names); + Py_INCREF(Py_None); + return(Py_None); + } + } + py_retval = PyList_New(c_retval);
the paranoid in me would feel safer if the for loop was protected by a test for names not being NULL even if I agree it's not strictly needed :-)
+ for (i = 0;i < c_retval;i++) { + PyList_SetItem(py_retval, i, libvirt_constcharPtrWrap(names[i])); + free(names[i]); + } + + if (names) + free(names); + + return(py_retval); + }
Please commit after adding the malloc return check :-) Daniel -- Red Hat Virtualization group http://redhat.com/virtualization/ Daniel Veillard | virtualization library http://libvirt.org/ veillard@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
On Wed, Nov 15, 2006 at 04:57:23AM -0500, Daniel Veillard wrote:
On Wed, Nov 15, 2006 at 02:20:33AM +0000, Daniel P. Berrange wrote:
The attached patch implements the virListDefinedDomains method in the python bindings, since it wasn't being automatically generated by the generator.py script.
+ c_retval = virConnectNumOfDefinedDomains(conn); + if (c_retval < 0) { + Py_INCREF(Py_None); + return (Py_None); + } + + if (c_retval) { + names = malloc(sizeof(char *) * c_retval);
Miss NULL return check here I think.
Good catch!
+ c_retval = virConnectListDefinedDomains(conn, names, c_retval); + if (c_retval < 0) { + free(names); + Py_INCREF(Py_None); + return(Py_None); + } + } + py_retval = PyList_New(c_retval);
the paranoid in me would feel safer if the for loop was protected by a test for names not being NULL even if I agree it's not strictly needed :-)
+ for (i = 0;i < c_retval;i++) { + PyList_SetItem(py_retval, i, libvirt_constcharPtrWrap(names[i])); + free(names[i]); + }
Yeah, worth doing the extra check - never know when someone might do a 'trivial' code rearrangement & break my current asusmption. 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 -=|
The attached path makes the xen_internal, xend_internall, xs_internal and proxy driver backends ignore all virDomainPtr objects which have an ID number of -1. All operations on such domains need to instead be handled by the new xm_internal.c backend which uses the config files. Without this patch, the existing drivers would all throw bogus errors due to ID of -1. This patch is slightly simpler than last time, because the code to ignore HTTP 404 errors from XenD was already merged last week. 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 Wed, Nov 15, 2006 at 02:23:23AM +0000, Daniel P. Berrange wrote:
The attached path makes the xen_internal, xend_internall, xs_internal and proxy driver backends ignore all virDomainPtr objects which have an ID number of -1. All operations on such domains need to instead be handled by the new xm_internal.c backend which uses the config files. Without this patch, the existing drivers would all throw bogus errors due to ID of -1.
This patch is slightly simpler than last time, because the code to ignore HTTP 404 errors from XenD was already merged last week.
[...]
*************** *** 716,730 **** int ret;
if (!VIR_IS_CONNECTED_DOMAIN(domain)) { ! if (domain == NULL) ! virProxyError(NULL, VIR_ERR_INVALID_DOMAIN, __FUNCTION__); ! else ! virProxyError(domain->conn, VIR_ERR_INVALID_DOMAIN, __FUNCTION__); ! return (0); } if (info == NULL) { virProxyError(domain->conn, VIR_ERR_INVALID_ARG, __FUNCTION__); ! return (-1); } memset(&req, 0, sizeof(req)); req.command = VIR_PROXY_DOMAIN_INFO; --- 718,734 ---- int ret;
if (!VIR_IS_CONNECTED_DOMAIN(domain)) { ! if (domain == NULL) ! virProxyError(NULL, VIR_ERR_INVALID_DOMAIN, __FUNCTION__); ! else ! virProxyError(domain->conn, VIR_ERR_INVALID_DOMAIN, __FUNCTION__); ! return (-1);
Agreed for the Proxy driver you now want to return -1 on Info if it's not a connected domain. We already emitted an error there anyway make sense, but I wonder why it used to return 0 there ... hum....
--- src/xend_internal.c 15 Nov 2006 02:34:04 -0000 *************** *** 1266,1272 **** return node; }
! static int xend_get_config_version(virConnectPtr conn) { struct sexpr *root; const char *value; --- 1266,1272 ---- return node; }
! int xend_get_config_version(virConnectPtr conn) { struct sexpr *root; const char *value;
I assume exporting it is needed for other patches. Just check :-)
*** src/xend_internal.h 12 Sep 2006 01:16:22 -0000 1.26 --- src/xend_internal.h 15 Nov 2006 02:34:05 -0000 *************** *** 613,618 **** --- 613,619 ---- */ int xend_log(virConnectPtr xend, char *buffer, size_t n_buffer);
+ int xend_get_config_version(virConnectPtr conn); char *xend_parse_domain_sexp(virConnectPtr conn, char *root, int xendConfigVersion);
Okidoc, long patch but repetitive changes. The only mistake possible is to forgot some entry points or code paths. There is a few reformatting changes too, but all fine :-) Daniel -- Red Hat Virtualization group http://redhat.com/virtualization/ Daniel Veillard | virtualization library http://libvirt.org/ veillard@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
On Wed, Nov 15, 2006 at 05:12:49AM -0500, Daniel Veillard wrote:
On Wed, Nov 15, 2006 at 02:23:23AM +0000, Daniel P. Berrange wrote:
*************** *** 716,730 **** int ret;
if (!VIR_IS_CONNECTED_DOMAIN(domain)) { ! if (domain == NULL) ! virProxyError(NULL, VIR_ERR_INVALID_DOMAIN, __FUNCTION__); ! else ! virProxyError(domain->conn, VIR_ERR_INVALID_DOMAIN, __FUNCTION__); ! return (0); } if (info == NULL) { virProxyError(domain->conn, VIR_ERR_INVALID_ARG, __FUNCTION__); ! return (-1); } memset(&req, 0, sizeof(req)); req.command = VIR_PROXY_DOMAIN_INFO; --- 718,734 ---- int ret;
if (!VIR_IS_CONNECTED_DOMAIN(domain)) { ! if (domain == NULL) ! virProxyError(NULL, VIR_ERR_INVALID_DOMAIN, __FUNCTION__); ! else ! virProxyError(domain->conn, VIR_ERR_INVALID_DOMAIN, __FUNCTION__); ! return (-1);
Agreed for the Proxy driver you now want to return -1 on Info if it's not a connected domain. We already emitted an error there anyway make sense, but I wonder why it used to return 0 there ... hum....
Just looks like a typo to me.
--- src/xend_internal.c 15 Nov 2006 02:34:04 -0000 *************** *** 1266,1272 **** return node; }
! static int xend_get_config_version(virConnectPtr conn) { struct sexpr *root; const char *value; --- 1266,1272 ---- return node; }
! int xend_get_config_version(virConnectPtr conn) { struct sexpr *root; const char *value;
I assume exporting it is needed for other patches. Just check :-)
Yeah, need to use it in xm_internal when generating the SEXPR to feed to Xend to start an inactive domain. 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 Wed, Nov 15, 2006 at 04:32:40PM +0000, Daniel P. Berrange wrote:
On Wed, Nov 15, 2006 at 05:12:49AM -0500, Daniel Veillard wrote:
On Wed, Nov 15, 2006 at 02:23:23AM +0000, Daniel P. Berrange wrote:
*************** *** 716,730 **** int ret;
if (!VIR_IS_CONNECTED_DOMAIN(domain)) { ! if (domain == NULL) ! virProxyError(NULL, VIR_ERR_INVALID_DOMAIN, __FUNCTION__); ! else ! virProxyError(domain->conn, VIR_ERR_INVALID_DOMAIN, __FUNCTION__); ! return (0); } if (info == NULL) { virProxyError(domain->conn, VIR_ERR_INVALID_ARG, __FUNCTION__); ! return (-1); } memset(&req, 0, sizeof(req)); req.command = VIR_PROXY_DOMAIN_INFO; --- 718,734 ---- int ret;
if (!VIR_IS_CONNECTED_DOMAIN(domain)) { ! if (domain == NULL) ! virProxyError(NULL, VIR_ERR_INVALID_DOMAIN, __FUNCTION__); ! else ! virProxyError(domain->conn, VIR_ERR_INVALID_DOMAIN, __FUNCTION__); ! return (-1);
Agreed for the Proxy driver you now want to return -1 on Info if it's not a connected domain. We already emitted an error there anyway make sense, but I wonder why it used to return 0 there ... hum....
Just looks like a typo to me.
--- src/xend_internal.c 15 Nov 2006 02:34:04 -0000 *************** *** 1266,1272 **** return node; }
! static int xend_get_config_version(virConnectPtr conn) { struct sexpr *root; const char *value; --- 1266,1272 ---- return node; }
! int xend_get_config_version(virConnectPtr conn) { struct sexpr *root; const char *value;
I assume exporting it is needed for other patches. Just check :-)
Yeah, need to use it in xm_internal when generating the SEXPR to feed to Xend to start an inactive domain.
Now committed. 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 -=|
With the paravirtualized framebuffer, the 'vnc' related options are all within the (image (linux ...)) bit of the S-Expression. When using the pygrub bootloader though, there is no (image) block in the SXPR at all. This patch changes this, so that if a bootloader is used, then we still generate an (image) block containing only the VNC related bits. This patch is dependant on a corresponding fix to XenD server - I'm also attaching this patch here for conveinance. It appears as if the paravirt framebuffer configuration will change significantly before it is merged upstream in xen-devel, so I don't anticpate the attached patch being merged in libvirt CVS. Instead it'll probably live in the Fedora / RHEL RPM spec files as a patch. I include it here merely for completeness 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 Wed, Nov 15, 2006 at 02:27:53AM +0000, Daniel P. Berrange wrote:
With the paravirtualized framebuffer, the 'vnc' related options are all within the (image (linux ...)) bit of the S-Expression. When using the pygrub bootloader though, there is no (image) block in the SXPR at all. This patch changes this, so that if a bootloader is used, then we still generate an (image) block containing only the VNC related bits.
This patch is dependant on a corresponding fix to XenD server - I'm also attaching this patch here for conveinance. It appears as if the paravirt framebuffer configuration will change significantly before it is merged upstream in xen-devel, so I don't anticpate the attached patch being merged in libvirt CVS. Instead it'll probably live in the Fedora / RHEL RPM spec files as a patch. I include it here merely for completeness
Okay, I just note the second patch is not contextual. It's too bad we can't extract whether xend has the feature, but well ... Daniel -- Red Hat Virtualization group http://redhat.com/virtualization/ Daniel Veillard | virtualization library http://libvirt.org/ veillard@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
On Wed, Nov 15, 2006 at 05:15:31AM -0500, Daniel Veillard wrote:
On Wed, Nov 15, 2006 at 02:27:53AM +0000, Daniel P. Berrange wrote:
With the paravirtualized framebuffer, the 'vnc' related options are all within the (image (linux ...)) bit of the S-Expression. When using the pygrub bootloader though, there is no (image) block in the SXPR at all. This patch changes this, so that if a bootloader is used, then we still generate an (image) block containing only the VNC related bits.
This patch is dependant on a corresponding fix to XenD server - I'm also attaching this patch here for conveinance. It appears as if the paravirt framebuffer configuration will change significantly before it is merged upstream in xen-devel, so I don't anticpate the attached patch being merged in libvirt CVS. Instead it'll probably live in the Fedora / RHEL RPM spec files as a patch. I include it here merely for completeness
Okay, I just note the second patch is not contextual. It's too bad we can't extract whether xend has the feature, but well ...
Well, one option is to just send this style SXPR regardless and not even bother to try and detect if my patch is enabled in xend - pushing the failure from libvirt space, to xend space. We'll be able to detect the new style paravirt framebuffer code in xen because they'll hopefully update xend_config_version counter when merging it. 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 -=|
This is the main patch. It adds a new 'xm config file' backend driver to libvirt. This basically just scans /etc/xen for config files, reads them and turns them into libvirt XML to create domains. Similarly it can accept libvirt XML and create config files from it. Some important points - It can override /etc/xen with LIBVIRT_XM_CONFIG_DIR env variable - It only re-scans /etc/xen every X seconds & caches parsed config files in-memory to avoid parsing / I/O overhead - Automatically generates a UUID if the config file doesn;t have one - Skips over a blacklist of files from /etc/xen which are known to not be valid config files to avoid too many error messages - Filters list of inactive domains to strip out any which are currently running (acccording to XenD) - Allows changing of VCPUS, memory & max memory config settings and writes changes back out to disk - Can start, create, delete domains from disk 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 Wed, Nov 15, 2006 at 02:33:26AM +0000, Daniel P. Berrange wrote:
This is the main patch. It adds a new 'xm config file' backend driver to libvirt. This basically just scans /etc/xen for config files, reads them and turns them into libvirt XML to create domains. Similarly it can accept libvirt XML and create config files from it.
Haha, the whole point of the exercise :-)
Some important points
- It can override /etc/xen with LIBVIRT_XM_CONFIG_DIR env variable - It only re-scans /etc/xen every X seconds & caches parsed config files in-memory to avoid parsing / I/O overhead - Automatically generates a UUID if the config file doesn;t have one - Skips over a blacklist of files from /etc/xen which are known to not be valid config files to avoid too many error messages - Filters list of inactive domains to strip out any which are currently running (acccording to XenD) - Allows changing of VCPUS, memory & max memory config settings and writes changes back out to disk - Can start, create, delete domains from disk
+ /* This method is called by various methods to scan /etc/xen + (or whatever directory was set by LIBVIRT_XM_CONFIG_DIR + environment variable) and process any domain configs. It + has rate-limited so never rescans more frequently than + once every X seconds */
Right, I don't want to be chastanized publicly by Dave Jones again at OLS 2007 :-)
+ static int xenXMConfigCacheRefresh(void) { [...] + /* Skip anything which isn't a file (takes care of scripts/ subdir */ + if ((stat(path, &st) < 0) || + !S_ISREG(st.st_mode)) {
I would put extra braces (!S_ISREG(st.st_mode)) ... paranoid as usual :-)
+ memset(info, 0, sizeof(virDomainInfo)); + if (xenXMConfigGetInt(entry->conf, "memory", &mem) < 0 || + mem < 0) + info->memory = 64 * 1024;
64 MB is a minimal memory limit that we are referencing in various places in libvirt for Xen back-ends, maybe that ought to be isolated in internal.h
+ virBufferAdd(buf, "<domain type='xen'>\n", -1); + virBufferVSprintf(buf, " <name>%s</name>\n", name);
Okay one of the things which scares me a bit here is encoding for the strings. We generate XML files without any encoding information which means it has to be UTF-8 (or UTF-16 but clearly it's not), it was more or less fine to do those direct buffers write before because we were in a contained environment, but now those strings come from external files, and well a chinese person may genuinely want to use chinese name in his config file and use BIG5 encoding for example, and in such case the config file generated here would not be XML. I assume config files generated by virt-manager when creating new domains are likely to use the strings returned from Gnome widget, I wonder if it is UTF-8 or the encoding of the user's locale. I guess the simplest and cleanest at this point would be to make sure anything parsed by conf.[ch] is actually UTF-8, and maybe double check virt-manager string generation.
+ virBufferVSprintf(buf, " <graphics type='vnc' port='%d'/>\n", (5900+display));
Shouldn't the 5900 magic number be in a header too ?
+ /* + * Start a domain from an existing defined config file + */ + int xenXMDomainCreate(virDomainPtr domain) { + char *xml; + char *sexpr; + int ret, xendConfigVersion; + unsigned char uuid[16]; + + if ((domain == NULL) || (domain->conn == NULL) || (domain->name == NULL)) { + xenXMError((domain ? domain->conn : NULL), VIR_ERR_INVALID_ARG, + __FUNCTION__); + return(-1); + } + + if (domain->handle != -1) + return (-1); + if (domain->conn->flags & VIR_CONNECT_RO) + return (-1); + + if (!(xml = xenXMDomainDumpXML(domain, 0))) + return (-1); + + if ((xendConfigVersion = xend_get_config_version(domain->conn)) < 0) { + xenXMError(domain->conn, VIR_ERR_INTERNAL_ERROR, "cannot determine xend config version"); + return (-1); + } + printf("'%s'\n", xml); + if (!(sexpr = virDomainParseXMLDesc(xml, NULL, xendConfigVersion))) { + free(xml); + return (-1); + } + free(xml); + printf("'%s'\n", sexpr); + ret = xenDaemonDomainCreateLinux(domain->conn, sexpr); + free(sexpr); + if (ret != 0) { + fprintf(stderr, "Failed to create domain %s\n", domain->name); + return (-1); + } + + ret = xend_wait_for_devices(domain->conn, domain->name); + if (ret != 0) { + fprintf(stderr, "Failed to get devices for domain %s\n", domain->name); + return (-1); + } + + if ((ret = xenDaemonDomainLookupByName_ids(domain->conn, domain->name, uuid)) < 0) { + return (-1); + } + domain->handle = ret; + + ret = xenDaemonDomainResume(domain); + if (ret != 0) { + fprintf(stderr, "Failed to resume new domain %s\n", domain->name); + xenDaemonDomainDestroy(domain); + domain->handle = -1; + return (-1); + } + + return 0; + }
Good to finally have the bridge from config files to libvirt loading :-) Very cool ! Push it :-) Daniel -- Red Hat Virtualization group http://redhat.com/virtualization/ Daniel Veillard | virtualization library http://libvirt.org/ veillard@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
On Wed, Nov 15, 2006 at 05:45:09AM -0500, Daniel Veillard wrote:
On Wed, Nov 15, 2006 at 02:33:26AM +0000, Daniel P. Berrange wrote:
This is the main patch. It adds a new 'xm config file' backend driver to libvirt. This basically just scans /etc/xen for config files, reads them and turns them into libvirt XML to create domains. Similarly it can accept libvirt XML and create config files from it.
Haha, the whole point of the exercise :-)
Some important points
- It can override /etc/xen with LIBVIRT_XM_CONFIG_DIR env variable
I added an extra check so that this environment variable is *NOT* used if getuid() != geteuid(), or getgid() != getegid() to avoid a potential security issue in setuid usage.
- It only re-scans /etc/xen every X seconds & caches parsed config files in-memory to avoid parsing / I/O overhead - Automatically generates a UUID if the config file doesn;t have one - Skips over a blacklist of files from /etc/xen which are known to not be valid config files to avoid too many error messages - Filters list of inactive domains to strip out any which are currently running (acccording to XenD) - Allows changing of VCPUS, memory & max memory config settings and writes changes back out to disk - Can start, create, delete domains from disk
+ /* This method is called by various methods to scan /etc/xen + (or whatever directory was set by LIBVIRT_XM_CONFIG_DIR + environment variable) and process any domain configs. It + has rate-limited so never rescans more frequently than + once every X seconds */
Right, I don't want to be chastanized publicly by Dave Jones again at OLS 2007 :-)
+ static int xenXMConfigCacheRefresh(void) { [...] + /* Skip anything which isn't a file (takes care of scripts/ subdir */ + if ((stat(path, &st) < 0) || + !S_ISREG(st.st_mode)) {
I would put extra braces (!S_ISREG(st.st_mode)) ... paranoid as usual :-)
Ok, added that.
+ memset(info, 0, sizeof(virDomainInfo)); + if (xenXMConfigGetInt(entry->conf, "memory", &mem) < 0 || + mem < 0) + info->memory = 64 * 1024;
64 MB is a minimal memory limit that we are referencing in various places in libvirt for Xen back-ends, maybe that ought to be isolated in internal.h
I committed the code as is for now. There're a bunch of things which can be shared between various backends, so I'll knock up a separate patch to pull out some of this duplicated code.
+ virBufferAdd(buf, "<domain type='xen'>\n", -1); + virBufferVSprintf(buf, " <name>%s</name>\n", name);
Okay one of the things which scares me a bit here is encoding for the strings. We generate XML files without any encoding information which means it has to be UTF-8 (or UTF-16 but clearly it's not), it was more or less fine to do those direct buffers write before because we were in a contained environment, but now those strings come from external files, and well a chinese person may genuinely want to use chinese name in his config file and use BIG5 encoding for example, and in such case the config file generated here would not be XML. I assume config files generated by virt-manager when creating new domains are likely to use the strings returned from Gnome widget, I wonder if it is UTF-8 or the encoding of the user's locale.
GNOME will use whatever your localized character set is defined to. Fedora defaults to UTF-8 everywhere, but this isn't true in the general case. This should be fixed in the virtinst library when it generates its XML to create new domains, ensuring a sensible encoding is specified.
I guess the simplest and cleanest at this point would be to make sure anything parsed by conf.[ch] is actually UTF-8, and maybe double check virt-manager string generation.
I think we have to expect the char set to be bsaed on user's locale rather than assuming the config files are UTF-8 ? This all said, the only bit of the config file which is free text is its name, and since the name is used for the filename of the config too, this will have to be encoded UTF-8. So I guess validating UTF-8 compliance when parsing is sufficient.
+ virBufferVSprintf(buf, " <graphics type='vnc' port='%d'/>\n", (5900+display));
Shouldn't the 5900 magic number be in a header too ?
Yes, will refactor later.
Good to finally have the bridge from config files to libvirt loading :-)
Very cool ! Push it :-)
Committed. 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, Nov 15, 2006 at 02:11:59AM +0000, Daniel P. Berrange wrote:
The following series of patches is an update of my previous patches for supporting inactive domains in libvirt. A rough highlight of changes since last time:
- Can override /etc/xen by setting LIBVIRT_XM_CONFIG_DIR env variable for people who move it to non-standard location - Several memory corruption bugs in config file refresh were solved - Fixed booting with paravirt framebuffer + grub - Added support for new APIs to python binding - Removed logic for supressing 404 errors (already merged earlier) - More complete device handling support (now creates network devices)
Previous posting was here:
http://www.redhat.com/archives/libvir-list/2006-September/msg00003.html
I forgot to mention, there is a branch of the main virt-manager codebase which adds support for managing inactive domains. This is the main way I have been validating correct operation of these patches (along with the standard virsh). http://hg.et.redhat.com/virt/applications/virt-manager--passive The only issue this has exposed was not in the patches themselves, but in XenD - the filtering of currently running domains from the inactive domain list imposes a non-trivial performance hit. This is because of the absolutely ridiculous number of XenStored database transactions that XenD triggers. I've investigated many ways around this, but none made any signficant difference - this ultimately has to be fixed in xenstored which is something targetted for xen 3.0.4 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 Wed, Nov 15, 2006 at 02:11:59AM +0000, Daniel P. Berrange wrote:
The following series of patches is an update of my previous patches for supporting inactive domains in libvirt. A rough highlight of changes since last time:
- Can override /etc/xen by setting LIBVIRT_XM_CONFIG_DIR env variable for people who move it to non-standard location - Several memory corruption bugs in config file refresh were solved - Fixed booting with paravirt framebuffer + grub - Added support for new APIs to python binding - Removed logic for supressing 404 errors (already merged earlier) - More complete device handling support (now creates network devices)
Sounds good to me, see other posts for each individual patch review :-)
Previous posting was here:
http://www.redhat.com/archives/libvir-list/2006-September/msg00003.html
Thanks ! Daniel -- Red Hat Virtualization group http://redhat.com/virtualization/ Daniel Veillard | virtualization library http://libvirt.org/ veillard@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
participants (2)
-
Daniel P. Berrange -
Daniel Veillard