On 10/25/2012 11:18 AM, Peter Krempa wrote:

When there's no new definition the helper overwrote the old one with NULL. --- src/conf/network_conf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 8976f2a..93d1b4c 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -359,7 +359,7 @@ virNetworkObjSetDefTransient(virNetworkObjPtr network, bool live) void virNetworkObjUnsetDefTransient(virNetworkObjPtr network) { - if (network->def) { + if (network->newDef) { virNetworkDefFree(network->def); network->def = network->newDef; network->newDef = NULL;

Yes. ACK. (/raises hand acknowledging the screwup) BTW, although the change is correct, it currently is a NOP: 1) in the case of a persistent network, but def and newDef are always non-NULL when this is called, 2) in the case of a transient network, when this is called, the very next thing that happens is always to delete the entire networkObj.

On 10/25/2012 04:39 PM, Peter Krempa wrote:

On 10/25/12 22:34, Laine Stump wrote:

...

On 10/25/2012 11:18 AM, Peter Krempa wrote:

...

When there's no new definition the helper overwrote the old one with NULL. --- src/conf/network_conf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 8976f2a..93d1b4c 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -359,7 +359,7 @@ virNetworkObjSetDefTransient(virNetworkObjPtr network, bool live) void virNetworkObjUnsetDefTransient(virNetworkObjPtr network) { - if (network->def) { + if (network->newDef) { virNetworkDefFree(network->def); network->def = network->newDef; network->newDef = NULL;

Yes. ACK. (/raises hand acknowledging the screwup)

Thanks.

...

BTW, although the change is correct, it currently is a NOP:

1) in the case of a persistent network, but def and newDef are always non-NULL when this is called, 2) in the case of a transient network, when this is called, the very next thing that happens is always to delete the entire networkObj.

In a patch 4/6 of this series, I added code that accesses the definition right before tne network object is discarded. This caused segfault of the daemon when I tried to get rid of the dnsmasq and radvd status files when removing a network completely.

Aha!

On 10/25/2012 11:18 AM, Peter Krempa wrote:

