Parsing an XML configuration file for network monitoring

Question

I want to simplify this C code into different functions:

#include <stdio.h>
#include <string.h>
#include <stdlib.h>
#include <libxml/xmlmemory.h>
#include <libxml/parser.h>

// Declare functions
void parseSystemProperties(char *xmlFileName);
void parseSystemModules(xmlDocPtr doc, xmlNodePtr cur);
void parseSystemConfiguration(xmlDocPtr doc, xmlNodePtr cur);
void parseProfiles(xmlDocPtr doc, xmlNodePtr cur);
void parseMonitoringPort(xmlDocPtr doc, xmlNodePtr cur);
void parseWebServicePort(xmlDocPtr doc, xmlNodePtr cur);
void parseBindingInterfaces(xmlDocPtr doc, xmlNodePtr cur);
void parseExtensions(xmlDocPtr doc, xmlNodePtr cur);

int main(int argc, char **argv) {
    char *xmlFileName;

    if (argc <= 1) {
        printf("Usage: %s inputfile.xml\n", argv[0]);
        return (0);
    }

    // Get the file name from the argv[1]
    xmlFileName = argv[1];

    // Custom function to parse XML file
    parseSystemProperties(xmlFileName);

    return (1);
}

// Parsing the XML file and Reading the Element Nodes

void parseSystemProperties(char *xmlFileName) {
    xmlDocPtr doc; // pointer to parse xml Document
    xmlNodePtr cur; // node pointer. It interacts with individual node

    // Parse XML file
    doc = xmlParseFile(xmlFileName);

    // Check to see that the document was successfully parsed.
    if (doc == NULL) {
        fprintf(stderr, "Error!. Document is not parsed successfully. \n");
        return;
    }

    // Retrieve the document's root element - system-properties
    cur = xmlDocGetRootElement(doc);

    // Check to make sure the document actually contains something
    if (cur == NULL) {
        fprintf(stderr, "Document is Empty\n");
        xmlFreeDoc(doc);
        return;
    }

    /* We need to make sure the document is the right type.
     * "system-properties" is the root type of the documents used in user Config XML file
     */
    if (xmlStrcmp(cur->name, (const xmlChar *) "system-properties")) {
        fprintf(stderr, "Cannot find system-properties");
        xmlFreeDoc(doc);
        return;
    }

    /* Get the first child node of cur.
     * At this point, cur points at the document root,
     * which is the element "root"
     */

    cur = cur->xmlChildrenNode;

    // This loop iterates through the elements that are children of "root"
    while (cur != NULL) {
        // Get the data from system-modules
        if ((!xmlStrcmp(cur->name, (const xmlChar *) "system-modules"))) {
            parseSystemModules(doc, cur);
        }

        // Get the values from system-configuration
        if ((!xmlStrcmp(cur->name, (const xmlChar *) "system-configuration"))) {
            parseSystemConfiguration(doc, cur);
        }

        cur = cur->next;
    }

    /* Save XML document to the Disk
     * Otherwise, you changes will not be reflected to the file.
     * Currently it's only in the memory
     */
    // xmlSaveFormatFile (xmlFileName, doc, 1);

    /* free the document */
    xmlFreeDoc(doc);

    /*
     * Free the global variables that may
     * have been allocated by the parser.
     */
    xmlCleanupParser();

    return;

} // end of function

// Get Modules part
// -------------------------------------------

void parseSystemModules(xmlDocPtr doc, xmlNodePtr cur) {
    xmlChar *key;
    xmlAttrPtr attr;

    // Get the sub Element Node of system-configuration node
    cur = cur->xmlChildrenNode;

    // This loop iterates through the elements that are children of system-configuration
    while (cur != NULL) {
        if ((!xmlStrcmp(cur->name, (const xmlChar *) "extensions"))) {
            parseExtensions(doc, cur);
        }
        cur = cur->next;
    }

    return;

} // end of function()

