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@xxxxxxxxxxxxxxxxxx>
>
> ---
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
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list