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

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

> +
> +    *num = fdset->nextfdset;
> +
> +    if (virHashAddEntry(fdset->aliasToFdSet, alias, num) < 0) {

virHashAddEntry(fdset->aliasToFdSet, alias, (void*)fdset->nextfdset);

> +        VIR_FREE(num);
> +        return -1;
> +    }
> +
> +    *fdsetnum = fdset->nextfdset++;

That would also avoid the need for a hash cleanup function.

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

Then again, if you store 'int' instead of 'int*' in the hashtable, this
would just be '(int)payload' instead of '*(unsigned int*)payload'.

> +}
> +
> +void virFdSetFormatXML(virFdSetPtr fdset, virBufferPtr buf)
> +{
> +    virBufferAsprintf(buf, "<fdsets>\n");
> +    virHashForEach(fdset->aliasToFdSet, virFdSetPrintAliasToFdSet, buf);
> +    virBufferAsprintf(buf, "</fdsets>\n");
> +}
> +
> +int virFdSetParseXML(virFdSetPtr fdset, const char *xPath,
> +                     xmlXPathContextPtr ctxt)
> +{
> +    xmlNodePtr *nodes = NULL;
> +    int n, i;
> +    char *key = NULL;
> +    char *val = NULL;
> +    unsigned int *fdsetnum = NULL;
> +    int ret = 0;
> +
> +    virFdSetReset(fdset);

Is anyone outside of this file ever going to call virFdSetReset, or can
you mark it static?

> +
> +    if ((n = virXPathNodeSet(xPath, ctxt, &nodes)) < 0) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR,
> +                       "%s", _("failed to parse qemu file descriptor sets"));
> +        goto error;
> +    }
> +    if (n > 0) {
> +        for (i = 0 ; i < n ; i++) {
> +            key = virXMLPropString(nodes[i], "alias");
> +            val = virXMLPropString(nodes[i], "fdset");
> +            if (key && val) {
> +                if (VIR_ALLOC(fdsetnum) < 0) {
> +                    virReportOOMError();
> +                    ret = -1;
> +                    goto error;

Again, if you just store the int directly (since int is smaller than
void*), this avoids wasteful allocations of an int*.


> +++ libvirt/src/util/virfdset.h

> +
> +/**
> + * virFdSetNew
> + *
> + * Create a new FdSet,
> + * Returns pointer to new FdSet on success, NULL if no memory was available.
> + */
> +virFdSetPtr virFdSetNew(void) ATTRIBUTE_RETURN_CHECK;
> +
> +/**
> + * virFdSetFree
> + * @fdset : the FdSet to free

No space before the colon.

> Index: libvirt/src/libvirt_private.syms
> ===================================================================
> --- libvirt.orig/src/libvirt_private.syms
> +++ libvirt/src/libvirt_private.syms
> @@ -650,6 +650,16 @@ virFDStreamOpen;
>  virFDStreamOpenFile;
>  
>  
> +# virfdset.h
> +virFdSetFormatXML;
> +virFdSetFree;
> +virFdSetNew;
> +virFdSetNextSet;
> +virFdSetParseXML;
> +virFdSetRemoveAlias;
> +virFdSetReset;
> +
> +
>  # hash.h
>  virHashAddEntry;
>  virHashCreate;

Hmm, this file is a bit out of order; it's easier to find exports if we
were to sort sections by file name.  I'll submit that patch independently.

Overall looks pretty nice - I'm glad Dan provided a better design than
what I had been thinking of.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

Attachment: signature.asc
Description: OpenPGP digital signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]