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@xxxxxxxxxxxxxxxxxx>
---
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.
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list