The argument check_active is used only as a boolean so this patch changes the type and updates callers. --- src/conf/network_conf.c | 2 +- src/conf/network_conf.h | 2 +- src/network/bridge_driver.c | 4 ++-- 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 93d1b4c..8a3cc75 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -3056,7 +3056,7 @@ cleanup: int virNetworkObjIsDuplicate(virNetworkObjListPtr doms, virNetworkDefPtr def, - unsigned int check_active) + bool check_active) { int ret = -1; int dupVM = 0; diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h index 4c0c8c1..3e46304 100644 --- a/src/conf/network_conf.h +++ b/src/conf/network_conf.h @@ -337,7 +337,7 @@ virNetworkObjUpdate(virNetworkObjPtr obj,

int virNetworkObjIsDuplicate(virNetworkObjListPtr doms, virNetworkDefPtr def, - unsigned int check_active); + bool check_active);

void virNetworkObjLock(virNetworkObjPtr obj); void virNetworkObjUnlock(virNetworkObjPtr obj); diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 8837843..43a5540 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -2672,7 +2672,7 @@ static virNetworkPtr networkCreate(virConnectPtr conn, const char *xml) { if (!(def = virNetworkDefParseString(xml))) goto cleanup;

- if (virNetworkObjIsDuplicate(&driver->networks, def, 1) < 0) + if (virNetworkObjIsDuplicate(&driver->networks, def, true) < 0) goto cleanup;

/* Only the three L3 network types that are configured by libvirt @@ -2731,7 +2731,7 @@ static virNetworkPtr networkDefine(virConnectPtr conn, const char *xml) { if (!(def = virNetworkDefParseString(xml))) goto cleanup;

- if (virNetworkObjIsDuplicate(&driver->networks, def, 0) < 0) + if (virNetworkObjIsDuplicate(&driver->networks, def, false) < 0) goto cleanup;

/* Only the three L3 network types that are configured by libvirt

Trivial. ACK.

On 10/25/2012 11:18 AM, Peter Krempa wrote:

The hosts file was created in the network definition function. This patch moves the place the file is being created to the point where dnsmasq is being started. --- src/network/bridge_driver.c | 24 ++++++++++++++---------- 1 file changed, 14 insertions(+), 10 deletions(-)

diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 43a5540..976c132 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -846,6 +846,8 @@ networkStartDhcpDaemon(virNetworkObjPtr network) char *pidfile = NULL; int ret = -1; dnsmasqContext *dctx = NULL; + virNetworkIpDefPtr ipdef; + int i;

if (!virNetworkDefGetIpByIndex(network->def, AF_UNSPEC, 0)) { /* no IPv6 addresses, so we don't need to run radvd */ @@ -886,6 +888,18 @@ networkStartDhcpDaemon(virNetworkObjPtr network) if (ret < 0) goto cleanup;

+ /* populate dnsmasq hosts file */ + for (i = 0; (ipdef = virNetworkDefGetIpByIndex(network->def, AF_UNSPEC, i)); i++) { + if (VIR_SOCKET_ADDR_IS_FAMILY(&ipdef->address, AF_INET) && + (ipdef->nranges || ipdef->nhosts)) { + if (networkBuildDnsmasqHostsfile(dctx, ipdef, + network->def->dns) < 0) + goto cleanup; + + break; + } + } + ret = dnsmasqSave(dctx); if (ret < 0) goto cleanup; @@ -2724,7 +2738,6 @@ static virNetworkPtr networkDefine(virConnectPtr conn, const char *xml) { virNetworkObjPtr network = NULL; virNetworkPtr ret = NULL; int ii; - dnsmasqContext* dctx = NULL;

networkDriverLock(driver);

@@ -2781,21 +2794,12 @@ static virNetworkPtr networkDefine(virConnectPtr conn, const char *xml) { goto cleanup; }

- if (ipv4def) { - dctx = dnsmasqContextNew(def->name, DNSMASQ_STATE_DIR); - if (dctx == NULL || - networkBuildDnsmasqHostsfile(dctx, ipv4def, def->dns) < 0 || - dnsmasqSave(dctx) < 0) - goto cleanup; - } - VIR_INFO("Defining network '%s'", def->name); ret = virGetNetwork(conn, def->name, def->uuid);

cleanup: if (freeDef) virNetworkDefFree(def); - dnsmasqContextFree(dctx); if (network) virNetworkObjUnlock(network); networkDriverUnlock(driver);

Makes sense to me. ACK.

On 10/25/2012 11:18 AM, Peter Krempa wrote:

The network driver didn't care about config files when a network was destroyed, just when it was undefined leaving behind files for transient networks.

This patch splits out the cleanup code to a helper function that handles the cleanup if the inactive network object is being removed and re-uses this code when getting rid of inactive networks. --- src/network/bridge_driver.c | 133 +++++++++++++++++++++++++------------------- 1 file changed, 76 insertions(+), 57 deletions(-)

diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 976c132..45fca0e 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -155,6 +155,67 @@ networkRadvdConfigFileName(const char *netname) return configfile; }

+/* do needed cleanup steps and remove the network from the list */ +static int +networkRemoveInactive(struct network_driver *driver, + virNetworkObjPtr net) +{ + char *leasefile = NULL; + char *radvdconfigfile = NULL; + char *radvdpidbase = NULL; + dnsmasqContext *dctx = NULL; + virNetworkIpDefPtr ipdef; + virNetworkDefPtr def = virNetworkObjGetPersistentDef(net); + + int ret = -1; + int i; + + /* we only support dhcp on one IPv4 address per defined network */ + for (i = 0; + (ipdef = virNetworkDefGetIpByIndex(def, AF_UNSPEC, i)); + i++) { + if (VIR_SOCKET_ADDR_IS_FAMILY(&ipdef->address, AF_INET) && + (ipdef->nranges || ipdef->nhosts)) { + /* clean up files left over by dnsmasq */ + if (!(dctx = dnsmasqContextNew(def->name, DNSMASQ_STATE_DIR))) + goto cleanup; + + if (!(leasefile = networkDnsmasqLeaseFileName(def->name))) + goto cleanup; + + dnsmasqDelete(dctx); + unlink(leasefile);

A couple of things about this: 1) I think it would be fine to do all of these deletes anytime a network is destroyed, not just when it's undefined (although I guess it's possible someone might rely on this behavior, it's not documented and not part of the API (and I don't know why they would rely on it anyway). 2) Since we're not checking for errors anyway, I think we should just always try the deletes/unlinks - it's possible that the configuration of the network changed during its lifetime and the IP addresses/ranges/hosts/whatever were deleted, so that now the network isn't doing dhcp, but it was in the past and has stale files left around.

+ + } else if (VIR_SOCKET_ADDR_IS_FAMILY(&ipdef->address, AF_INET6)) { + /* clean up files left over by radvd */ + + if (!(radvdconfigfile = networkRadvdConfigFileName(def->name))) + goto no_memory; + + if (!(radvdpidbase = networkRadvdPidfileBasename(def->name))) + goto no_memory; + + unlink(radvdconfigfile); + virPidFileDelete(NETWORK_PID_DIR, radvdpidbase);

Same here. I think we should just always attempt these operations, whether there are any ipv6 addresses or not. (anyway, the way it's written now, if you had 5 IPv6 addresses on the network, you would be attempting the same unlinks 5 times.)

+ } + } + + virNetworkRemoveInactive(&driver->networks, net); + + ret = 0; + +cleanup: + VIR_FREE(leasefile); + VIR_FREE(radvdconfigfile); + VIR_FREE(radvdpidbase); + dnsmasqContextFree(dctx); + return ret; + +no_memory: + virReportOOMError(); + goto cleanup; +} + static char * networkBridgeDummyNicName(const char *brname) { @@ -2806,12 +2867,11 @@ cleanup: return ret; }