void parseExtensions(xmlDocPtr doc, xmlNodePtr cur) {
    xmlChar *key;
    xmlAttrPtr attr;

    // Get the sub Element Node of Profiles node
    cur = cur->xmlChildrenNode;

    // This loop iterates through the elements that are children of Profiles
    while (cur != NULL) {
        if ((!xmlStrcmp(cur->name, (const xmlChar *) "extension"))) {
            key = xmlGetProp(cur, (const xmlChar*) "module");
            fprintf(stderr, "module: %s\n", key);
            xmlFree(key);
        }
        cur = cur->next;
    }

    return;

} // end of function()



// Get Configuration part
// -------------------------------------------

void parseSystemConfiguration(xmlDocPtr doc, xmlNodePtr cur) {
    xmlChar *key;
    xmlAttrPtr attr;

    // Get the sub Element Node of system-configuration node
    cur = cur->xmlChildrenNode;

    // This loop iterates through the elements that are children of system-configuration
    while (cur != NULL) {
        if ((!xmlStrcmp(cur->name, (const xmlChar *) "profiles"))) {
            parseProfiles(doc, cur);
        }
        cur = cur->next;
    }

    return;

} // end of function()

void parseProfiles(xmlDocPtr doc, xmlNodePtr cur) {
    xmlChar *key;
    xmlAttrPtr attr;

    // Get the sub Element Node of Profiles node
    cur = cur->xmlChildrenNode;

    // This loop iterates through the elements that are children of Profiles
    while (cur != NULL) {
        if ((!xmlStrcmp(cur->name, (const xmlChar *) "profile"))) {
            // Get monitoring-port
            parseMonitoringPort(doc, cur);
            // Get web-service-port
            parseWebServicePort(doc, cur);
            parseBindingInterfaces(doc, cur);
        }
        cur = cur->next;
    }

    return;

} // end of function()


// Monitoring Port

void parseMonitoringPort(xmlDocPtr doc, xmlNodePtr cur) {
    xmlChar *key;
    xmlAttrPtr attr;

    // Get the sub Element Node of Profile node
    cur = cur->xmlChildrenNode;

    // This loop iterates through the elements that are children of Profile
    while (cur != NULL) {
        if ((!xmlStrcmp(cur->name, (const xmlChar *) "monitoring-port"))) {
            key = xmlGetProp(cur, (const xmlChar*) "port");
            fprintf(stderr, "monitoring-port: %s\n", key);
            xmlFree(key);
        }
        cur = cur->next;
    }

    return;

} // end of function()


// web service port

void parseWebServicePort(xmlDocPtr doc, xmlNodePtr cur) {
    xmlChar *key;
    xmlAttrPtr attr;

    // Get the sub Element Node of Profile node
    cur = cur->xmlChildrenNode;

    // This loop iterates through the elements that are children of "root"
    while (cur != NULL) {
        if ((!xmlStrcmp(cur->name, (const xmlChar *) "web-service-port"))) {
            //parseMonitoringPort (doc, cur);
            key = xmlGetProp(cur, (const xmlChar*) "port");
            fprintf(stderr, "web-service-port: %s\n", key);
            xmlFree(key);
        }
        cur = cur->next;
    }

    return;

} // end of function()


// binding interface

void parseBindingInterfaces(xmlDocPtr doc, xmlNodePtr cur) {
    xmlChar *key;
    xmlAttrPtr attr;

    // Get the sub Element Node of Profile node
    cur = cur->xmlChildrenNode;

    // This loop iterates through the elements that are children of "root"
    while (cur != NULL) {
        if ((!xmlStrcmp(cur->name, (const xmlChar *) "socket-binding"))) {
            //parseMonitoringPort (doc, cur);
            key = xmlGetProp(cur, (const xmlChar*) "interface");
            fprintf(stderr, "interface: %s\n", key);
            xmlFree(key);
            key = xmlGetProp(cur, (const xmlChar*) "ipaddress");
            fprintf(stderr, "ipaddress: %s\n", key);
            xmlFree(key);
            key = xmlGetProp(cur, (const xmlChar*) "port");
            fprintf(stderr, "port: %s\n", key);
            xmlFree(key);
        }
        cur = cur->next;
    }

    return;

} // end of function()

