[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH V6 1/3] Add a class for file descriptor sets



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.



[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]