-static int networkUndefine(virNetworkPtr net) { +static int +networkUndefine(virNetworkPtr net) { struct network_driver *driver = net->conn->networkPrivateData; virNetworkObjPtr network; - virNetworkIpDefPtr ipdef; - bool dhcp_present = false, v6present = false; - int ret = -1, ii; + int ret = -1;

networkDriverLock(driver);

@@ -2833,58 +2893,12 @@ static int networkUndefine(virNetworkPtr net) { network) < 0) goto cleanup;

- /* we only support dhcp on one IPv4 address per defined network */ - for (ii = 0; - (ipdef = virNetworkDefGetIpByIndex(network->def, AF_UNSPEC, ii)); - ii++) { - if (VIR_SOCKET_ADDR_IS_FAMILY(&ipdef->address, AF_INET)) { - if (ipdef->nranges || ipdef->nhosts) - dhcp_present = true; - } else if (VIR_SOCKET_ADDR_IS_FAMILY(&ipdef->address, AF_INET6)) { - v6present = true; - } - } - - if (dhcp_present) { - char *leasefile; - dnsmasqContext *dctx = dnsmasqContextNew(network->def->name, DNSMASQ_STATE_DIR); - if (dctx == NULL) - goto cleanup; - - dnsmasqDelete(dctx); - dnsmasqContextFree(dctx); - - leasefile = networkDnsmasqLeaseFileName(network->def->name); - if (!leasefile) - goto cleanup; - unlink(leasefile); - VIR_FREE(leasefile); - } - - if (v6present) { - char *configfile = networkRadvdConfigFileName(network->def->name); - - if (!configfile) { - virReportOOMError(); - goto cleanup; - } - unlink(configfile); - VIR_FREE(configfile); - - char *radvdpidbase = networkRadvdPidfileBasename(network->def->name); - - if (!(radvdpidbase)) { - virReportOOMError(); - goto cleanup; - } - virPidFileDelete(NETWORK_PID_DIR, radvdpidbase); - VIR_FREE(radvdpidbase); - + VIR_INFO("Undefining network '%s'", network->def->name); + if (networkRemoveInactive(driver, network) < 0) { + network = NULL; + goto cleanup; }

- VIR_INFO("Undefining network '%s'", network->def->name); - virNetworkRemoveInactive(&driver->networks, - network); network = NULL; ret = 0;

@@ -3085,10 +3099,15 @@ static int networkDestroy(virNetworkPtr net) { goto cleanup; }

- ret = networkShutdownNetwork(driver, network); + if ((ret = networkShutdownNetwork(driver, network)) < 0) + goto cleanup; + if (!network->persistent) { - virNetworkRemoveInactive(&driver->networks, - network); + if (networkRemoveInactive(driver, network) < 0) { + network = NULL; + ret = -1; + goto cleanup; + } network = NULL; }

So, definitely you should do (2) above, and I think you should also do (1), but if you want to do that separately/later, that's okay too.

On 10/25/2012 05:06 PM, Peter Krempa wrote:

On 10/25/12 22:56, Laine Stump wrote:

...

On 10/25/2012 11:18 AM, Peter Krempa wrote:

...

The network driver didn't care about config files when a network was destroyed, just when it was undefined leaving behind files for transient networks.

This patch splits out the cleanup code to a helper function that handles the cleanup if the inactive network object is being removed and re-uses this code when getting rid of inactive networks. --- src/network/bridge_driver.c | 133 +++++++++++++++++++++++++------------------- 1 file changed, 76 insertions(+), 57 deletions(-)

diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 976c132..45fca0e 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -155,6 +155,67 @@ networkRadvdConfigFileName(const char *netname) return configfile; }

+/* do needed cleanup steps and remove the network from the list */ +static int +networkRemoveInactive(struct network_driver *driver, + virNetworkObjPtr net) +{ + char *leasefile = NULL; + char *radvdconfigfile = NULL; + char *radvdpidbase = NULL; + dnsmasqContext *dctx = NULL; + virNetworkIpDefPtr ipdef; + virNetworkDefPtr def = virNetworkObjGetPersistentDef(net); + + int ret = -1; + int i; + + /* we only support dhcp on one IPv4 address per defined network */ + for (i = 0; + (ipdef = virNetworkDefGetIpByIndex(def, AF_UNSPEC, i)); + i++) { + if (VIR_SOCKET_ADDR_IS_FAMILY(&ipdef->address, AF_INET) && + (ipdef->nranges || ipdef->nhosts)) { + /* clean up files left over by dnsmasq */ + if (!(dctx = dnsmasqContextNew(def->name, DNSMASQ_STATE_DIR))) + goto cleanup; + + if (!(leasefile = networkDnsmasqLeaseFileName(def->name))) + goto cleanup; + + dnsmasqDelete(dctx); + unlink(leasefile);

A couple of things about this:

1) I think it would be fine to do all of these deletes anytime a network is destroyed, not just when it's undefined (although I guess it's possible someone might rely on this behavior, it's not documented and not part of the API (and I don't know why they would rely on it anyway).

That was the way I implemented it at first. I know it's not documented in any way but deleting the lease file has one implication: Every time you destroy the network you are risking that your guests are being re-addressed. It can be fully expected while using DHCP without static leases, but I think of it as a pretty sweet feature and I remember addresses of some of my guests and I'm glad they get the same addresses every time. On the other hand, the static hosts file can be deleted when destroying the net every time without any problems.

Good point. I retract that suggestion.

...

2) Since we're not checking for errors anyway, I think we should just always try the deletes/unlinks - it's possible that the configuration of the network changed during its lifetime and the IP addresses/ranges/hosts/whatever were deleted, so that now the network isn't doing dhcp, but it was in the past and has stale files left around.

Agreed, I didn't think about this option.

Peter

On 10/26/2012 05:58 AM, Peter Krempa wrote:

The network driver didn't care about config files when a network was destroyed, just when it was undefined leaving behind files for transient networks.

This patch splits out the cleanup code to a helper function that handles the cleanup if the inactive network object is being removed and re-uses this code when getting rid of inactive networks. --- Diff to v1: unconditionaly delete the config files even if they might not exist --- src/network/bridge_driver.c | 123 ++++++++++++++++++++++++-------------------- 1 file changed, 66 insertions(+), 57 deletions(-)

diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 08e55d9..ca254a3 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -155,6 +155,57 @@ networkRadvdConfigFileName(const char *netname) return configfile; }