I use while cycle to parse xml document and to call parseSystemModules and parseSystemConfiguration. Is there more simple ways to split the while cycle into C functions. I want to add more and more functions without any problem in future time.

This is the XML file:

<?xml version="1.0" encoding="UTF-8"?>
<system-properties version="1.0">
    <system-modules>
        <extensions>
            <extension module="NetworkModule"/>
            <extension module="MonitoringModule"/>
            <extension module="OracleModule"/>
            <extension module="DataFilterModule"/>          
        </extensions>   
    </system-modules>
    <system-configuration>
        <profiles>
            <profile name="default">
                <monitoring-port port="6051"/>
                <web-service-port port="7050"/>
                <socket-binding interface="management" ipaddress="192.168.1.101" port="6050"/>
                <socket-binding interface="monitoring" ipaddress="192.168.1.106" port="7050"/>
                <network-pool threads="40"/>                    
            </profile>
        </profiles>
    </system-configuration>
</system-properties>

Answer 1

A few observations, Avoid comments that do not tell the reader any thing that they can not understand by reading the code. They just add to the visual noice. Instead add comments as to how the function fits in with the rest of the program, or how the function is to be used.

#include <stdio.h>
#include <string.h>
#include <stdlib.h>
#include <libxml/xmlmemory.h>
#include <libxml/parser.h>

A small observation about your code is that it prints the error to the error stream in the case of errors. This is not always the best solution. A better idea is to indicate the error with a return error code and let the caller decide what to do about the error. (This is not implemented in below code.)

typedef enum {
  SUCCESS,
  NO_DOC,
  NO_PARSE,
  NO_ELEM
} ParseResult;

typedef void (*xmlFn)(xmlDocPtr, xmlNodePtr);

ParseResult parseXml(char *xmlFileName);
void parseSystemProperties(xmlDocPtr doc, xmlNodePtr cur);
void parseSystemModules(xmlDocPtr doc, xmlNodePtr cur);
void parseSystemConfiguration(xmlDocPtr doc, xmlNodePtr cur);
void parseProfiles(xmlDocPtr doc, xmlNodePtr cur);
void parseMonitoringPort(xmlDocPtr doc, xmlNodePtr cur);
void parseWebServicePort(xmlDocPtr doc, xmlNodePtr cur);
void parseBindingInterfaces(xmlDocPtr doc, xmlNodePtr cur);
void parseExtensions(xmlDocPtr doc, xmlNodePtr cur);

Always indicate success in main with 0 rather than 1. We do this because we have to interact with the system outside where 0 is considered a success any any other value is considered an error code.

Also avoid extra ((())) in the function arguments. Especially, it is not required in return statements.

int main(int argc, char **argv) {
    if (argc <= 1) {
        printf("Usage: %s inputfile.xml\n", argv[0]);
        return 1;
    }
    parseXml(argv[1]);
    return 0;
}


void xprint(xmlNodePtr cur, const char* var, const char* msg) {
  xmlChar *key = xmlGetProp(cur, (const xmlChar*) var);
  fprintf(stderr, "%s: %s\n", msg, key);
  xmlFree(key);
}

From looking at your cdode, it seems that you have primarily two types of functions. Abstracting them away can help you to avoid duplication.

void parseFn(xmlDocPtr doc, xmlNodePtr cur, const char* check, xmlFn myfn) {
  if (xmlStrcmp(cur->name, (const xmlChar*) check)) return;
  for(cur = cur->xmlChildrenNode; cur; cur=cur->next) myfn(doc, cur);
}

