On 02/14/2013 05:00 AM, Stefan Berger wrote:
Rather than passing the next-to-use file descriptor set Id and the hash table for rembering the mappings of aliases to file descriptor sets around, encapsulate the two in a class.
Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
---
In addition to Corey's review,
Index: libvirt/src/util/virfdset.c ===================================================================
+ +void virFdSetFree(virFdSetPtr fdset) +{ + virObjectUnref(fdset); +}
This function is not necessary; callers can directly use virObjectUnref().
+ +int virFdSetNextSet(virFdSetPtr fdset, const char *alias, + unsigned int *fdsetnum) +{ + int *num; + + if (VIR_ALLOC(num) < 0) { + virReportOOMError(); + return -1; + }
Is it necessary to allocate an integer? Rather, I would just:
+ + *num = fdset->nextfdset; + + if (virHashAddEntry(fdset->aliasToFdSet, alias, num) < 0) {
virHashAddEntry(fdset->aliasToFdSet, alias, (void*)fdset->nextfdset);
+ VIR_FREE(num); + return -1; + } + + *fdsetnum = fdset->nextfdset++;
That would also avoid the need for a hash cleanup function.
+static void virFdSetPrintAliasToFdSet(void *payload, + const void *name, + void *data) +{ + virBufferPtr buf = data; + + virBufferAsprintf(buf, " <entry alias='%s' fdset='%u'/>\n", + (char *)name, + *(unsigned int *)payload);
Minor type mismatch - the hash stores ints but here you are printing unsigned ints (won't matter in practice). Then again, if you store 'int' instead of 'int*' in the hashtable, this would just be '(int)payload' instead of '*(unsigned int*)payload'.
+} + +void virFdSetFormatXML(virFdSetPtr fdset, virBufferPtr buf) +{ + virBufferAsprintf(buf, "<fdsets>\n"); + virHashForEach(fdset->aliasToFdSet, virFdSetPrintAliasToFdSet, buf); + virBufferAsprintf(buf, "</fdsets>\n"); +} + +int virFdSetParseXML(virFdSetPtr fdset, const char *xPath, + xmlXPathContextPtr ctxt) +{ + xmlNodePtr *nodes = NULL; + int n, i; + char *key = NULL; + char *val = NULL; + unsigned int *fdsetnum = NULL; + int ret = 0; + + virFdSetReset(fdset);
Is anyone outside of this file ever going to call virFdSetReset, or can you mark it static?
+ + if ((n = virXPathNodeSet(xPath, ctxt, &nodes)) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("failed to parse qemu file descriptor sets")); + goto error; + } + if (n > 0) { + for (i = 0 ; i < n ; i++) { + key = virXMLPropString(nodes[i], "alias"); + val = virXMLPropString(nodes[i], "fdset"); + if (key && val) { + if (VIR_ALLOC(fdsetnum) < 0) { + virReportOOMError(); + ret = -1; + goto error;
Again, if you just store the int directly (since int is smaller than void*), this avoids wasteful allocations of an int*.
+++ libvirt/src/util/virfdset.h
+ +/** + * virFdSetNew + * + * Create a new FdSet, + * Returns pointer to new FdSet on success, NULL if no memory was available. + */ +virFdSetPtr virFdSetNew(void) ATTRIBUTE_RETURN_CHECK; + +/** + * virFdSetFree + * @fdset : the FdSet to free
No space before the colon.
Index: libvirt/src/libvirt_private.syms =================================================================== --- libvirt.orig/src/libvirt_private.syms +++ libvirt/src/libvirt_private.syms @@ -650,6 +650,16 @@ virFDStreamOpen; virFDStreamOpenFile;
+# virfdset.h +virFdSetFormatXML; +virFdSetFree; +virFdSetNew; +virFdSetNextSet; +virFdSetParseXML; +virFdSetRemoveAlias; +virFdSetReset; + + # hash.h virHashAddEntry; virHashCreate;
Hmm, this file is a bit out of order; it's easier to find exports if we were to sort sections by file name. I'll submit that patch independently. Overall looks pretty nice - I'm glad Dan provided a better design than what I had been thinking of. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org