Re: [libvirt] [PATCH V6 1/3] Add a class for file descriptor sets
On 02/15/2013 03:34 PM, Stefan Berger wrote:
>> +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.
The virXYZNew() is nice. But the virXYZFree() is spurious - if we know
that XYZ is a virObject, then virObjectUnref() is sufficient. Besides,
if XYZ is a virObject, it is misleading to have a function named *Free()
that doesn't always free data (it frees data on the last unref, but if
other references are still held is it just decrementing the refcount).
For comparison, we have virDomainObjNew(), but no virDomainObjFree(),
because all clients of virDomainObjPtr use virObjectUnref().
>
>> +
>> +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.
Not true - the hash table makes a copy of sizeof(void*) bytes, and since
sizeof(void*) >= sizeof(int), casting int to void* lets you avoid an
allocation. We do it all the time:
src/nwfilter/nwfilter_dhcpsnoop.c: if
(virHashAddEntry(virNWFilterSnoopState.active, key, (void *)0x1) < 0) {
src/qemu/qemu_conf.c: if (virHashAddEntry(driver->sharedDisks,
key, (void *)0x1))
...
just that here, you would be using a value that is not necessarily 0x1.
>> +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.
Why does anyone outside this file ever have to do a reset? The set is
per-vm, and short of reloading (which the parseXML handles), I don't see
any reason why there need to be outside callers. But maybe I'll change
my mind when I get through the other patches.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachments:
- signature.asc (application/pgp-signature — 621 bytes)