void parseForFn(xmlDocPtr doc, xmlNodePtr cur, const char* check, xmlFn myfn) {
  for(cur = cur->xmlChildrenNode; cur; cur=cur->next) {
    if (xmlStrcmp(cur->name, (const xmlChar*) check)) continue;
    myfn(doc, cur);
  }
}

Another recommendation I have is to move the checks for system-modules and system-configuration into the respective functions as pre-conditions. This will allow you to simplify.

ParseResult parseXml(char *xmlFileName) {
    xmlDocPtr doc; 
    xmlNodePtr cur;
    ParseResult result = SUCCESS;
    doc = xmlParseFile(xmlFileName);
    if (!doc) return NO_DOC;
    cur = xmlDocGetRootElement(doc);
    if (cur == NULL) {
        result = NO_PARSE;
        goto cleanup;
    }

    /* We need to make sure the document is the right type.
     * "system-properties" is the root type of the documents used in user Config XML file
     */
    /* I decided to extract this to parseSystemProperties, but in doing
       do, I lost the NO_SYSTEM_PROP output. It could be added to the current
       framework by making the functions return NO_ELEM, and checking for
       it here. */
    parseSystemProperties(doc, cur);
cleanup:
    xmlFreeDoc(doc);
    xmlCleanupParser();
    return result;    
}

Note the use of goto, I feel that it is better to do this than to distribute the cleanup code every where else in the function.

Now, observe the functions below. Another way to do them is populate them into a table rather than defining function wrappers as below. An advantage of that approach is that, you could avoid defining too many helper_x functions.

void parseSystemProperties_x(xmlDocPtr doc, xmlNodePtr cur) {
  parseSystemModules(doc, cur);
  parseSystemConfiguration(doc, cur);
}

void parseSystemProperties(xmlDocPtr doc, xmlNodePtr cur) {
  parseFn(doc,cur, "system-properties",parseSystemProperties_x);
}

void parseSystemModules(xmlDocPtr doc, xmlNodePtr cur) {
  parseFn(doc,cur, "system-modules",parseExtensions);
}

void parseSystemConfiguration(xmlDocPtr doc, xmlNodePtr cur) {
  parseFn(doc,cur, "system-configuration",parseProfiles);
}

void parseProfiles_x(xmlDocPtr doc, xmlNodePtr cur) {
  parseMonitoringPort(doc, cur);
  parseWebServicePort(doc, cur);
  parseBindingInterfaces(doc, cur);
}

void parseProfiles(xmlDocPtr doc, xmlNodePtr cur) {
  parseFn(doc,cur, "profiles", parseProfiles_x);
}

printing attribute values is the other kind of function.

void parseExtensions_x(xmlDocPtr doc, xmlNodePtr cur) {
  if (xmlStrcmp(cur->name, "extension")) return;
  xprint(cur, "module", "module");
}

void parseExtensions(xmlDocPtr doc, xmlNodePtr cur) {
  parseFn(doc,cur, "extensions", parseExtensions_x);
}

void parseMonitoringPort_x(xmlDocPtr doc, xmlNodePtr cur) {
  xprint(cur, "port", "monitoring-port");
}

void parseMonitoringPort(xmlDocPtr doc, xmlNodePtr cur) {
  parseForFn(doc,cur, "monitoring-port", parseMonitoringPort_x);
}

void parseWebServicePort_x(xmlDocPtr doc, xmlNodePtr cur) {
  xprint(cur, "port", "web-service-port");
}

void parseWebServicePort(xmlDocPtr doc, xmlNodePtr cur) {
  parseForFn(doc,cur, "web-service-port", parseWebServicePort_x);
}

void parseBindingInterfaces_x(xmlDocPtr doc, xmlNodePtr cur) {
  xprint(cur, "interface", "interface");
  xprint(cur, "ipaddress", "ipaddress");
  xprint(cur, "port", "port");
}

void parseBindingInterfaces(xmlDocPtr doc, xmlNodePtr cur) {
  parseForFn(doc,cur, "socket-binding", parseBindingInterfaces_x);
}