[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