On 02/15/2013 04:49 PM, Eric Blake wrote:
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().
I personally find it 'nicer' to have a virXYZNew() and a virXYZFree() . I think Daniel had suggested something like that.
+ +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:
Yes, it's necessary since the hash table does not make a copy of the void * since it doesn't know the size of the value. However, it makes a copy of the key.
+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).
Fixed. + + virFdSetReset(fdset);
Is anyone outside of this file ever going to call virFdSetReset, or can you mark it static?
We have callers.