Re: [libvirt] [PATCH V6 1/3] Add a class for file descriptor sets
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(a)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
Attachments:
- signature.asc (application/pgp-signature — 621 bytes)