Thanks for your inputs Daniel, please see inline.


> Date: Mon, 9 Jul 2012 09:21:36 +0100
> From: berrange@redhat.com
> To: ata.husain@hotmail.com
> CC: libvirt-list@redhat.com
> Subject: Re: [libvirt] [PATCH] ESX: interface driver routines
>
> On Sun, Jul 08, 2012 at 08:52:01AM +0530, Ata Bohra wrote:
> > Attached patch adds missing routines to esx interface driver, one of
> > the missing routine is esxInterfaceDefineXML. I am currently working
> > on it and will update patch soon. Also, patch addresses some of the
> > changes needed in esx_vi_generator.py and esx_vi_types.c to deserialize
> > "String List" types.
>
> Thanks for taking the time todo all this work !
>
> > +/**
> > + * Generates native XML descritpor for a given interface.
> > + * For instance:
> > + * <interface type="bridge" name="%s">
> > + * <start mode="onboot"/>"
> > + * <mtu size="%d"/>"
> > + * <mac address="%s"/>
> > + * <protocol family="ipv4">
> > + * <dhcp/>
> > + * <ip address="%s" prefix="%d"/>
> > + * <route gateway="%s"/>
> > + * </protocol>
> > + * <bridge stp="off">
> > + * <interface type="ethernet" name="%s">
> > + * <mac address="%s"/>
> > + * </interface>
> > + * </bridge>
> > + * </interface>
> > + */
> > +static char*
> > +esxGetNativeInterfaceXMLDesc(const esxVI_HostVirtualNic *virtualNic,
> > + const esxVI_HostIpRouteConfig *ipRouteConfig,
> > + const esxVI_PhysicalNic *physicalNicList,
> > + const unsigned int flags)
> > +{
> > + const esxVI_PhysicalNic *physicalNic = NULL;
> > + xmlDocPtr doc = NULL;
> > + xmlNodePtr root = NULL;
> > + xmlNodePtr startNode = NULL;
> > + xmlNodePtr mtuNode = NULL;
> > + xmlNodePtr protocolNode = NULL;
> > + xmlNodePtr bridgeNode = NULL;
> > + xmlNodePtr dhcpNode = NULL;
> > + xmlChar *xmlbuff = NULL;
> > + int use_static = 0;
> > + struct in_addr addr;
> > + uint32_t host_addr = 0;
> > + int zero_count = 0;
> > + int masklen = 0;
> > + int i = 0;
> > + virBuffer item = VIR_BUFFER_INITIALIZER;
> > + int bufferSize = 0;
> > + char *ret = NULL;
> > +
> > + if (VIR_INTERFACE_XML_INACTIVE & flags) {
> > + use_static = 1;
> > + }
> > +
> > + doc = xmlNewDoc(XML_CAST "1.0");
> > + root = xmlNewDocNode(doc, NULL, XML_CAST "interface", NULL);
>
> You must not construct XML documents manually like this. We have
> an internal data structure for representing the inteface configuration
> that you should populate. This has an API for generating the correct
> formed XML document. See the structs & APIs in src/conf/interface_conf.h
>
> The same applies for parsing XML too - use the inteface APIs.
[AB]: I am using the predefined API provided by src/conf/interface_conf.h/c to generate the libvirt domain XML returned from this function (virInterfaceDefParseString). Analyzing the interface_conf.h, there are couple of ways to get interfaceDefPtr: virInterfaceDefParseString, virInterfaceDefParseFile or virInterfaceDefParseNode. So instead of preparing the native XML by using virBuffer technique I prepared XML document which is then given to virInterfaceDefParseString() to generate virInterfaceDef object and further used to generate libvirt domain interface XML file (virInterfaceDefFormat). While coding I found the XML one easy to read and follow. Please correct me if wrong.
>
> > +
> > + xmlNewProp(root, XML_CAST "type", XML_CAST "bridge");
> > + xmlNewProp(root, XML_CAST "name", XML_CAST virtualNic->device);
> > + xmlDocSetRootElement(doc, root);
> > +
> > + /* define boot start mode */
> > + startNode = xmlNewChild(root, NULL, XML_CAST "start", NULL);
> > + xmlNewProp(startNode, XML_CAST "mode", XML_CAST "onboot");
> > +
> > + /* append mtu value */
> > + mtuNode = xmlNewChild(root, NULL, XML_CAST "mtu", NULL);
> > + virBufferAsprintf(&item, "%d",virtualNic->spec->mtu &&
> > + virtualNic->spec->mtu->value ?
> > + virtualNic->spec->mtu->value :
> > + 1500);
> > + const char *mtustr = virBufferContentAndReset(&item);
> > + if (mtustr == NULL) {
> > + virReportOOMError();
> > + goto cleanup;
> > + }
> > + xmlNewProp(mtuNode, XML_CAST "size", XML_CAST mtustr);
> > +
> > + /* append mac address field */
> > + if (!use_static && virtualNic->spec->mac) {
> > + xmlNodePtr mac_node = xmlNewChild(root, NULL, XML_CAST "mac", NULL);
> > + xmlNewProp(mac_node, XML_CAST "address",
> > + XML_CAST virtualNic->spec->mac);
> > + }
> > +
> > + /* TODO - Handle VLAN (via portgroup?) */
> > + if (virtualNic->spec->ip->subnetMask &&
> > + *virtualNic->spec->ip->subnetMask &&
> > + inet_aton(virtualNic->spec->ip->subnetMask, &addr) == 0) {
> > + ESX_ERROR(VIR_ERR_INTERNAL_ERROR, "%s",
> > + _("Error parsing netmask"));
> > + goto cleanup;
> > + }
> > +
> > + host_addr = ntohl(addr.s_addr);
> > + /* Calculate masklen */
> > + for (i = 0; i < 32; ++i) {
> > + if (host_addr & 0x01) {
> > + break;
> > + }
> > + zero_count++;
> > + host_addr >>= 1;
> > + }
> > + masklen = 32 - zero_count;
> > +
> > + /* append protocol field */
> > + /* TODO - Add IPv6 Support */
> > + protocolNode = xmlNewChild(root, NULL, XML_CAST "protocol", NULL);
> > + xmlNewProp(protocolNode, XML_CAST "family", XML_CAST "ipv4");
> > + if (virtualNic->spec->ip->dhcp == 1) {
> > + dhcpNode = xmlNewChild(protocolNode, NULL, XML_CAST "dhcp", NULL);
> > + /* avoids compiler warning */
> > + VIR_DEBUG("dhcpNode name: %s", (char *)dhcpNode->name);
> > + }
> > +
> > + if (virtualNic->spec->ip->dhcp != 1 || !use_static) {
> > + if (virtualNic->spec->ip->ipAddress &&
> > + *virtualNic->spec->ip->ipAddress) {
> > + xmlNodePtr ipAddrNode =
> > + xmlNewChild(protocolNode, NULL, XML_CAST "ip", NULL);
> > + xmlNewProp(ipAddrNode, XML_CAST "address", XML_CAST
> > + virtualNic->spec->ip->ipAddress);
> > +
> > + virBufferAsprintf(&item, "%d", masklen);
> > + const char *maskstr = virBufferContentAndReset(&item);
> > + if (maskstr == NULL) {
> > + virReportOOMError();
> > + goto cleanup;
> > + }
> > + xmlNewProp(ipAddrNode, XML_CAST "prefix", XML_CAST maskstr);
> > +
> > + xmlNodePtr routeNode =
> > + xmlNewChild(protocolNode, NULL, XML_CAST "route", NULL);
> > + xmlNewProp(routeNode, XML_CAST "gateway",
> > + XML_CAST ipRouteConfig->defaultGateway);
> > + }
> > + }
> > +
> > + /* Add bridge information */
> > + bridgeNode = xmlNewChild(root, NULL, XML_CAST "bridge", NULL);
> > + xmlNewProp(bridgeNode, XML_CAST "stp", XML_CAST "off");
> > +
> > + for (physicalNic = physicalNicList;
> > + physicalNic != NULL;
> > + physicalNic = physicalNic->_next) {
> > + xmlNodePtr bridgeIfaceNode =
> > + xmlNewChild(bridgeNode, NULL, XML_CAST "interface", NULL);
> > + xmlNewProp(bridgeIfaceNode, XML_CAST "type", XML_CAST "ethernet");
> > + xmlNewProp(bridgeIfaceNode, XML_CAST "name",
> > + XML_CAST physicalNic->device);
> > +
> > + xmlNodePtr bridgeIfaceMacNode =
> > + xmlNewChild(bridgeIfaceNode, NULL, XML_CAST "mac", NULL);
> > + xmlNewProp(bridgeIfaceMacNode, XML_CAST "address",
> > + XML_CAST physicalNic->mac);
> > + }
> > +
> > + xmlDocDumpFormatMemory(doc, &xmlbuff, &bufferSize, 1);
> > + if (xmlbuff == NULL) {
> > + virReportOOMError();
> > + goto cleanup;
> > + }
> > +
> > + ret = strdup((char *)xmlbuff);
> > + if (ret == NULL) {
> > + virReportOOMError();
> > + goto cleanup;
> > + }
> > +
> > + cleanup:
> > + VIR_FREE(mtustr);
> > + if (xmlbuff != NULL) {
> > + xmlFree(xmlbuff);
> > + }
> > + xmlFreeDoc(doc);
> > +
> > + return ret;
> > +}
> > +static int
>
>
> Regards,
> Daniel
> --
> |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
> |: http://libvirt.org -o- http://virt-manager.org :|
> |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
> |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

Thanks!
Ata