+/* do needed cleanup steps and remove the network from the list */ +static int +networkRemoveInactive(struct network_driver *driver, + virNetworkObjPtr net) +{ + char *leasefile = NULL; + char *radvdconfigfile = NULL; + char *radvdpidbase = NULL; + dnsmasqContext *dctx = NULL; + virNetworkDefPtr def = virNetworkObjGetPersistentDef(net); + + int ret = -1; + + /* remove the (possibly) existing dnsmasq and radvd files */ + if (!(dctx = dnsmasqContextNew(def->name, DNSMASQ_STATE_DIR))) + goto cleanup; + + if (!(leasefile = networkDnsmasqLeaseFileName(def->name))) + goto cleanup; + + if (!(radvdconfigfile = networkRadvdConfigFileName(def->name))) + goto no_memory; + + if (!(radvdpidbase = networkRadvdPidfileBasename(def->name))) + goto no_memory; + + /* dnsmasq */ + dnsmasqDelete(dctx); + unlink(leasefile); + + /* radvd */ + unlink(radvdconfigfile); + virPidFileDelete(NETWORK_PID_DIR, radvdpidbase); + + /* remove the network definition */ + virNetworkRemoveInactive(&driver->networks, net); + + ret = 0; + +cleanup: + VIR_FREE(leasefile); + VIR_FREE(radvdconfigfile); + VIR_FREE(radvdpidbase); + dnsmasqContextFree(dctx); + return ret; + +no_memory: + virReportOOMError(); + goto cleanup; +} + static char * networkBridgeDummyNicName(const char *brname) { @@ -2822,12 +2873,11 @@ cleanup: return ret; }

