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

Stefan Berger stefanb at linux.vnet.ibm.com
Fri Feb 15 22:34:35 UTC 2013


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 at 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.





More information about the libvir-list mailing list