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(a)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.