-static int networkUndefine(virNetworkPtr net) { +static int +networkUndefine(virNetworkPtr net) { struct network_driver *driver = net->conn->networkPrivateData; virNetworkObjPtr network; - virNetworkIpDefPtr ipdef; - bool dhcp_present = false, v6present = false; - int ret = -1, ii; + int ret = -1;

networkDriverLock(driver);

@@ -2849,58 +2899,12 @@ static int networkUndefine(virNetworkPtr net) { network) < 0) goto cleanup;

- /* we only support dhcp on one IPv4 address per defined network */ - for (ii = 0; - (ipdef = virNetworkDefGetIpByIndex(network->def, AF_UNSPEC, ii)); - ii++) { - if (VIR_SOCKET_ADDR_IS_FAMILY(&ipdef->address, AF_INET)) { - if (ipdef->nranges || ipdef->nhosts) - dhcp_present = true; - } else if (VIR_SOCKET_ADDR_IS_FAMILY(&ipdef->address, AF_INET6)) { - v6present = true; - } - } - - if (dhcp_present) { - char *leasefile; - dnsmasqContext *dctx = dnsmasqContextNew(network->def->name, DNSMASQ_STATE_DIR); - if (dctx == NULL) - goto cleanup; - - dnsmasqDelete(dctx); - dnsmasqContextFree(dctx); - - leasefile = networkDnsmasqLeaseFileName(network->def->name); - if (!leasefile) - goto cleanup; - unlink(leasefile); - VIR_FREE(leasefile); - } - - if (v6present) { - char *configfile = networkRadvdConfigFileName(network->def->name); - - if (!configfile) { - virReportOOMError(); - goto cleanup; - } - unlink(configfile); - VIR_FREE(configfile); - - char *radvdpidbase = networkRadvdPidfileBasename(network->def->name); - - if (!(radvdpidbase)) { - virReportOOMError(); - goto cleanup; - } - virPidFileDelete(NETWORK_PID_DIR, radvdpidbase); - VIR_FREE(radvdpidbase); - + VIR_INFO("Undefining network '%s'", network->def->name); + if (networkRemoveInactive(driver, network) < 0) { + network = NULL; + goto cleanup; }

- VIR_INFO("Undefining network '%s'", network->def->name); - virNetworkRemoveInactive(&driver->networks, - network); network = NULL; ret = 0;

@@ -3101,10 +3105,15 @@ static int networkDestroy(virNetworkPtr net) { goto cleanup; }

- ret = networkShutdownNetwork(driver, network); + if ((ret = networkShutdownNetwork(driver, network)) < 0) + goto cleanup; + if (!network->persistent) { - virNetworkRemoveInactive(&driver->networks, - network); + if (networkRemoveInactive(driver, network) < 0) { + network = NULL; + ret = -1; + goto cleanup; + } network = NULL; }

ACK

On 10/25/2012 11:18 AM, Peter Krempa wrote:

When a transient network was created some of the checks weren't run on the definition allowing to start invalid networks.

This patch splits out code to the network validation function and re-uses that code when creating transient networks.

Nice cleanup / fix! There are a few other problems with transient networks that I've been meaning to fix in order to bring their level of operation up to parity with transient domains. See https://bugzilla.redhat.com/show_bug.cgi?id=819416 for example.

--- src/network/bridge_driver.c | 96 +++++++++++++++++++-------------------------- 1 file changed, 40 insertions(+), 56 deletions(-)

diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 45fca0e..e90444d 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -2689,11 +2689,48 @@ cleanup:

static int -networkValidate(virNetworkDefPtr def) +networkValidate(struct network_driver *driver, + virNetworkDefPtr def, + bool check_active) { int ii; bool vlanUsed, vlanAllowed; const char *defaultPortGroup = NULL; + virNetworkIpDefPtr ipdef; + bool ipv4def = false; + int i; + + /* check for duplicate networks */ + if (virNetworkObjIsDuplicate(&driver->networks, def, check_active) < 0) + return -1; + + /* Only the three L3 network types that are configured by libvirt + * need to have a bridge device name / mac address provided + */ + if (def->forwardType == VIR_NETWORK_FORWARD_NONE || + def->forwardType == VIR_NETWORK_FORWARD_NAT || + def->forwardType == VIR_NETWORK_FORWARD_ROUTE) { + + if (virNetworkSetBridgeName(&driver->networks, def, 1)) + return -1; + + virNetworkSetBridgeMacAddr(def);

Well, those aren't strictly "validating the network", but are rather auto-setting some attributes. But since they are required in both cases, and happen at the same time we are validating that the network config is okay, I guess it's reasonable to put it here.

+ } + + /* We only support dhcp on one IPv4 address per defined network */ + for (i = 0; (ipdef = virNetworkDefGetIpByIndex(def, AF_INET, i)); i++) { + if (ipdef->nranges || ipdef->nhosts) { + if (ipv4def) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("Multiple dhcp sections found. " + "dhcp is supported only for a " + "single IPv4 address on each network")); + return -1; + } else { + ipv4def = true; + } + } + }

/* The only type of networks that currently support transparent * vlan configuration are those using hostdev sr-iov devices from @@ -2747,23 +2784,7 @@ static virNetworkPtr networkCreate(virConnectPtr conn, const char *xml) { if (!(def = virNetworkDefParseString(xml))) goto cleanup;

- if (virNetworkObjIsDuplicate(&driver->networks, def, true) < 0) - goto cleanup; - - /* Only the three L3 network types that are configured by libvirt - * need to have a bridge device name / mac address provided - */ - if (def->forwardType == VIR_NETWORK_FORWARD_NONE || - def->forwardType == VIR_NETWORK_FORWARD_NAT || - def->forwardType == VIR_NETWORK_FORWARD_ROUTE) { - - if (virNetworkSetBridgeName(&driver->networks, def, 1)) - goto cleanup; - - virNetworkSetBridgeMacAddr(def); - } - - if (networkValidate(def) < 0) + if (networkValidate(driver, def, true) < 0) goto cleanup;

/* NB: "live" is false because this transient network hasn't yet @@ -2793,54 +2814,17 @@ cleanup:

static virNetworkPtr networkDefine(virConnectPtr conn, const char *xml) { struct network_driver *driver = conn->networkPrivateData; - virNetworkIpDefPtr ipdef, ipv4def = NULL; virNetworkDefPtr def; bool freeDef = true; virNetworkObjPtr network = NULL; virNetworkPtr ret = NULL; - int ii;

networkDriverLock(driver);

if (!(def = virNetworkDefParseString(xml))) goto cleanup;

- if (virNetworkObjIsDuplicate(&driver->networks, def, false) < 0) - goto cleanup; - - /* Only the three L3 network types that are configured by libvirt - * need to have a bridge device name / mac address provided - */ - if (def->forwardType == VIR_NETWORK_FORWARD_NONE || - def->forwardType == VIR_NETWORK_FORWARD_NAT || - def->forwardType == VIR_NETWORK_FORWARD_ROUTE) { - - if (virNetworkSetBridgeName(&driver->networks, def, 1)) - goto cleanup; - - virNetworkSetBridgeMacAddr(def); - } - - /* We only support dhcp on one IPv4 address per defined network */ - for (ii = 0; - (ipdef = virNetworkDefGetIpByIndex(def, AF_UNSPEC, ii)); - ii++) { - if (VIR_SOCKET_ADDR_IS_FAMILY(&ipdef->address, AF_INET)) { - if (ipdef->nranges || ipdef->nhosts) { - if (ipv4def) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("Multiple dhcp sections found. " - "dhcp is supported only for a " - "single IPv4 address on each network")); - goto cleanup; - } else { - ipv4def = ipdef; - } - } - } - } - - if (networkValidate(def) < 0) + if (networkValidate(driver, def, false) < 0) goto cleanup;

