[Libvirt-cim] [PATCH v5] vlan extention - add function library for readonly usage
xiaxia347work
xiaxia347work at 163.com
Fri Jan 27 13:53:26 UTC 2012
于 2012-1-26 7:47, Chip Vincent 写道:
> 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.
>
I think the problem is caused by loader by default to load libxkutil.so in /usr/lib64 which need to be updated to contain the new binary code.
> $ 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 at 163.com wrote:
>> From: Wenchao Xia<xiawenc at linux.vnet.ibm.com>
>>
>> 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<xiawenc at linux.vnet.ibm.com>
>> ---
>> 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 at .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<xiawenc at cn.ibm.com>
>> + *
>> + * 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?
ok, let the codes share one define.
>> +
>> +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
sorry missed it.
>> + 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?
ok.
>> + 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
ok.
>> + 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/
I tried the netcf in virt-manager, if one eth is attached to a bridge, then the eth have no "standalone"
xml, but a embbed one in the bridge. it is something like following:
<bridge br1>
<interface name=eth0.11>
>> + 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
>
ok.
>> + 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.
the purpose of this macro is to allow broker to be set to NULL which would happen in the test program.
If you wish I would remove this macro and write "if (broker != NULL)" directly.
>> + 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 .
Do you mean that the API should be able to list active or inactive ones according to another parameter?
>> +
>> + /* 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?
right.
>> + 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<xiawenc at cn.ibm.com>
>> + *
>> + * 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.
>
yes, the macro is made to allow filter out some debug message and more it does not contain a head like "*.c:556 " so some printing could be more formated.I'd like to rename this macro to
CU_DEBUG_OPTIONAL.
>> +
>> +#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).
The macro did not do as its name. My thought is that if malloc fails, it have a record in debug file, and in future it can acts like a hook, and we can remove the checking of the code in the macro if
code seems too much.
>> +
>> +#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.
virt_set_status is a function, what could I do to the macro according to that?
>> +
>> +typedef enum {
>> + ETH_TYPE_NOT_GOT = 0x0000,
> The 'NOT_GOT' is hard to read, IMO. How about just using '_NONE'?
OK.
>> + 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?
sorry, forgot that.
>> @@ -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);
>>
>
>
More information about the Libvirt-cim
mailing list