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 :-)

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 -=|

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 + */

+ 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 + */

+ 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; + }

+ /** + * 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); + }

/************************************************************************ * * * 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.

*/ ! 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);

+ + + 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

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 :-)

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]); > + }

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);

--- 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;

*** 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);

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.

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

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 ?

Good to finally have the bridge from config files to libvirt loading :-) Very cool ! Push it :-)

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