if (!(network = virNetworkAssignDef(&driver->networks, def, false)))

ACK.

On 10/25/2012 11:18 AM, Peter Krempa wrote:

Until now, the network undefine API was able to undefine only inactive networks. The restriction doesn't make sense any more so this patch implements changing networks to transient. --- src/network/bridge_driver.c | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index e90444d..95aaea9 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -2856,6 +2856,7 @@ networkUndefine(virNetworkPtr net) { struct network_driver *driver = net->conn->networkPrivateData; virNetworkObjPtr network; int ret = -1; + bool active = false;

networkDriverLock(driver);

@@ -2866,24 +2867,25 @@ networkUndefine(virNetworkPtr net) { goto cleanup; }

- if (virNetworkObjIsActive(network)) { - virReportError(VIR_ERR_OPERATION_INVALID, - "%s", _("network is still active")); - goto cleanup; - } + if (virNetworkObjIsActive(network)) + active = true;

if (virNetworkDeleteConfig(driver->networkConfigDir, driver->networkAutostartDir, network) < 0) goto cleanup;

+ network->persistent = 0; + VIR_INFO("Undefining network '%s'", network->def->name); - if (networkRemoveInactive(driver, network) < 0) { + if (!active) { + if (networkRemoveInactive(driver, network) < 0) { + network = NULL; + goto cleanup; + } network = NULL; - goto cleanup; }

If the network *is* active, you also need to free network->newDef and NULL it out. newDef should only be non-NULL for persistent networks (I *really* dislike the whole "def, newDef" way of doing things. I think there should be a "stateDef" and "configDef" instead, and they should be moved back and forth all the time - that just makes life confusing; configDef should always contain the persistent config (if it's a persistent network, or 0 otherwise) and stateDef should always contain the live state of the network (or 0 if its an inactive persistent network).

- network = NULL; ret = 0;

cleanup:

ACK once you clear out newDef (that other change is for later :-)

On 10/26/2012 08:55 AM, Peter Krempa wrote:

When the assignment fails, the network object is not unlocked and next call that would use it deadlocks. --- src/conf/network_conf.c | 1 + 1 file changed, 1 insertion(+)

diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 93d1b4c..1955853 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -295,6 +295,7 @@ virNetworkAssignDef(virNetworkObjListPtr nets,

if ((network = virNetworkFindByName(nets, def->name))) { if (virNetworkObjAssignDef(network, def, live) < 0) { + virNetworkObjUnlock(network); return NULL; } return network;

ACK.

On 10/26/2012 08:58 AM, Peter Krempa wrote:

When assigning the new persistent definition for a transient network (thus making it persistent) the network needs to be marked persistent before actually atempting to assign the definition. --- src/network/bridge_driver.c | 7 +++++++ 1 file changed, 7 insertions(+)

diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 22bd99b..f0dbd66 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -2833,6 +2833,13 @@ static virNetworkPtr networkDefine(virConnectPtr conn, const char *xml) { if (networkValidate(driver, def, false) < 0) goto cleanup;

+ /* make the network persistent if we're defininig it */ + if ((network = virNetworkFindByName(&driver->networks, def->name))) { + network->persistent = 1; + virNetworkObjUnlock(network); + network = NULL; + } + if (!(network = virNetworkAssignDef(&driver->networks, def, false))) goto cleanup; freeDef = false;

It's a bit bothersome that this results in two back to back calls to virNetworkFindByName(). Is there a reasonable way to enhance virNetworkAssignDef() to do this instead?

On 10/29/2012 03:35 AM, Peter Krempa wrote:

When assigning the new persistent definition for a transient network (thus making it persistent) the network needs to be marked persistent before actually atempting to assign the definition. --- src/network/bridge_driver.c | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-)

You might want to get Laine's opinion as well, but I think this is a candidate for 1.0.0, and looks right to me. ACK.

diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 22bd99b..643b00c 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -2820,7 +2820,7 @@ cleanup:

static virNetworkPtr networkDefine(virConnectPtr conn, const char *xml) { struct network_driver *driver = conn->networkPrivateData; - virNetworkDefPtr def; + virNetworkDefPtr def = NULL; bool freeDef = true; virNetworkObjPtr network = NULL; virNetworkPtr ret = NULL; @@ -2833,11 +2833,17 @@ static virNetworkPtr networkDefine(virConnectPtr conn, const char *xml) { if (networkValidate(driver, def, false) < 0) goto cleanup;

- if (!(network = virNetworkAssignDef(&driver->networks, def, false))) - goto cleanup; - freeDef = false; + if ((network = virNetworkFindByName(&driver->networks, def->name))) { + network->persistent = 1; + if (virNetworkObjAssignDef(network, def, false) < 0) + goto cleanup; + } else { + if (!(network = virNetworkAssignDef(&driver->networks, def, false))) + goto cleanup; + }

- network->persistent = 1; + /* def was asigned */ + freeDef = false;

if (virNetworkSaveConfig(driver->networkConfigDir, def) < 0) { virNetworkRemoveInactive(&driver->networks, network);

-- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

On 2012年11月02日 01:30, Laine Stump wrote:

On 11/01/2012 11:25 AM, Eric Blake wrote:

...

On 10/29/2012 03:35 AM, Peter Krempa wrote:

...

When assigning the new persistent definition for a transient network (thus making it persistent) the network needs to be marked persistent before actually atempting to assign the definition. --- src/network/bridge_driver.c | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) You might want to get Laine's opinion as well, but I think this is a candidate for 1.0.0, and looks right to me.

Transient networks have never worked very well (mainly because nobody has ever used them) and the other patches in this series (plus more) are needed to make them work properly, so I think it makes more sense to save them all until after 1.0 is released.

...

ACK.

...

diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 22bd99b..643b00c 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -2820,7 +2820,7 @@ cleanup:

static virNetworkPtr networkDefine(virConnectPtr conn, const char

*xml) {

...

...

struct network_driver *driver = conn->networkPrivateData; - virNetworkDefPtr def; + virNetworkDefPtr def = NULL; bool freeDef = true; virNetworkObjPtr network = NULL; virNetworkPtr ret = NULL; @@ -2833,11 +2833,17 @@ static virNetworkPtr

networkDefine(virConnectPtr conn, const char *xml) {

...

if (networkValidate(driver, def, false)< 0) goto cleanup;

- if (!(network = virNetworkAssignDef(&driver->networks, def,

false)))

...

- goto cleanup; - freeDef = false; + if ((network = virNetworkFindByName(&driver->networks, def->name))) { + network->persistent = 1; + if (virNetworkObjAssignDef(network, def, false)< 0) + goto cleanup;

Except the introduced regression [1]. This could cause incorrect result once virNetworkObjAssignDef fails, though I believe it's unlikely to fail (Assuming there is a same network, but transient, it will be changed to persistent).

...

...

+ } else { + if (!(network = virNetworkAssignDef(&driver->networks, def, false))) + goto cleanup; + }

- network->persistent = 1; + /* def was asigned */ + freeDef = false;

if (virNetworkSaveConfig(driver->networkConfigDir, def)< 0) { virNetworkRemoveInactive(&driver->networks, network);

And virNetworkRemoveInactive could be bindly removed a previous inactive network obj. Also we have to restore the previous network's def if virNetworkSaveConfig fails. [4] https://www.redhat.com/archives/libvir-list/2012-December/msg01355.html Osier