[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).


+    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]