On 11/19/2012 11:51 AM, Michal Privoznik wrote:
> Currently, we are only keeping a inactive XML configuration
> in status dir. This is no longer enough as we need to keep
> this class_id attribute so we don't overwrite old entries
> when the daemon restarts.
Aha! So you're looking at solving the problem in a different way - save
everything to the status file rather than recomputing it as you restart.
While I like the idea of having the network status file hold this
information, I think its reliability is suspect. What if a guest's
process is terminated while libvirtd isn't running? Or what if libvirtd
exits unexpectedly after the commands to setup bandwidth have been
executed, but before the new network state file has been written (or
vice versa, depending on the code).
Also, networks aren't properly shut down when the host is being shutdown
(there's no equivalent to the libvirt-guests service, although at least
one person a month or two ago requested it).
If everybody agrees that saving this info to a file and re-reading it on
start up is reliable, though, then we might as well do the same thing
with the device pool (although it's currently a bit different - the
inuse count is stored in the virNetworkDef rather than Obj, and is
reported during net-dumpxml)
> However, since there has already
> been a libvirt release
*a* libvirt release? :-)
> which has just <network/> as root element,
> and we want to keep things compatible, detect that loaded
> status file is older one, and don't scream about it.
> ---
> src/conf/network_conf.c | 199 ++++++++++++++++++++++++++++++++++---------
> src/conf/network_conf.h | 2 +
> src/libvirt_private.syms | 1 +
> src/network/bridge_driver.c | 9 +--
> 4 files changed, 165 insertions(+), 46 deletions(-)
>
> diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
> index 80189e6..9c2e4d4 100644
> --- a/src/conf/network_conf.c
> +++ b/src/conf/network_conf.c
> @@ -1645,6 +1645,78 @@ cleanup:
> return def;
> }
>
> +int
> +virNetworkObjUpdateParseFile(const char *filename,
> + virNetworkObjPtr net)
> +{
> + int ret = -1;
> + xmlDocPtr xml = NULL;
> + xmlNodePtr node = NULL;
> + virNetworkDefPtr tmp = NULL;
> + xmlXPathContextPtr ctxt = NULL;
> +
> + xml = virXMLParse(filename, NULL, _("(network status)"));
> + if (!xml)
> + return -1;
> +
> + ctxt = xmlXPathNewContext(xml);
> + if (ctxt == NULL) {
> + virReportOOMError();
> + goto cleanup;
> + }
> +
> + node = xmlDocGetRootElement(xml);
> + if (xmlStrEqual(node->name, BAD_CAST "networkstatus")) {
> + /* Newer network status file. Contains useful
> + * info which are not to be found in bare config XML */
> + char *class_id = NULL;
> + char *floor_sum = NULL;
> +
> + ctxt->node = node;
> + class_id = virXPathString("string(./class_id[1]/@next)", ctxt);
> + if (class_id &&
> + virStrToLong_ui(class_id, NULL, 10, &net->class_id) < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Malformed 'class_id' attribute: %s"),
> + class_id);
> + VIR_FREE(class_id);
> + goto cleanup;
> + }
> + VIR_FREE(class_id);
> +
> + floor_sum = virXPathString("string(./floor[1]/@sum)", ctxt);
> + if (floor_sum &&
> + virStrToLong_ull(floor_sum, NULL, 10, &net->floor_sum) < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Malformed 'floor_sum' attribute: %s"),
> + floor_sum);
> + VIR_FREE(floor_sum);
> + }
> + VIR_FREE(floor_sum);
> + }
> +
> + node = virXPathNode("//network", ctxt);
> + if (!node) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("Could not find any 'network' element"));
> + goto cleanup;
> + }
> +
> + ctxt->node = node;
> + tmp = virNetworkDefParseXML(ctxt);
> +
> + if (tmp) {
> + net->newDef = net->def;
> + net->def = tmp;
> + }
> +
> + ret = 0;
> +
> +cleanup:
> + xmlXPathFreeContext(ctxt);
> + return ret;
> +}
> +
> static int
> virNetworkDNSDefFormat(virBufferPtr buf,
> virNetworkDNSDefPtr def)
> @@ -1823,24 +1895,26 @@ virPortGroupDefFormat(virBufferPtr buf,
> return 0;
> }
>
> -char *virNetworkDefFormat(const virNetworkDefPtr def, unsigned int flags)
> +static int
> +virNetworkDefFormatInternal(virBufferPtr buf,
> + const virNetworkDefPtr def,
> + unsigned int flags)
> {
> - virBuffer buf = VIR_BUFFER_INITIALIZER;
> unsigned char *uuid;
> char uuidstr[VIR_UUID_STRING_BUFLEN];
> int ii;
>
> - virBufferAddLit(&buf, "<network");
> + virBufferAddLit(buf, "<network");
> if (!(flags & VIR_NETWORK_XML_INACTIVE) && (def->connections > 0)) {
> - virBufferAsprintf(&buf, " connections='%d'", def->connections);
> + virBufferAsprintf(buf, " connections='%d'", def->connections);
> }
> - virBufferAddLit(&buf, ">\n");
> - virBufferAdjustIndent(&buf, 2);
> - virBufferEscapeString(&buf, "<name>%s</name>\n", def->name);
> + virBufferAddLit(buf, ">\n");
> + virBufferAdjustIndent(buf, 2);
> + virBufferEscapeString(buf, "<name>%s</name>\n", def->name);
>
> uuid = def->uuid;
> virUUIDFormat(uuid, uuidstr);
> - virBufferAsprintf(&buf, "<uuid>%s</uuid>\n", uuidstr);
> + virBufferAsprintf(buf, "<uuid>%s</uuid>\n", uuidstr);
>
> if (def->forwardType != VIR_NETWORK_FORWARD_NONE) {
> const char *dev = NULL;
> @@ -1854,40 +1928,40 @@ char *virNetworkDefFormat(const virNetworkDefPtr def, unsigned int flags)
> def->forwardType, def->name);
> goto error;
> }
> - virBufferAddLit(&buf, "<forward");
> - virBufferEscapeString(&buf, " dev='%s'", dev);
> - virBufferAsprintf(&buf, " mode='%s'", mode);
> + virBufferAddLit(buf, "<forward");
> + virBufferEscapeString(buf, " dev='%s'", dev);
> + virBufferAsprintf(buf, " mode='%s'", mode);
> if (def->forwardType == VIR_NETWORK_FORWARD_HOSTDEV) {
> if (def->managed == 1)
> - virBufferAddLit(&buf, " managed='yes'");
> + virBufferAddLit(buf, " managed='yes'");
> else
> - virBufferAddLit(&buf, " managed='no'");
> + virBufferAddLit(buf, " managed='no'");
> }
> - virBufferAsprintf(&buf, "%s>\n",
> + virBufferAsprintf(buf, "%s>\n",
> (def->nForwardIfs || def->nForwardPfs) ? "" : "/");
> - virBufferAdjustIndent(&buf, 2);
> + virBufferAdjustIndent(buf, 2);
>
> /* For now, hard-coded to at most 1 forwardPfs */
> if (def->nForwardPfs)
> - virBufferEscapeString(&buf, "<pf dev='%s'/>\n",
> + virBufferEscapeString(buf, "<pf dev='%s'/>\n",
> def->forwardPfs[0].dev);
>
> if (def->nForwardIfs &&
> (!def->nForwardPfs || !(flags & VIR_NETWORK_XML_INACTIVE))) {
> for (ii = 0; ii < def->nForwardIfs; ii++) {
> if (def->forwardType != VIR_NETWORK_FORWARD_HOSTDEV) {
> - virBufferEscapeString(&buf, "<interface dev='%s'",
> + virBufferEscapeString(buf, "<interface dev='%s'",
> def->forwardIfs[ii].device.dev);
> if (!(flags & VIR_NETWORK_XML_INACTIVE) &&
> (def->forwardIfs[ii].connections > 0)) {
> - virBufferAsprintf(&buf, " connections='%d'",
> + virBufferAsprintf(buf, " connections='%d'",
> def->forwardIfs[ii].connections);
> }
> - virBufferAddLit(&buf, "/>\n");
> + virBufferAddLit(buf, "/>\n");
> }
> else {
> if (def->forwardIfs[ii].type == VIR_NETWORK_FORWARD_HOSTDEV_DEVICE_PCI) {
> - if (virDevicePCIAddressFormat(&buf,
> + if (virDevicePCIAddressFormat(buf,
> def->forwardIfs[ii].device.pci,
> true) < 0)
> goto error;
> @@ -1895,67 +1969,111 @@ char *virNetworkDefFormat(const virNetworkDefPtr def, unsigned int flags)
> }
> }
> }
> - virBufferAdjustIndent(&buf, -2);
> + virBufferAdjustIndent(buf, -2);
> if (def->nForwardPfs || def->nForwardIfs)
> - virBufferAddLit(&buf, "</forward>\n");
> + virBufferAddLit(buf, "</forward>\n");
> }
>
> if (def->forwardType == VIR_NETWORK_FORWARD_NONE ||
> - def->forwardType == VIR_NETWORK_FORWARD_NAT ||
> - def->forwardType == VIR_NETWORK_FORWARD_ROUTE) {
> + def->forwardType == VIR_NETWORK_FORWARD_NAT ||
> + def->forwardType == VIR_NETWORK_FORWARD_ROUTE) {
>
> - virBufferAddLit(&buf, "<bridge");
> + virBufferAddLit(buf, "<bridge");
> if (def->bridge)
> - virBufferEscapeString(&buf, " name='%s'", def->bridge);
> - virBufferAsprintf(&buf, " stp='%s' delay='%ld' />\n",
> + virBufferEscapeString(buf, " name='%s'", def->bridge);
> + virBufferAsprintf(buf, " stp='%s' delay='%ld' />\n",
> def->stp ? "on" : "off",
> def->delay);
> } else if (def->forwardType == VIR_NETWORK_FORWARD_BRIDGE &&
> def->bridge) {
> - virBufferEscapeString(&buf, "<bridge name='%s' />\n", def->bridge);
> + virBufferEscapeString(buf, "<bridge name='%s' />\n", def->bridge);
> }
>
>
> if (def->mac_specified) {
> char macaddr[VIR_MAC_STRING_BUFLEN];
> virMacAddrFormat(&def->mac, macaddr);
> - virBufferAsprintf(&buf, "<mac address='%s'/>\n", macaddr);
> + virBufferAsprintf(buf, "<mac address='%s'/>\n", macaddr);
> }
>
> if (def->domain)
> - virBufferAsprintf(&buf, "<domain name='%s'/>\n", def->domain);
> + virBufferAsprintf(buf, "<domain name='%s'/>\n", def->domain);
>
> - if (virNetworkDNSDefFormat(&buf, def->dns) < 0)
> + if (virNetworkDNSDefFormat(buf, def->dns) < 0)
> goto error;
>
> - if (virNetDevVlanFormat(&def->vlan, &buf) < 0)
> + if (virNetDevVlanFormat(&def->vlan, buf) < 0)
> goto error;
> - if (virNetDevBandwidthFormat(def->bandwidth, &buf) < 0)
> + if (virNetDevBandwidthFormat(def->bandwidth, buf) < 0)
> goto error;
>
> for (ii = 0; ii < def->nips; ii++) {
> - if (virNetworkIpDefFormat(&buf, &def->ips[ii]) < 0)
> + if (virNetworkIpDefFormat(buf, &def->ips[ii]) < 0)
> goto error;
> }
>
> - if (virNetDevVPortProfileFormat(def->virtPortProfile, &buf) < 0)
> + if (virNetDevVPortProfileFormat(def->virtPortProfile, buf) < 0)
> goto error;
>
> for (ii = 0; ii < def->nPortGroups; ii++)
> - if (virPortGroupDefFormat(&buf, &def->portGroups[ii]) < 0)
> + if (virPortGroupDefFormat(buf, &def->portGroups[ii]) < 0)
> goto error;
>
> + virBufferAdjustIndent(buf, -2);
> + virBufferAddLit(buf, "</network>\n");
> +
> + return 0;
> +
> +error:
> + return -1;
> +}
> +
> +char *
> +virNetworkDefFormat(virNetworkDefPtr def,
> + unsigned int flags)
> +{
> + virBuffer buf = VIR_BUFFER_INITIALIZER;
> +
> + if (virNetworkDefFormatInternal(&buf, def, flags) < 0)
> + goto error;
> +
> + if (virBufferError(&buf))
> + goto no_memory;
> +
> + return virBufferContentAndReset(&buf);
> +
> +no_memory:
> + virReportOOMError();
> +error:
> + virBufferFreeAndReset(&buf);
> + return NULL;
> +}
> +
> +static char *
> +virNetworkObjFormat(virNetworkObjPtr net,
> + unsigned int flags)
> +{
> + virBuffer buf = VIR_BUFFER_INITIALIZER;
> +
> + virBufferAddLit(&buf, "<networkstatus>\n");
> + virBufferAsprintf(&buf, " <class_id next='%u'/>\n", net->class_id);
> + virBufferAsprintf(&buf, " <floor sum='%llu'/>\n", net->floor_sum);
> +
> + virBufferAdjustIndent(&buf, 2);
> + if (virNetworkDefFormatInternal(&buf, net->def, flags) < 0)
> + goto error;
> +
> virBufferAdjustIndent(&buf, -2);
> - virBufferAddLit(&buf, "</network>\n");
> + virBufferAddLit(&buf, "</networkstatus>");
>
> if (virBufferError(&buf))
> goto no_memory;
>
> return virBufferContentAndReset(&buf);
>
> - no_memory:
> +no_memory:
> virReportOOMError();
> - error:
> +error:
> virBufferFreeAndReset(&buf);
> return NULL;
> }
> @@ -2027,9 +2145,10 @@ int virNetworkSaveStatus(const char *statusDir,
> virNetworkObjPtr network)
> {
> int ret = -1;
> + int flags = 0;
> char *xml;
>
> - if (!(xml = virNetworkDefFormat(network->def, 0)))
> + if (!(xml = virNetworkObjFormat(network, flags)))
> goto cleanup;
>
> if (virNetworkSaveXML(statusDir, network->def, xml))
> diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h
> index 8a8d1e4..efa9dea 100644
> --- a/src/conf/network_conf.h
> +++ b/src/conf/network_conf.h
> @@ -270,6 +270,8 @@ virNetworkDefPtr virNetworkDefParseString(const char *xmlStr);
> virNetworkDefPtr virNetworkDefParseFile(const char *filename);
> virNetworkDefPtr virNetworkDefParseNode(xmlDocPtr xml,
> xmlNodePtr root);
> +int virNetworkObjUpdateParseFile(const char *filename,
> + virNetworkObjPtr net);
>
> char *virNetworkDefFormat(const virNetworkDefPtr def, unsigned int flags);
>
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 5a07139..3ed0c97 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -858,6 +858,7 @@ virNetworkObjSetDefTransient;
> virNetworkObjUnlock;
> virNetworkObjUnsetDefTransient;
> virNetworkObjUpdate;
> +virNetworkObjUpdateParseFile;
> virNetworkRemoveInactive;
> virNetworkSaveConfig;
> virNetworkSaveStatus;
> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
> index 44fa56e..8dc9d19 100644
> --- a/src/network/bridge_driver.c
> +++ b/src/network/bridge_driver.c
> @@ -236,7 +236,6 @@ networkFindActiveConfigs(struct network_driver *driver) {
>
> for (i = 0 ; i < driver->networks.count ; i++) {
> virNetworkObjPtr obj = driver->networks.objs[i];
> - virNetworkDefPtr tmp;
> char *config;
>
> virNetworkObjLock(obj);
> @@ -254,12 +253,10 @@ networkFindActiveConfigs(struct network_driver *driver) {
> }
>
> /* Try and load the live config */
> - tmp = virNetworkDefParseFile(config);
> + if (virNetworkObjUpdateParseFile(config, obj) < 0)
> + VIR_WARN("Unable to update config of '%s' network",
> + obj->def->name);
> VIR_FREE(config);
> - if (tmp) {
> - obj->newDef = obj->def;
> - obj->def = tmp;
> - }
>
> /* If bridge exists, then mark it active */
> if (obj->def->bridge &&
ACK.
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list