Re: [Libvirt-cim] [PATCH v5] vlan extention - add function library for readonly usage

Thank you for the quick turnaround. The patch applied and built with no issues. Overall, I think the patch looks great. See my comments in-line. Test program: NOTE: I had to remove an existing installation of libvirt-cim, use make rpm, and install an rpm with this change to get the test program to work. Otherwise, the problem would link to the wrong version of libxkutil or not properly find it. So, just updating the rpm was the quickest way to test.

$ sudo CU_DEBUG=stdout .libs/network_parsing_test .libs/network_parsing_test: symbol lookup error: .libs/network_parsing_test: undefined symbol: eth_ifaceslist_init For reference, I'm using RHEL 6.2. # rpm -qa | grep libvirt libvirt-devel-0.9.4-23.el6_2.1.x86_64 libvirt-0.9.4-23.el6_2.1.x86_64 libvirt-cim-0.6.0-1.el6.x86_64 Here's the output on my system: network_parsing_test.c(30): time is [1327531475773] ms. misc_util.c(168): Connecting to libvirt with uri `qemu:///system' misc_util.c(130): 'readonly' not found in config file, assuming false network_parsing.c(593): 0 defined ifaces found from libvirt API. network_parsing.c(630): 1 active ifaces found from libvirt API. network_parsing.c(642): active interface 0 xml: <interface type='ethernet' name='lo'> <start mode='onboot'/> <protocol family='ipv4'> <ip address='127.0.0.1' prefix='8'/> </protocol> </interface> network_parsing_test.c(30): time is [1327531475831] ms. network_parsing_test.c(51): cost [58]ms in discovering host network. Result: Have 1 ifaces in the list: 0000 Iface device: name lo. --Main Props: parent (null), attach to (null), mac (null), status 1, boot_mode 1, iface type 0x100. network_parsing_test.c(55): xml gen msg is (null). xml is: <tmp> <interface name="lo" type="ethernet"> <start mode="onboot"/> </interface> </tmp> This matches the output I get with either netcf or virsh. On 01/25/2012 03:04 PM, xiaxia347work(a)163.com wrote: > From: Wenchao Xia&lt;xiawenc(a)linux.vnet.ibm.com&gt; > > This patch contain 1 test program and 1 c file doing xml and structure > mapping. It expose some API to translate xml to device structure, in a similar > way to code in device_parsing.c. Also some other files are changed to let new > code use their internal functions. Type following Command in libxkutil > directory: > sudo CU_DEBUG=stdout .libs/network_parsing_test > could see what it have done. > > v5: calling libvirt API which employ netcf instead of using libnl. From git log > these API are available since 0.9.2, and this patch is tested with libvirt > 0.9.4. > > Signed-off-by: Wenchao Xia&lt;xiawenc(a)linux.vnet.ibm.com&gt; > --- > libxkutil/Makefile.am | 12 +- > libxkutil/misc_util.c | 10 +- > libxkutil/network_parsing.c | 665 ++++++++++++++++++++++++++++++++++++++ > libxkutil/network_parsing.h | 160 +++++++++ > libxkutil/network_parsing_test.c | 61 ++++ > libxkutil/xmlgen.c | 2 +- > libxkutil/xmlgen.h | 2 + > 7 files changed, 906 insertions(+), 6 deletions(-) > create mode 100644 libxkutil/network_parsing.c > create mode 100644 libxkutil/network_parsing.h > create mode 100644 libxkutil/network_parsing_test.c > > diff --git a/libxkutil/Makefile.am b/libxkutil/Makefile.am > index f1adc03..c0e62eb 100644 > --- a/libxkutil/Makefile.am > +++ b/libxkutil/Makefile.am > @@ -4,12 +4,14 @@ AM_CFLAGS = $(CFLAGS_STRICT) \ > -DLIBVIRTCIM_CONF=\"@sysconfdir@/@PACKAGE@.conf\" > > noinst_HEADERS = cs_util.h misc_util.h device_parsing.h xmlgen.h infostore.h \ > - pool_parsing.h acl_parsing.h > + pool_parsing.h acl_parsing.h \ > + network_parsing.h > > lib_LTLIBRARIES = libxkutil.la > > libxkutil_la_SOURCES = cs_util_instance.c misc_util.c device_parsing.c \ > - xmlgen.c infostore.c pool_parsing.c acl_parsing.c > + xmlgen.c infostore.c pool_parsing.c acl_parsing.c \ > + network_parsing.c > libxkutil_la_LDFLAGS = -version-info @VERSION_INFO@ > libxkutil_la_LIBADD = @LIBVIRT_LIBS@ \ > @LIBUUID_LIBS@ > @@ -19,3 +21,9 @@ noinst_PROGRAMS = xml_parse_test > xml_parse_test_SOURCES = xml_parse_test.c > xml_parse_test_LDADD = libxkutil.la \ > @LIBVIRT_LIBS@ > + > +noinst_PROGRAMS += network_parsing_test > + > +network_parsing_test_SOURCES = network_parsing_test.c > +network_parsing_test_LDADD = libxkutil.la \ > + @LIBVIRT_LIBS@ > diff --git a/libxkutil/misc_util.c b/libxkutil/misc_util.c > index 61893c3..1c18c33 100644 > --- a/libxkutil/misc_util.c > +++ b/libxkutil/misc_util.c > @@ -152,9 +152,13 @@ virConnectPtr connect_by_classname(const CMPIBroker *broker, > > uri = cn_to_uri(classname); > if (!uri) { > - cu_statusf(broker, s, > - CMPI_RC_ERR_FAILED, > - "Unable to generate URI from classname"); > + if (broker) { > + cu_statusf(broker, s, > + CMPI_RC_ERR_FAILED, > + "Unable to generate URI from classname"); > + } > + CU_DEBUG("Unable to generate URI from classname," > + " uri is %s.", uri); > return NULL; > } > > diff --git a/libxkutil/network_parsing.c b/libxkutil/network_parsing.c > new file mode 100644 > index 0000000..932000c > --- /dev/null > +++ b/libxkutil/network_parsing.c > @@ -0,0 +1,665 @@ > +/* > + * Copyright IBM Corp. 2012 > + * > + * Authors: > + * Wenchao Xia&lt;xiawenc(a)cn.ibm.com&gt; > + * > + * This library is free software; you can redistribute it and/or > + * modify it under the terms of the GNU Lesser General Public > + * License as published by the Free Software Foundation; either > + * version 2.1 of the License, or (at your option) any later version. > + * > + * This library is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > + * Lesser General Public License for more details. > + * > + * You should have received a copy of the GNU Lesser General Public > + * License along with this library; if not, write to the Free Software > + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA > + */ > +#include<stdio.h> > +#include<string.h> > +#include<stdlib.h> > +#include<stdbool.h> > +#include<inttypes.h> > +#include<sys/stat.h> > + > +#include<libvirt/libvirt.h> > +#include<libxml/tree.h> > +#include<libxml/parser.h> > +#include<libxml/xpath.h> > +#include<libcmpiutil/libcmpiutil.h> > + > +#include "misc_util.h" > +#include "xmlgen.h" > +#include "device_parsing.h" > +#include "network_parsing.h" > + > +#define XML_ERROR "Failed to allocate XML memory" I see this is dup'd from xmlgen.c. Perhaps we should just move it to xmlgen.h so everyone can share?

> + > +static void vlan_prop_print(struct VLAN_Prop *pvlan_prop) > +{ > + struct VLAN_Prop_8021q *p_8021q; > + CMD_DEBUG(1, "--VLAN props: type %d.\n", > + pvlan_prop->vlan_type); > + if (pvlan_prop->vlan_type == VLAN_TYPE_802_1_Q) { > + p_8021q =&(pvlan_prop->props.prop_8021q); > + CMD_DEBUG(1, "----IEEE802.1.Q: id %d, parent %s.\n", > + p_8021q->vlan_id, p_8021q->parent); > + } > +} > + > +static void br_prop_print(struct BR_Prop *pbr_prop) > +{ > + int i = 0; > + CMD_DEBUG(1, "--Bridge props: stp %d, delay %d, port_num %d.\n", > + pbr_prop->STP, pbr_prop->delay, pbr_prop->port_num); > + if (pbr_prop->port_names != NULL) { > + CMD_DEBUG(1, "----Ports attached: "); > + while (i< pbr_prop->port_num) { > + CMD_DEBUG(1, " %s,", *(pbr_prop->port_names+i)); > + i++; > + } > + CMD_DEBUG(1, "\n"); > + } > +} > + > +void eth_iface_print(struct EthIface *piface) > +{ > + CMD_DEBUG(1, "Iface device: name %s.\n" > + "--Main Props: parent %s, attach to %s, mac %s," > + " status %d, boot_mode %d, iface type 0x%x.\n", > + piface->name, piface->dep_ifname, piface->attach_bridge, > + piface->mac, > + piface->run_prop.status, piface->run_prop.boot_mode, piface->eth_type); > + if (piface->pbr_prop != NULL) { > + br_prop_print(piface->pbr_prop); > + } > + if (piface->pvlan_prop != NULL) { > + vlan_prop_print(piface->pvlan_prop); > + } > + return; > +} > + > +void eth_ifaceslist_print(struct EthIfacesList *plist) > +{ > + int i = 0; > + CMD_DEBUG(1, "Have %d ifaces in the list:\n", plist->count); > + while (i< plist->count) { > + CMD_DEBUG(1, "%04d ", i); > + eth_iface_print(plist->pifaces[i]); > + i++; > + } > +} > + > +static void vlan_prop_init(struct VLAN_Prop *pvlan_prop, int vlan_type) > +{ > + struct VLAN_Prop_8021q *p_8021q; > + memset(pvlan_prop, 0, sizeof(struct VLAN_Prop)); > + pvlan_prop->vlan_type = vlan_type; > + if (pvlan_prop->vlan_type == VLAN_TYPE_802_1_Q) { > + p_8021q =&(pvlan_prop->props.prop_8021q); > + p_8021q->vlan_id = NUM_NOT_GOT; > + } > +} > + > +static void br_prop_init(struct BR_Prop *pbr_prop) > +{ > + memset(pbr_prop, 0, sizeof(struct BR_Prop)); > + pbr_prop->STP = NUM_NOT_GOT; > + pbr_prop->delay = NUM_NOT_GOT; > + pbr_prop->port_num = NUM_NOT_GOT; > +} > + > +void eth_iface_init(struct EthIface *piface) > +{ > + memset(piface, 0, sizeof(struct EthIface)); > + piface->eth_type = ETH_TYPE_NOT_GOT; > + piface->run_prop.status = NUM_NOT_GOT; > + piface->run_prop.boot_mode = BOOT_MODE_NOT_GOT; > + return; > +} > + > +void eth_iface_add_br_prop(struct EthIface *piface) > +{ > + if (piface->pbr_prop != NULL) { > + return; > + } > + SAFE_MALLOC(piface->pbr_prop, sizeof(struct BR_Prop)); > + br_prop_init(piface->pbr_prop); > +} > + > +void eth_iface_add_vlan_prop(struct EthIface *piface, int vlan_type) > +{ > + if (piface->pvlan_prop != NULL) { > + return; > + } Indent above

> + SAFE_MALLOC(piface->pvlan_prop, sizeof(struct VLAN_Prop)); > + vlan_prop_init(piface->pvlan_prop, vlan_type); > +} > + > +static void vlan_prop_uninit(struct VLAN_Prop *pvlan_prop) > +{ > + struct VLAN_Prop_8021q *p_8021q; > + if (pvlan_prop == NULL) { > + return; > + } > + if (pvlan_prop->vlan_type == VLAN_TYPE_802_1_Q) { > + p_8021q =&(pvlan_prop->props.prop_8021q); > + SAFE_FREE(p_8021q->parent); > + } > +} > + > +static void br_prop_uninit(struct BR_Prop *pbr_prop) > +{ > + int i; > + if (pbr_prop == NULL) { > + return; > + } > + i = 0; why not init i when defined?

> + if (pbr_prop->port_names != NULL) { > + while (i< pbr_prop->port_num) { > + SAFE_FREE(pbr_prop->port_names[i]); > + i++; > + } > + SAFE_FREE(pbr_prop->port_names); > + } > +} > + > +void eth_iface_uninit(struct EthIface *piface) > +{ > + if (piface == NULL) { > + return; > + } > + SAFE_FREE(piface->name); > + SAFE_FREE(piface->dep_ifname); > + SAFE_FREE(piface->attach_bridge); > + SAFE_FREE(piface->mac); > + br_prop_uninit(piface->pbr_prop); > + SAFE_FREE(piface->pbr_prop); > + vlan_prop_uninit(piface->pvlan_prop); > + SAFE_FREE(piface->pvlan_prop); > + return; > +} > + > +void eth_ifaceslist_init(struct EthIfacesList *plist) > +{ > + plist->count = 0; > +} > + > +void eth_ifaceslist_uninit(struct EthIfacesList *plist) > +{ > + struct EthIface **t; > + int i; init variable to 0 or NULL

> + if (plist->count<= 0) { > + return; > + } > + t = plist->pifaces; > + i = 0; > + while (i< plist->count) { > + if (*t != NULL) { > + eth_iface_uninit(*t); > + SAFE_FREE(*t); > + } > + t++; > + i++; > + } > + return; > +} > + > +int eth_ifaceslist_add(struct EthIfacesList *plist, struct EthIface **ppiface) > +{ > + if (plist->count>= MAX_IFACE_NUM) { > + CU_DEBUG("too much device found."); > + return 0; > + } > + plist->pifaces[plist->count] = *ppiface; > + *ppiface = NULL; > + plist->count++; > + return 1; > +} > + > +struct EthIface *eth_ifaceslist_search(struct EthIfacesList *plist, > + char *name) > +{ > + int i = 0; > + struct EthIface *piface = NULL; > + > + while (i< plist->count) { > + piface = plist->pifaces[i]; > + i++; > + if (piface != NULL) { > + if (0 == strcmp(piface->name, name)) { > + return piface; > + } > + } > + } > + return NULL; > +} > + > +/* Dummy function to suppress error message from libxml2 */ > +static void swallow_err_msg(void *ctx, const char *msg, ...) > +{ > + /* do nothing, just swallow the message. */ > +} > + > + > +static const char *gen_eth_xmlnode_iface(xmlNodePtr root, > + struct EthIface *piface, > + struct EthIfacesList *plist, > + int bridge_port_flag) > +{ > + const char *msg = NULL; > + xmlNodePtr temp_node1 = NULL, temp_node2 = NULL, temp_node3 = NULL; > + char *str, buf[16]; > + int i; > + struct EthIface *pifaceport; By convention, we try to init all pointers. > + > + if (piface->name == NULL) { > + msg = "iface have no name.\n"; > + goto out; > + } > + /* netcfg have no xml for iface attatched to bridge */ s/have/has/

> + if ((piface->attach_bridge != NULL)&& (bridge_port_flag != 1)) { > + goto out; > + } > + > + temp_node1 = xmlNewChild(root, NULL, BAD_CAST "interface", NULL); > + if (temp_node1 == NULL) { > + msg = XML_ERROR; > + goto out; > + } > + xmlNewProp(temp_node1, BAD_CAST "name", BAD_CAST piface->name); > + if (piface->eth_type& ETH_TYPE_ETHER_ANY) { > + if (piface->eth_type& ETH_TYPE_ETHER_SUB_BRIDGE) { > + str = "bridge"; > + } else if (piface->eth_type& ETH_TYPE_ETHER_SUB_VLAN) { > + str = "vlan"; > + } else { > + str = "ethernet"; > + } > + xmlNewProp(temp_node1, BAD_CAST "type", BAD_CAST str); > + } > + > + if ((piface->pvlan_prop != NULL)&& > + (piface->pvlan_prop->vlan_type == VLAN_TYPE_802_1_Q)) { > + temp_node2 = xmlNewChild(temp_node1, NULL, BAD_CAST "vlan", NULL); > + snprintf(buf, sizeof(buf), > + "%d", piface->pvlan_prop->props.prop_8021q.vlan_id); > + xmlNewProp(temp_node2, BAD_CAST "tag", BAD_CAST buf); > + temp_node3 = xmlNewChild(temp_node2, NULL, BAD_CAST "interface", NULL); > + xmlNewProp(temp_node3, BAD_CAST "name", > + BAD_CAST piface->pvlan_prop->props.prop_8021q.parent); > + } > + > + /* if it is attached to bridge, only above properties could be set */ > + if (bridge_port_flag == 1) { > + goto out; > + } > + > + if (piface->pbr_prop != NULL) { > + temp_node2 = xmlNewChild(temp_node1, NULL, BAD_CAST "bridge", NULL); > + if (piface->pbr_prop->STP == 1) { > + snprintf(buf, sizeof(buf), "on"); > + } else { > + snprintf(buf, sizeof(buf), "off"); > + } > + xmlNewProp(temp_node2, BAD_CAST "stp", BAD_CAST buf); > + if (piface->pbr_prop->delay>= 0) { > + snprintf(buf, sizeof(buf), "%d", piface->pbr_prop->delay); > + xmlNewProp(temp_node2, BAD_CAST "delay", BAD_CAST buf); > + } > + if ((piface->pbr_prop->port_names != NULL)&& > + (piface->pbr_prop->port_num> 0)) { > + for (i = 0; i< piface->pbr_prop->port_num; i++) { > + pifaceport = eth_ifaceslist_search(plist, > + piface->pbr_prop->port_names[i]); > + if (pifaceport == NULL) { > + CU_DEBUG("failed to find port %s of bridge %s in list.", > + piface->pbr_prop->port_names[i], piface->name); > + } else { > + gen_eth_xmlnode_iface(temp_node2, pifaceport, plist, 1); > + } > + } > + } > + } > + > + if (piface->run_prop.boot_mode == BOOT_MODE_AUTOSTART) { > + temp_node2 = xmlNewChild(temp_node1, NULL, BAD_CAST "start", NULL); > + xmlNewProp(temp_node2, BAD_CAST "mode", BAD_CAST "onboot"); > + } else if (piface->run_prop.boot_mode == BOOT_MODE_NONE) { > + temp_node2 = xmlNewChild(temp_node1, NULL, BAD_CAST "start", NULL); > + xmlNewProp(temp_node2, BAD_CAST "mode", BAD_CAST "none"); > + } > + if (piface->mac != NULL) { > + temp_node2 = xmlNewChild(temp_node1, NULL, BAD_CAST "mac", NULL); > + xmlNewProp(temp_node2, BAD_CAST "address", BAD_CAST piface->mac); > + } > + > + out: > + return msg; > +} > + > +static const char *gen_eth_xmlnode(xmlNodePtr root, > + struct EthIfacesList *plist) > +{ > + const char *msg = NULL; > + int i = 0; > + struct EthIface *piface = NULL; > + > + while (i< plist->count) { > + piface = plist->pifaces[i]; > + i++; > + msg = gen_eth_xmlnode_iface(root, piface, plist, 0); > + if (msg != NULL) { > + goto out; > + } > + } > + > + out: > + return msg; > +} > + > +const char *EthIfaceListTOXML(char **ppxml, > + struct EthIfacesList *plist, > + int dump_all_flag) > +{ > + const char *msg = NULL; > + xmlNodePtr root = NULL; > + > + root = xmlNewNode(NULL, BAD_CAST "tmp"); > + if (root == NULL) { > + msg = "failed to create root node."; > + goto out; > + } > + msg = gen_eth_xmlnode(root, plist); > + if (msg == NULL) { > + if (dump_all_flag == 1) { > + *ppxml = tree_to_xml(root); > + } else { > + *ppxml = tree_to_xml(root->children); > + } > + } > + > + out: > + xmlFreeNode(root); > + return msg; > +} > + > +static int parse_eth_xmlnode(struct EthIfacesList *plist, xmlNode *inode, > + int status, char *attached) > +{ > + struct EthIface *piface = NULL; > + xmlNode *child1 = NULL, *child2 = NULL; > + char *temp = NULL, **ppchar; > + > + SAFE_MALLOC(piface, sizeof(struct EthIface)); > + eth_iface_init(piface); > + > + piface->name = get_attr_value(inode, "name"); > + piface->run_prop.status = status; > + if (attached != NULL) { > + piface->attach_bridge = strdup(attached); > + } > + temp = get_attr_value(inode, "type"); > + if (temp != NULL) { > + if (0 == strcmp(temp, "ethernet")) { By convention, we compare the function result with a value, in that order. Ex: strcmp(temp, "ethernet") == 0

> + piface->eth_type = ETH_TYPE_ETHER_ANY; > + } > + if (0 == strcmp(temp, "bridge")) { > + piface->eth_type = ETH_TYPE_ETHER_ANY | ETH_TYPE_ETHER_SUB_BRIDGE; > + } > + if (0 == strcmp(temp, "vlan")) { > + piface->eth_type = ETH_TYPE_ETHER_ANY | ETH_TYPE_ETHER_SUB_VLAN; > + } > + SAFE_FREE(temp); > + } > + > + for (child1 = inode->children; child1 != NULL; child1 = child1->next) { > + if (XSTREQ(child1->name, "start")) { > + temp = get_attr_value(child1, "mode"); > + if (0 == strcmp(temp, "onboot")) { > + piface->run_prop.boot_mode = BOOT_MODE_AUTOSTART; > + } > + if (0 == strcmp(temp, "none")) { > + piface->run_prop.boot_mode = BOOT_MODE_NONE; > + } > + SAFE_FREE(temp); > + } > + if (XSTREQ(child1->name, "mac")) { > + piface->mac = get_attr_value(child1, "address"); > + } > + if (XSTREQ(child1->name, "bridge")) { > + eth_iface_add_br_prop(piface); > + temp = get_attr_value(child1, "stp"); > + if (0 == strcmp(temp, "on")) { > + piface->pbr_prop->STP = 1; > + } > + if (0 == strcmp(temp, "off")) { > + piface->pbr_prop->STP = 0; > + } > + SAFE_FREE(temp); > + temp = get_attr_value(child1, "delay"); > + piface->pbr_prop->delay = strtol(temp, NULL, 10); > + SAFE_FREE(temp); > + } > + if (XSTREQ(child1->name, "bridge")) { > + eth_iface_add_br_prop(piface); > + temp = get_attr_value(child1, "stp"); > + if (0 == strcmp(temp, "on")) { > + piface->pbr_prop->STP = 1; > + } > + if (0 == strcmp(temp, "off")) { > + piface->pbr_prop->STP = 0; > + } > + SAFE_FREE(temp); > + temp = get_attr_value(child1, "delay"); > + piface->pbr_prop->delay = strtol(temp, NULL, 10); > + SAFE_FREE(temp); > + for (child2 = child1->children; child2 != NULL; > + child2 = child2->next) { > + if (XSTREQ(child2->name, "interface")) { > + if (piface->pbr_prop->port_names == NULL) { > + SAFE_CALLOC(piface->pbr_prop->port_names, > + MAX_IFACE_NUM, sizeof(char *)); > + piface->pbr_prop->port_num = 0; > + } > + ppchar = piface->pbr_prop->port_names + > + (piface->pbr_prop->port_num)++; > + *ppchar = get_attr_value(child2, "name"); > + parse_eth_xmlnode(plist, child2, status, piface->name); > + } > + } > + } > + if (XSTREQ(child1->name, "vlan")) { > + eth_iface_add_vlan_prop(piface, VLAN_TYPE_802_1_Q); > + temp = get_attr_value(child1, "tag"); > + piface->pvlan_prop->props.prop_8021q.vlan_id = > + strtol(temp, NULL, 10); > + SAFE_FREE(temp); > + for (child2 = child1->children; child2 != NULL; > + child2 = child2->next) { > + if (XSTREQ(child2->name, "interface")) { > + piface->pvlan_prop->props.prop_8021q.parent = > + get_attr_value(child2, "name"); > + piface->dep_ifname = > + get_attr_value(child2, "name"); > + } > + } > + } > + } > + > + eth_ifaceslist_add(plist,&piface); > + return 1; > +} > + > +static const char *XMLToEthIfaceList(struct EthIfacesList *plist, > + const char *xml, > + int status) > +{ > + xmlDoc *xmldoc; > + xmlXPathContext *xpathCtx; > + xmlXPathObject *xpathObj; > + xmlChar *xpathstr; > + xmlNode **dev_nodes = NULL; > + xmlNodeSet *nsv = NULL; > + int count = 0; > + int len, devidx; > + const char *msg = NULL; init everything possible > + > + len = strlen(xml) + 1; > + xpathstr = (xmlChar *)"/interface"; > + > + xmlSetGenericErrorFunc(NULL, swallow_err_msg); > + if ((xmldoc = xmlParseMemory(xml, len)) == NULL) { > + msg = "failed to get xmldoc."; > + goto err1; > + } > + > + if ((xpathCtx = xmlXPathNewContext(xmldoc)) == NULL) { > + msg = "failed to get pathCtx"; > + goto err2; > + } > + > + if ((xpathObj = xmlXPathEvalExpression(xpathstr, xpathCtx)) > + == NULL) { > + msg = "failed to get xpathObj"; > + goto err3; > + } > + > + nsv = xpathObj->nodesetval; > + if (nsv == NULL) { > + msg = "failed to get nodesetval."; > + goto out; > + } > + > + dev_nodes = nsv->nodeTab; > + count = nsv->nodeNr; > + > + if (count<= 0) { > + msg = "nodesetval have less that 1 values."; > + goto out; > + } > + > + for (devidx = 0; devidx< count; devidx++) { > + parse_eth_xmlnode(plist, dev_nodes[devidx], status, NULL); > + } > + > + out: > + xmlSetGenericErrorFunc(NULL, NULL); > + xmlXPathFreeObject(xpathObj); > + > + err3: > + xmlXPathFreeContext(xpathCtx); > + err2: > + xmlFreeDoc(xmldoc); > + err1: > + return msg; > +} > + > +CMPIStatus get_host_ifaces(struct EthIfacesList *plist, > + const CMPIBroker *broker, char *prefix) > +{ > + virConnectPtr conn = NULL; > + virInterfacePtr iface; > + CMPIStatus s = {CMPI_RC_ERR_FAILED, NULL}; > + int num, listnum, i; > + char **names = NULL; > + char *dump = NULL; > + int flags = 0; > + const char *msg; > + > + conn = connect_by_classname(broker, prefix,&s); > + if (conn == NULL) { > + RECORD_MSG(broker,&s, CMPI_RC_ERR_FAILED, "connect failed."); I think there is already a macro for this. CMReturn() and CIMReturnWithString(). See cmpimacs.h.

> + goto out; > + } > + > + /* list defined interfaces*/ > + num = virConnectNumOfDefinedInterfaces(conn); > + if (num< 0) { > + RECORD_MSG(broker,&s, CMPI_RC_ERR_FAILED, > + "failed to find number of defined interfaces."); > + goto out; > + } > + names = malloc(num * sizeof(char *)); > + listnum = virConnectListDefinedInterfaces(conn, names, num); > + if (listnum< 0) { > + RECORD_MSG(broker,&s, CMPI_RC_ERR_FAILED, > + "failed to list names of defined interfaces."); > + goto out; > + } > + CU_DEBUG("%d defined ifaces found from libvirt API.\n", listnum); > + > + flags |= VIR_INTERFACE_XML_INACTIVE; Good. We should be fetching the stored XML and not just that of the running VM. > + for (i = 0; i< listnum; i++) { > + iface = virInterfaceLookupByName(conn, names[i]); > + if (!iface) { > + CU_DEBUG("failed to look up %s.\n", names[i]); > + SAFE_FREE(names[i]); > + continue; > + } > + SAFE_FREE(names[i]); > + dump = virInterfaceGetXMLDesc(iface, flags); > + CU_DEBUG("defined interface %d xml:\n%s", i, dump); > + msg = XMLToEthIfaceList(plist, dump, 0); > + if (msg != NULL) { > + CU_DEBUG("failed parsing eth xml, msg is: %s.", msg); > + } > + SAFE_FREE(dump); > + virInterfaceFree(iface); > + } > + SAFE_FREE(names); I recently did some work and avoided using the existing get_devices() internal API to avoid excessive parsing. I'd like to re-factor the existing device parsing APIs to be more re-usable without the overhead. See my second comment in this post .

> + > + /* list active interfaces*/ > + num = virConnectNumOfInterfaces(conn); > + if (num< 0) { > + RECORD_MSG(broker,&s, CMPI_RC_ERR_FAILED, > + "failed to find number of active interfaces."); > + goto out; > + } > + names = malloc(num * sizeof(char *)); > + > + listnum = virConnectListInterfaces(conn, names, num); > + if (listnum< 0) { > + RECORD_MSG(broker,&s, CMPI_RC_ERR_FAILED, > + "failed to list names of active interfacess."); > + goto out; > + } > + CU_DEBUG("%d active ifaces found from libvirt API.\n", listnum); > + > + flags |= VIR_INTERFACE_XML_INACTIVE; > + for (i = 0; i< listnum; i++) { > + iface = virInterfaceLookupByName(conn, names[i]); > + if (!iface) { > + CU_DEBUG("failed to look up %s.\n", names[i]); > + SAFE_FREE(names[i]); > + continue; > + } > + SAFE_FREE(names[i]); > + dump = virInterfaceGetXMLDesc(iface, flags); Why not rename the dump variable to xml?

> + CU_DEBUG("active interface %d xml:\n%s", i, dump); Nit: In general, we've tried to capitalize the first letter of debug message so they are more like normal sentences. IMO, it makes reading logs a tiny bit easier. > + msg = XMLToEthIfaceList(plist, dump, 1); > + if (msg != NULL) { > + CU_DEBUG("failed parsing eth xml, msg is: %s.", msg); > + } > + SAFE_FREE(dump); > + virInterfaceFree(iface); > + } > + s.rc = CMPI_RC_OK; > + > + out: > + virConnectClose(conn); > + SAFE_FREE(names); > + return s; > +} > +/* > + * Local Variables: > + * mode: C > + * c-set-style: "K&R" > + * tab-width: 8 > + * c-basic-offset: 8 > + * indent-tabs-mode: nil > + * End: > + */ > diff --git a/libxkutil/network_parsing.h b/libxkutil/network_parsing.h > new file mode 100644 > index 0000000..bbfe729 > --- /dev/null > +++ b/libxkutil/network_parsing.h > @@ -0,0 +1,160 @@ > +/* > + * Copyright IBM Corp. 2012 > + * > + * Authors: > + * Wenchao Xia&lt;xiawenc(a)cn.ibm.com&gt; > + * > + * This library is free software; you can redistribute it and/or > + * modify it under the terms of the GNU Lesser General Public > + * License as published by the Free Software Foundation; either > + * version 2.1 of the License, or (at your option) any later version. > + */ > + > +#ifndef NETWORK_PARSING_H > +#define NETWORK_PARSING_H > + > +#include<cmpidt.h> > +#include<cmpift.h> > +#include<cmpimacs.h> > + > +/* value defines */ > +#define MAX_IFACE_NUM 4096 > + > +#define NUM_NOT_GOT -1 > + > +#define ETH_TYPE_BASE_MASK 0xff00 > +#define ETH_TYPE_SUB_MASK 0x00ff > + > +#define CMD_DEBUG_LEVEL 2 > + > +/* macro functions */ > +#define CMD_DEBUG(lvl, fmt, args...) do { \ > + if (CMD_DEBUG_LEVEL&& (lvl)<= CMD_DEBUG_LEVEL) { \ > + debug_print(fmt, ##args); \ > + } \ > +} while (0) I assume this exists to reduce noise in the current libvirt-cim log out put (CU_DEBUG), right? Perhaps should simply extend the current impl and add a CU_DEBUG_LEVEL? For now, I'm okay with this as-is.

> + > +#define SAFE_MALLOC(p, size) \ > +{ \ > + (p) = malloc((size)); \ > + if ((p) == NULL) { \ > + CU_DEBUG("malloc failed."); \ > + } \ > +} > + > +#define SAFE_CALLOC(p, nmen, size) \ > +{ \ > + (p) = calloc((nmen), (size)); \ > + if ((p) == NULL) { \ > + CU_DEBUG("calloc failed."); \ > + } \ > +} These are not really 'safe' since the program will likely crash once p is accessed. Rather, they just hide the return code checking. I know it's a lot of code, but I would prefer we put logic like these where the functions are called. Ideally, we'd all move to this sort of approach and handle any abort gracefully. Until, let's keep things simple (and verbose).

> + > +#define SAFE_FREE(p) {free(p); (p) = NULL; } > + > +#define RECORD_MSG(broker, ps, rc, fmt, args...) do { \ > + CU_DEBUG(fmt, ##args); \ > + if (broker) { \ > + cu_statusf((broker), (ps), (rc), fmt, ##args); \ > + } \ > +} while (0) Ah, you're macro takes a variable number of args. See virt_set_status is misc_util.h.

> + > +typedef enum { > + ETH_TYPE_NOT_GOT = 0x0000, The 'NOT_GOT' is hard to read, IMO. How about just using '_NONE'?

> + ETH_TYPE_ETHER_ANY = 0x0100, > + ETH_TYPE_ETHER_SUB_PHYSICAL = 0x0001, > + ETH_TYPE_ETHER_SUB_BRIDGE = 0x0002, > + ETH_TYPE_ETHER_SUB_VLAN = 0x0004 > +} EthType; > + > +typedef enum { > + VLAN_TYPE_NOT_GOT = NUM_NOT_GOT, > + VLAN_TYPE_802_1_Q = 1, > + VLAN_TYPE_802_1_QBG = 2, > + VLAN_TYPE_802_1_QBH = 4 > +} VLANType; > + > +typedef enum { > + BOOT_MODE_NOT_GOT = NUM_NOT_GOT, > + BOOT_MODE_AUTOSTART = 1, > + BOOT_MODE_NONE = 2 > +} BootMode; > + > +struct BR_Prop { > + int STP; > + int delay; > + char **port_names; > + int port_num; > +} ; > + > +struct Run_Prop { > + int status; > + BootMode boot_mode; > +} ; > + > +struct VLAN_Prop_8021q { > + int vlan_id; > + char *parent; > +} ; > + > +/* HP vlan standard, TBD */ > +struct VLAN_Prop_8021qbg { > + int invalid; > +} ; > + > +/* Cisco and VMware vlan standard, TBD */ > +struct VLAN_Prop_8021qbh { > + int invalid; > +} ; > + > +struct VLAN_Prop { > + int vlan_type; > + union { > + struct VLAN_Prop_8021q prop_8021q; > + struct VLAN_Prop_8021qbg prop_8021qbg; > + struct VLAN_Prop_8021qbh prop_8021qbh; > + } props; > +} ; > + > +/* EthIface is logical devices include eth ports and bridges */ > +struct EthIface { > + char *name; > + char *dep_ifname; /* parent dev name */ > + char *attach_bridge; /* bridge the iface is attached to */ > + char *mac; > + EthType eth_type; > + struct Run_Prop run_prop; > + /* optional properties */ > + struct BR_Prop *pbr_prop; > + struct VLAN_Prop *pvlan_prop; > +} ; > + > +struct EthIfacesList { > + struct EthIface *pifaces[MAX_IFACE_NUM]; > + int count; > +} ; > + > +void eth_iface_init(struct EthIface *piface); > +void eth_iface_add_br_prop(struct EthIface *piface); > +void eth_iface_add_vlan_prop(struct EthIface *piface, int vlan_type); > +void eth_iface_uninit(struct EthIface *piface); > + > +void eth_ifaceslist_init(struct EthIfacesList *plist); > +void eth_ifaceslist_uninit(struct EthIfacesList *plist); > +/* ppiface must be allocated from heap, to save code of struct duplication */ > +int eth_ifaceslist_add(struct EthIfacesList *plist, struct EthIface **ppiface); > +/* returned pointer is direct reference to a member in plist */ > +struct EthIface *eth_ifaceslist_search(struct EthIfacesList *plist, > + char *name); > + > +void eth_iface_print(struct EthIface *piface); > +void eth_ifaceslist_print(struct EthIfacesList *plist); > + > +CMPIStatus get_host_ifaces(struct EthIfacesList *plist, > + const CMPIBroker *broker, char *prefix); > + > +const char *EthIfaceListTOXML(char **ppxml, > + struct EthIfacesList *plist, > + int dump_all_flag); > + > +#endif > diff --git a/libxkutil/network_parsing_test.c b/libxkutil/network_parsing_test.c > new file mode 100644 > index 0000000..593bfd0 > --- /dev/null > +++ b/libxkutil/network_parsing_test.c Copyright?

> @@ -0,0 +1,61 @@ > +#include<stdio.h> > +#include<stdlib.h> > +#include<string.h> > +#include<pthread.h> > +#include<unistd.h> > +#include<time.h> > +#include<assert.h> > +#include<sys/time.h> > +#include<libvirt/libvirt.h> > +#include<libxml/tree.h> > +#include<libxml/parser.h> > +#include<libxml/xpath.h> > + > +#include<cmpidt.h> > +#include<cmpift.h> > +#include<cmpimacs.h> > + > +#include "misc_util.h" > +#include "device_parsing.h" > +#include "network_parsing.h" > + > +static const CMPIBroker *broker; > + > +static long print_and_ret_time_stamp(void) > +{ > + struct timeval tv; > + long ret; > + gettimeofday(&tv, NULL); > + ret = tv.tv_sec*1000 + tv.tv_usec/1000; > + CU_DEBUG("time is [%ld] ms.", ret); > + return ret; > +} > + > +/* try retrieve all information, and then map them back to xml. */ > +int main(int argc, char **argv) > +{ > + libvirt_cim_init(); > + struct EthIfacesList *plist = NULL; > + const char *msg = NULL; > + char *genxml = NULL; > + CMPIStatus s; > + long start_time, end_time; > + > + SAFE_MALLOC(plist, sizeof(struct EthIfacesList)); > + eth_ifaceslist_init(plist); > + > + start_time = print_and_ret_time_stamp(); > + s = get_host_ifaces(plist, broker, "kvm"); > + end_time = print_and_ret_time_stamp(); > + CU_DEBUG("cost [%d]ms in discovering host network. Result:", > + end_time - start_time); > + eth_ifaceslist_print(plist); > + > + msg = EthIfaceListTOXML(&genxml, plist, 1); > + CU_DEBUG("xml gen msg is %s. xml is:\n%s", msg, genxml); > + SAFE_FREE(genxml); > + > + eth_ifaceslist_uninit(plist); > + SAFE_FREE(plist); > + return 0; > +} > diff --git a/libxkutil/xmlgen.c b/libxkutil/xmlgen.c > index 9a2ada9..001246a 100644 > --- a/libxkutil/xmlgen.c > +++ b/libxkutil/xmlgen.c > @@ -830,7 +830,7 @@ static char *features_xml(xmlNodePtr root, struct domain *domain) > return NULL; > } > > -static char *tree_to_xml(xmlNodePtr root) > +char *tree_to_xml(xmlNodePtr root) > { > xmlBufferPtr buffer = NULL; > xmlSaveCtxtPtr savectx = NULL; > diff --git a/libxkutil/xmlgen.h b/libxkutil/xmlgen.h > index 743fc82..5d21a94 100644 > --- a/libxkutil/xmlgen.h > +++ b/libxkutil/xmlgen.h > @@ -32,6 +32,8 @@ struct kv { > const char *val; > }; > > +char *tree_to_xml(xmlNodePtr root); > + Good re-use! > char *system_to_xml(struct domain *dominfo); > char *device_to_xml(struct virt_device *dev); >