[libvirt] [PATCH 0/2] skip names of devices already existing on host when auto-assigning bridge name
Shivaprasad Bhat earlier sent patches to fix this problem which added a call to virNetDevExists() in the conf directory: https://www.redhat.com/archives/libvir-list/2015-March/msg00397.html (V1) https://www.redhat.com/archives/libvir-list/2015-March/msg00569.html (V2) https://www.redhat.com/archives/libvir-list/2015-March/msg01174.html (V3) https://www.redhat.com/archives/libvir-list/2015-March/msg01297.html (V4) After discussing it with him a few times (both email and irc), I decided that the only way to really understand the problems he described (related to locking) with my counterproposal (do the checking in the bridge driver itself), was to try fixing it myself. I do now understand the problems he encountered with locking, but also found that the instance of the call to the "Find a bridge name and set it" function that caused the problem was unnecessary (see the commit message of patch 1). The first patch here essentially reimplements current functionality but moving the majority of the code from conf/network_conf.c to network/bridge_driver.c. The 2nd patch adds the all-important virNetDevExists() call to the function that is looking for an unused bridge name. Laine Stump (2): network: move auto-assign of bridge name from XML parser to net driver network: check for bridge name conflict with existing devices src/conf/network_conf.c | 60 --------------------------- src/conf/network_conf.h | 9 +---- src/libvirt_private.syms | 2 +- src/network/bridge_driver.c | 98 ++++++++++++++++++++++++++++++++++++++++++++- 4 files changed, 99 insertions(+), 70 deletions(-) -- 2.1.0
We already check that any auto-assigned bridge device name for a virtual network (e.g. "virbr1") doesn't conflict with the bridge name for any existing libvirt network (via virNetworkSetBridgeName() in conf/network_conf.c). We also want to check that the name doesn't conflict with any bridge device created on the host system outside the control of libvirt (history: possibly due to the ploriferation of references to libvirt's bridge devices in HOWTO documents all around the web, it is not uncommon for an admin to manually create a bridge in their host's system network config and name it "virbrX"). To add such a check to virNetworkBridgeInUse() (which is called by virNetworkSetBridgeName()) we would have to call virNetDevExists() (from util/virnetdev.c); this function calls ioctl(SIOCGIFFLAGS), which everyone on the mailing list agreed should not be done from an XML parsing function in the conf directory. To remedy that problem, this patch removes virNetworkSetBridgeName() from conf/network_conf.c and puts an identically functioning networkBridgeNameValidate() in network/bridge_driver.c (because it's reasonable for the bridge driver to call virNetDevExists(), although we don't do that yet because I wanted this patch to have as close to 0 effect on function as possible). There are a couple of inevitable changes though: 1) We no longer check the bridge name during virNetworkLoadConfig(). Close examination of the code shows that this wasn't necessary anyway - the only *correct* way to get XML into the config files is via networkDefine(), and networkDefine() will always call networkValidate(), which previously called virNetworkSetBridgeName() (and now calls networkBridgeNameValidate()). This means that the only way the bridge name can be unset during virNetworkLoadConfig() is if someone edited the config file on disk by hand (which we explicitly prohibit). 2) Just on the off chance that somebody *has* edited the file by hand, rather than crashing when they try to start their malformed network, a check for non-NULL bridge name has been added to networkStartNetworkVirtual(). (For those wondering why I don't instead call networkValidateBridgeName() there to set a bridge name if one wasn't present - the problem is that during networkStartNetworkVirtual(), the lock for the network being started has already been acquired, but the lock for the network list itself *has not* (because we aren't adding/removing a network). But virNetworkBridgeInuse() iterates through *all* networks (including this one) and locks each network as it is checked for a duplicate entry; it is necessary to lock each network even before checking if it is the designated "skip" network because otherwise some other thread might acquire the list lock and delete the very entry we're examining. In the end, permitting a setting of the bridge name during network start would require that we lock the entire network list during any networkStartNetwork(), which eliminates a *lot* of parallelism that we've worked so hard to achieve (it can make a huge difference during libvirtd startup). So rather than try to adjust for someone playing against the rules, I choose to instead give them the error they deserve.) 3) virNetworkAllocateBridge() (now removed) would leak any "template" string set as the bridge name. Its replacement networkFindUnusedBridgeName() doesn't leak the template string - it is properly freed. --- src/conf/network_conf.c | 60 ------------------------------ src/conf/network_conf.h | 9 +---- src/libvirt_private.syms | 2 +- src/network/bridge_driver.c | 89 ++++++++++++++++++++++++++++++++++++++++++++- 4 files changed, 90 insertions(+), 70 deletions(-) diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index f4a9df0..aa8d6c6 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -45,7 +45,6 @@ #include "virfile.h" #include "virstring.h" -#define MAX_BRIDGE_ID 256 #define VIR_FROM_THIS VIR_FROM_NETWORK /* currently, /sbin/tc implementation allows up to 16 bits for minor class size */ #define CLASS_ID_BITMAP_SIZE (1<<16) @@ -3123,11 +3122,6 @@ virNetworkObjPtr virNetworkLoadConfig(virNetworkObjListPtr nets, virNetworkSetBridgeMacAddr(def); virNetworkSaveConfig(configDir, def); } - /* Generate a bridge if none is specified, but don't check for collisions - * if a bridge is hardcoded, so the network is at least defined. - */ - if (virNetworkSetBridgeName(nets, def, 0)) - goto error; } else { /* Throw away MAC address for other forward types, * which could have been generated by older libvirt RPMs */ @@ -3303,60 +3297,6 @@ int virNetworkBridgeInUse(virNetworkObjListPtr nets, return obj != NULL; } -char *virNetworkAllocateBridge(virNetworkObjListPtr nets, - const char *template) -{ - - int id = 0; - char *newname; - - if (!template) - template = "virbr%d"; - - do { - if (virAsprintf(&newname, template, id) < 0) - return NULL; - if (!virNetworkBridgeInUse(nets, newname, NULL)) - return newname; - VIR_FREE(newname); - - id++; - } while (id <= MAX_BRIDGE_ID); - - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Bridge generation exceeded max id %d"), - MAX_BRIDGE_ID); - return NULL; -} - -int virNetworkSetBridgeName(virNetworkObjListPtr nets, - virNetworkDefPtr def, - int check_collision) -{ - int ret = -1; - - if (def->bridge && !strstr(def->bridge, "%d")) { - /* We may want to skip collision detection in this case (ex. when - * loading configs at daemon startup, so the network is at least - * defined. */ - if (check_collision && - virNetworkBridgeInUse(nets, def->bridge, def->name)) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("bridge name '%s' already in use."), - def->bridge); - goto error; - } - } else { - /* Allocate a bridge name */ - if (!(def->bridge = virNetworkAllocateBridge(nets, def->bridge))) - goto error; - } - - ret = 0; - error: - return ret; -} - void virNetworkSetBridgeMacAddr(virNetworkDefPtr def) { diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h index f69d999..9411a02 100644 --- a/src/conf/network_conf.h +++ b/src/conf/network_conf.h @@ -1,7 +1,7 @@ /* * network_conf.h: network XML handling * - * Copyright (C) 2006-2014 Red Hat, Inc. + * Copyright (C) 2006-2015 Red Hat, Inc. * Copyright (C) 2006-2008 Daniel P. Berrange * * This library is free software; you can redistribute it and/or @@ -401,13 +401,6 @@ int virNetworkBridgeInUse(virNetworkObjListPtr nets, const char *bridge, const char *skipname); -char *virNetworkAllocateBridge(virNetworkObjListPtr nets, - const char *template); - -int virNetworkSetBridgeName(virNetworkObjListPtr nets, - virNetworkDefPtr def, - int check_collision); - void virNetworkSetBridgeMacAddr(virNetworkDefPtr def); int diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 8c50ea2..ae318dc 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -570,6 +570,7 @@ virNetDevVPortTypeToString; # conf/network_conf.h virNetworkAssignDef; +virNetworkBridgeInUse; virNetworkBridgeMACTableManagerTypeFromString; virNetworkBridgeMACTableManagerTypeToString; virNetworkConfigChangeSetup; @@ -612,7 +613,6 @@ virNetworkRemoveInactive; virNetworkSaveConfig; virNetworkSaveStatus; virNetworkSetBridgeMacAddr; -virNetworkSetBridgeName; virNetworkTaintTypeFromString; virNetworkTaintTypeToString; virPortGroupFindByName; diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index d195085..abacae3 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -76,6 +76,7 @@ #include "virjson.h" #define VIR_FROM_THIS VIR_FROM_NETWORK +#define MAX_BRIDGE_ID 256 /** * VIR_NETWORK_DHCP_LEASE_FILE_SIZE_MAX: @@ -449,6 +450,7 @@ networkUpdateState(virNetworkObjPtr obj, return ret; } + static int networkAutostartConfig(virNetworkObjPtr net, void *opaque) @@ -2034,6 +2036,18 @@ networkStartNetworkVirtual(virNetworkDriverStatePtr driver, return -1; /* Create and configure the bridge device */ + if (!network->def->bridge) { + /* this can only happen if the config files were edited + * directly. Otherwise networkValidate() (called after parsing + * the XML from networkCreateXML() and networkDefine()) + * guarantees we will have a valid bridge name before this + * point. + */ + virReportError(VIR_ERR_INTERNAL_ERROR, + _("network '%s' has no bridge name defined"), + network->def->name); + return -1; + } if (virNetDevBridgeCreate(network->def->bridge) < 0) return -1; @@ -2746,6 +2760,76 @@ static int networkIsPersistent(virNetworkPtr net) } +/* + * networkFindUnusedBridgeName() - try to find a bridge name that is + * unused by the currently configured libvirt networks, and set this + * network's name to that new name. + */ +static int +networkFindUnusedBridgeName(virNetworkObjListPtr nets, + virNetworkDefPtr def) +{ + + int ret = -1, id = 0; + char *newname = NULL; + const char *templ = def->bridge ? def->bridge : "virbr%d"; + + do { + if (virAsprintf(&newname, templ, id) < 0) + goto cleanup; + if (!virNetworkBridgeInUse(nets, newname, def->name)) { + VIR_FREE(def->bridge); /*could contain template */ + def->bridge = newname; + ret = 0; + goto cleanup; + } + VIR_FREE(newname); + } while (++id <= MAX_BRIDGE_ID); + + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Bridge generation exceeded max id %d"), + MAX_BRIDGE_ID); + ret = 0; + cleanup: + if (ret < 0) + VIR_FREE(newname); + return ret; +} + + + +/* + * networkValidateBridgeName() - if no bridge name is set, or if the + * bridge name contains a %d (indicating that this is a template for + * the actual name) try to set an appropriate bridge name. If a + * bridge name *is* set, make sure it doesn't conflict with any other + * network's bridge name. + */ +static int +networkBridgeNameValidate(virNetworkObjListPtr nets, + virNetworkDefPtr def) +{ + int ret = -1; + + if (def->bridge && !strstr(def->bridge, "%d")) { + if (virNetworkBridgeInUse(nets, def->bridge, def->name)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("bridge name '%s' already in use."), + def->bridge); + goto cleanup; + } + } else { + /* Allocate a bridge name */ + if (networkFindUnusedBridgeName(nets, def) < 0) + goto cleanup; + } + + ret = 0; + cleanup: + return ret; +} + + static int networkValidate(virNetworkDriverStatePtr driver, virNetworkDefPtr def) @@ -2765,7 +2849,10 @@ networkValidate(virNetworkDriverStatePtr driver, def->forward.type == VIR_NETWORK_FORWARD_NAT || def->forward.type == VIR_NETWORK_FORWARD_ROUTE) { - if (virNetworkSetBridgeName(driver->networks, def, 1)) + /* if no bridge name was given in the config, find a name + * unused by any other libvirt networks and assign it. + */ + if (networkBridgeNameValidate(driver->networks, def) < 0) return -1; virNetworkSetBridgeMacAddr(def); -- 2.1.0
Since some people use the same naming convention as libvirt for bridge devices they create outside the context of libvirt, it is much nicer if we check for those devices when looking for a bridge device name to auto-assign to a new network. --- src/network/bridge_driver.c | 25 +++++++++++++++++-------- 1 file changed, 17 insertions(+), 8 deletions(-) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index abacae3..3b879cd 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -2037,11 +2037,13 @@ networkStartNetworkVirtual(virNetworkDriverStatePtr driver, /* Create and configure the bridge device */ if (!network->def->bridge) { - /* this can only happen if the config files were edited - * directly. Otherwise networkValidate() (called after parsing - * the XML from networkCreateXML() and networkDefine()) - * guarantees we will have a valid bridge name before this - * point. + /* bridge name can only be empty if the config files were + * edited directly. Otherwise networkValidate() (called after + * parsing the XML from networkCreateXML() and + * networkDefine()) guarantees we will have a valid bridge + * name before this point. Since hand editing of the config + * files is explicitly prohibited we can, with clear + * conscience, log an error and fail at this point. */ virReportError(VIR_ERR_INTERNAL_ERROR, _("network '%s' has no bridge name defined"), @@ -2762,8 +2764,9 @@ static int networkIsPersistent(virNetworkPtr net) /* * networkFindUnusedBridgeName() - try to find a bridge name that is - * unused by the currently configured libvirt networks, and set this - * network's name to that new name. + * unused by the currently configured libvirt networks, as well as by + * the host system itself (possibly created by someone/something other + * than libvirt). Set this network's name to that new name. */ static int networkFindUnusedBridgeName(virNetworkObjListPtr nets, @@ -2777,7 +2780,13 @@ networkFindUnusedBridgeName(virNetworkObjListPtr nets, do { if (virAsprintf(&newname, templ, id) < 0) goto cleanup; - if (!virNetworkBridgeInUse(nets, newname, def->name)) { + /* check if this name is used in another libvirt network or + * there is an existing device with that name. ignore errors + * from virNetDevExists(), just in case it isn't implemented + * on this platform (probably impossible). + */ + if (!(virNetworkBridgeInUse(nets, newname, def->name) || + virNetDevExists(newname) == 1)) { VIR_FREE(def->bridge); /*could contain template */ def->bridge = newname; ret = 0; -- 2.1.0
Thanks for the patches Laine. I agree pretty much with both the patches. also had a chance to try these out. Only scenario I see a trouble is, net-create without bridge name in the xml, followed by net-define using the same xml file. The live and --inactive xmls both show different bridge names, though the active bridge can be reused by the network for "this scenario alone". I think this is a very rare case and not worth to fix it, as net-destroy followed by net-create using the same xml file would anyway reuse the same old bridge name if available. Outside of this patch, the net->newDef->bridge's are not considered in virNetworkBridgeInUseHelper(). Do we need to? Thanks, Shiva On Fri, Apr 24, 2015 at 12:33 AM, Laine Stump <laine@laine.org> wrote:
Since some people use the same naming convention as libvirt for bridge devices they create outside the context of libvirt, it is much nicer if we check for those devices when looking for a bridge device name to auto-assign to a new network. --- src/network/bridge_driver.c | 25 +++++++++++++++++-------- 1 file changed, 17 insertions(+), 8 deletions(-)
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index abacae3..3b879cd 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -2037,11 +2037,13 @@ networkStartNetworkVirtual(virNetworkDriverStatePtr driver,
/* Create and configure the bridge device */ if (!network->def->bridge) { - /* this can only happen if the config files were edited - * directly. Otherwise networkValidate() (called after parsing - * the XML from networkCreateXML() and networkDefine()) - * guarantees we will have a valid bridge name before this - * point. + /* bridge name can only be empty if the config files were + * edited directly. Otherwise networkValidate() (called after + * parsing the XML from networkCreateXML() and + * networkDefine()) guarantees we will have a valid bridge + * name before this point. Since hand editing of the config + * files is explicitly prohibited we can, with clear + * conscience, log an error and fail at this point. */ virReportError(VIR_ERR_INTERNAL_ERROR, _("network '%s' has no bridge name defined"), @@ -2762,8 +2764,9 @@ static int networkIsPersistent(virNetworkPtr net)
/* * networkFindUnusedBridgeName() - try to find a bridge name that is - * unused by the currently configured libvirt networks, and set this - * network's name to that new name. + * unused by the currently configured libvirt networks, as well as by + * the host system itself (possibly created by someone/something other + * than libvirt). Set this network's name to that new name. */ static int networkFindUnusedBridgeName(virNetworkObjListPtr nets, @@ -2777,7 +2780,13 @@ networkFindUnusedBridgeName(virNetworkObjListPtr nets, do { if (virAsprintf(&newname, templ, id) < 0) goto cleanup; - if (!virNetworkBridgeInUse(nets, newname, def->name)) { + /* check if this name is used in another libvirt network or + * there is an existing device with that name. ignore errors + * from virNetDevExists(), just in case it isn't implemented + * on this platform (probably impossible). + */ + if (!(virNetworkBridgeInUse(nets, newname, def->name) || + virNetDevExists(newname) == 1)) { VIR_FREE(def->bridge); /*could contain template */ def->bridge = newname; ret = 0; -- 2.1.0
On 04/24/2015 06:58 AM, Shivaprasad bhat wrote:
Thanks for the patches Laine. I agree pretty much with both the patches. also had a chance to try these out.
Only scenario I see a trouble is,
net-create without bridge name in the xml, followed by net-define using the same xml file. The live and --inactive xmls both show different bridge names, though the active bridge can be reused by the network for "this scenario alone". I think this is a very rare case and not worth to fix it,
Using the original XML from net-create for a subsequent net-define is problematic in other ways too - if you don't specify MAC address you'll end up with def and newDef having different MAC addresses, and if you don't specify uuid (I would daresay almost nobody does) you will be unable to define the network since a different uuid will be autogenerated in both cases, and libvirt will refuse to redefine a network with the same name but different uuid. So I don't think we should consider this same/similar complication with bridge name a problem. The proper way to make a transient network persistent is this: virsh net-dumpxml $netname --inactive >/tmp/xyz.xml && \ virsh net-define /tmp/xyz.xml
as net-destroy followed by net-create using the same xml file would anyway reuse the same old bridge name if available.
Outside of this patch, the net->newDef->bridge's are not considered in virNetworkBridgeInUseHelper(). Do we need to?
Yes, that's a good idea. I'm posting a patch 3/2 to this series. You seem to approve of the patches and have tested them; will you be ACKing them?
Thanks, Shiva
On Fri, Apr 24, 2015 at 12:33 AM, Laine Stump <laine@laine.org> wrote:
Since some people use the same naming convention as libvirt for bridge devices they create outside the context of libvirt, it is much nicer if we check for those devices when looking for a bridge device name to auto-assign to a new network. --- src/network/bridge_driver.c | 25 +++++++++++++++++-------- 1 file changed, 17 insertions(+), 8 deletions(-)
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index abacae3..3b879cd 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -2037,11 +2037,13 @@ networkStartNetworkVirtual(virNetworkDriverStatePtr driver,
/* Create and configure the bridge device */ if (!network->def->bridge) { - /* this can only happen if the config files were edited - * directly. Otherwise networkValidate() (called after parsing - * the XML from networkCreateXML() and networkDefine()) - * guarantees we will have a valid bridge name before this - * point. + /* bridge name can only be empty if the config files were + * edited directly. Otherwise networkValidate() (called after + * parsing the XML from networkCreateXML() and + * networkDefine()) guarantees we will have a valid bridge + * name before this point. Since hand editing of the config + * files is explicitly prohibited we can, with clear + * conscience, log an error and fail at this point. */ virReportError(VIR_ERR_INTERNAL_ERROR, _("network '%s' has no bridge name defined"), @@ -2762,8 +2764,9 @@ static int networkIsPersistent(virNetworkPtr net)
/* * networkFindUnusedBridgeName() - try to find a bridge name that is - * unused by the currently configured libvirt networks, and set this - * network's name to that new name. + * unused by the currently configured libvirt networks, as well as by + * the host system itself (possibly created by someone/something other + * than libvirt). Set this network's name to that new name. */ static int networkFindUnusedBridgeName(virNetworkObjListPtr nets, @@ -2777,7 +2780,13 @@ networkFindUnusedBridgeName(virNetworkObjListPtr nets, do { if (virAsprintf(&newname, templ, id) < 0) goto cleanup; - if (!virNetworkBridgeInUse(nets, newname, def->name)) { + /* check if this name is used in another libvirt network or + * there is an existing device with that name. ignore errors + * from virNetDevExists(), just in case it isn't implemented + * on this platform (probably impossible). + */ + if (!(virNetworkBridgeInUse(nets, newname, def->name) || + virNetDevExists(newname) == 1)) { VIR_FREE(def->bridge); /*could contain template */ def->bridge = newname; ret = 0; -- 2.1.0
On Sat, Apr 25, 2015 at 12:11 AM, Laine Stump <laine@laine.org> wrote:
On 04/24/2015 06:58 AM, Shivaprasad bhat wrote:
Thanks for the patches Laine. I agree pretty much with both the patches. also had a chance to try these out.
Only scenario I see a trouble is,
net-create without bridge name in the xml, followed by net-define using the same xml file. The live and --inactive xmls both show different bridge names, though the active bridge can be reused by the network for "this scenario alone". I think this is a very rare case and not worth to fix it,
Using the original XML from net-create for a subsequent net-define is problematic in other ways too - if you don't specify MAC address you'll end up with def and newDef having different MAC addresses, and if you don't specify uuid (I would daresay almost nobody does) you will be unable to define the network since a different uuid will be autogenerated in both cases, and libvirt will refuse to redefine a network with the same name but different uuid. So I don't think we should consider this same/similar complication with bridge name a problem.
The proper way to make a transient network persistent is this:
virsh net-dumpxml $netname --inactive >/tmp/xyz.xml && \ virsh net-define /tmp/xyz.xml
as net-destroy followed by net-create using the same xml file would anyway reuse the same old bridge name if available.
Outside of this patch, the net->newDef->bridge's are not considered in virNetworkBridgeInUseHelper(). Do we need to?
Yes, that's a good idea. I'm posting a patch 3/2 to this series.
You seem to approve of the patches and have tested them; will you be ACKing them?
Certainly! ACK the first two patches. The third patch, I am leaving a comment there. Thanks, Shiva
Thanks, Shiva
On Fri, Apr 24, 2015 at 12:33 AM, Laine Stump <laine@laine.org> wrote:
Since some people use the same naming convention as libvirt for bridge devices they create outside the context of libvirt, it is much nicer if we check for those devices when looking for a bridge device name to auto-assign to a new network. --- src/network/bridge_driver.c | 25 +++++++++++++++++-------- 1 file changed, 17 insertions(+), 8 deletions(-)
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index abacae3..3b879cd 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -2037,11 +2037,13 @@ networkStartNetworkVirtual(virNetworkDriverStatePtr driver,
/* Create and configure the bridge device */ if (!network->def->bridge) { - /* this can only happen if the config files were edited - * directly. Otherwise networkValidate() (called after parsing - * the XML from networkCreateXML() and networkDefine()) - * guarantees we will have a valid bridge name before this - * point. + /* bridge name can only be empty if the config files were + * edited directly. Otherwise networkValidate() (called after + * parsing the XML from networkCreateXML() and + * networkDefine()) guarantees we will have a valid bridge + * name before this point. Since hand editing of the config + * files is explicitly prohibited we can, with clear + * conscience, log an error and fail at this point. */ virReportError(VIR_ERR_INTERNAL_ERROR, _("network '%s' has no bridge name defined"), @@ -2762,8 +2764,9 @@ static int networkIsPersistent(virNetworkPtr net)
/* * networkFindUnusedBridgeName() - try to find a bridge name that is - * unused by the currently configured libvirt networks, and set this - * network's name to that new name. + * unused by the currently configured libvirt networks, as well as by + * the host system itself (possibly created by someone/something other + * than libvirt). Set this network's name to that new name. */ static int networkFindUnusedBridgeName(virNetworkObjListPtr nets, @@ -2777,7 +2780,13 @@ networkFindUnusedBridgeName(virNetworkObjListPtr nets, do { if (virAsprintf(&newname, templ, id) < 0) goto cleanup; - if (!virNetworkBridgeInUse(nets, newname, def->name)) { + /* check if this name is used in another libvirt network or + * there is an existing device with that name. ignore errors + * from virNetDevExists(), just in case it isn't implemented + * on this platform (probably impossible). + */ + if (!(virNetworkBridgeInUse(nets, newname, def->name) || + virNetDevExists(newname) == 1)) { VIR_FREE(def->bridge); /*could contain template */ def->bridge = newname; ret = 0; -- 2.1.0
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
If someone has updated a network to change its bridge name, but the network is still active (so that bridge name hasn't taken effect yet), we still want to disallow another network from taking that new name. --- As suggested by Shivaprasad following his tests of patches 1 and 2... src/conf/network_conf.c | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index aa8d6c6..5b734f2 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -3270,15 +3270,22 @@ virNetworkBridgeInUseHelper(const void *payload, const void *name ATTRIBUTE_UNUSED, const void *opaque) { - int ret = 0; + int ret; virNetworkObjPtr net = (virNetworkObjPtr) payload; const struct virNetworkBridgeInUseHelperData *data = opaque; virObjectLock(net); - if (net->def->bridge && - STREQ(net->def->bridge, data->bridge) && - !(data->skipname && STREQ(net->def->name, data->skipname))) + if (data->skipname && + ((net->def && STREQ(net->def->name, data->skipname)) || + (net->newDef && STREQ(net->newDef->name, data->skipname)))) + ret = 0; + else if ((net->def && net->def->bridge && + STREQ(net->def->bridge, data->bridge)) || + (net->newDef && net->newDef->bridge && + STREQ(net->newDef->bridge, data->bridge))) ret = 1; + else + ret = 0; virObjectUnlock(net); return ret; } -- 2.1.0
On Sat, Apr 25, 2015 at 12:14 AM, Laine Stump <laine@laine.org> wrote:
If someone has updated a network to change its bridge name, but the network is still active (so that bridge name hasn't taken effect yet), we still want to disallow another network from taking that new name. --- As suggested by Shivaprasad following his tests of patches 1 and 2...
src/conf/network_conf.c | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-)
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index aa8d6c6..5b734f2 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -3270,15 +3270,22 @@ virNetworkBridgeInUseHelper(const void *payload, const void *name ATTRIBUTE_UNUSED, const void *opaque) { - int ret = 0; + int ret; virNetworkObjPtr net = (virNetworkObjPtr) payload; const struct virNetworkBridgeInUseHelperData *data = opaque;
virObjectLock(net); - if (net->def->bridge && - STREQ(net->def->bridge, data->bridge) && - !(data->skipname && STREQ(net->def->name, data->skipname))) + if (data->skipname && + ((net->def && STREQ(net->def->name, data->skipname)) || + (net->newDef && STREQ(net->newDef->name, data->skipname))))
Should the newDef->name be really be checked? The network cant be renamed. Right ? Thanks, Shiva
+ ret = 0; + else if ((net->def && net->def->bridge && + STREQ(net->def->bridge, data->bridge)) || + (net->newDef && net->newDef->bridge && + STREQ(net->newDef->bridge, data->bridge))) ret = 1; + else + ret = 0; virObjectUnlock(net); return ret; } -- 2.1.0
On 04/27/2015 05:54 AM, Shivaprasad bhat wrote:
On Sat, Apr 25, 2015 at 12:14 AM, Laine Stump <laine@laine.org> wrote:
If someone has updated a network to change its bridge name, but the network is still active (so that bridge name hasn't taken effect yet), we still want to disallow another network from taking that new name. --- As suggested by Shivaprasad following his tests of patches 1 and 2...
src/conf/network_conf.c | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-)
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index aa8d6c6..5b734f2 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -3270,15 +3270,22 @@ virNetworkBridgeInUseHelper(const void *payload, const void *name ATTRIBUTE_UNUSED, const void *opaque) { - int ret = 0; + int ret; virNetworkObjPtr net = (virNetworkObjPtr) payload; const struct virNetworkBridgeInUseHelperData *data = opaque;
virObjectLock(net); - if (net->def->bridge && - STREQ(net->def->bridge, data->bridge) && - !(data->skipname && STREQ(net->def->name, data->skipname))) + if (data->skipname && + ((net->def && STREQ(net->def->name, data->skipname)) || + (net->newDef && STREQ(net->newDef->name, data->skipname)))) Should the newDef->name be really be checked? The network cant be renamed. Right ?
Right now it can't be renamed, but I recall some discussion about possibly making that possible in the future. Adding this check doesn't hurt anything now, and may prevent confusion later. (more thinking about it - right now, we know that newDef->name and def->name will always match, so that clause is a NOP. If we ever do allow renaming a network, this clause would come into play if someone had already edited the network once (so that newDef was populated), then wanted to edit it a 2nd time before enacting that edited version; in this case we *would* want to allow the same bridge device name as was used in newDef, so what's being done here would be proper). (Actually this has me thinking that maybe the whole idea of skipping a particular *name* here is incorrect. Maybe we should be skipping a particular *uuid* (since that's the thing that's guaranteed to never change). Anyway, I think for now this patch can stand as-is. Are you convinced?
On Mon, Apr 27, 2015 at 11:20 PM, Laine Stump <laine@laine.org> wrote:
On 04/27/2015 05:54 AM, Shivaprasad bhat wrote:
On Sat, Apr 25, 2015 at 12:14 AM, Laine Stump <laine@laine.org> wrote:
If someone has updated a network to change its bridge name, but the network is still active (so that bridge name hasn't taken effect yet), we still want to disallow another network from taking that new name. --- As suggested by Shivaprasad following his tests of patches 1 and 2...
src/conf/network_conf.c | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-)
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index aa8d6c6..5b734f2 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -3270,15 +3270,22 @@ virNetworkBridgeInUseHelper(const void *payload, const void *name ATTRIBUTE_UNUSED, const void *opaque) { - int ret = 0; + int ret; virNetworkObjPtr net = (virNetworkObjPtr) payload; const struct virNetworkBridgeInUseHelperData *data = opaque;
virObjectLock(net); - if (net->def->bridge && - STREQ(net->def->bridge, data->bridge) && - !(data->skipname && STREQ(net->def->name, data->skipname))) + if (data->skipname && + ((net->def && STREQ(net->def->name, data->skipname)) || + (net->newDef && STREQ(net->newDef->name, data->skipname)))) Should the newDef->name be really be checked? The network cant be renamed. Right ?
Right now it can't be renamed, but I recall some discussion about possibly making that possible in the future. Adding this check doesn't hurt anything now, and may prevent confusion later.
(more thinking about it - right now, we know that newDef->name and def->name will always match, so that clause is a NOP. If we ever do allow renaming a network, this clause would come into play if someone had already edited the network once (so that newDef was populated), then wanted to edit it a 2nd time before enacting that edited version; in this case we *would* want to allow the same bridge device name as was used in newDef, so what's being done here would be proper).
(Actually this has me thinking that maybe the whole idea of skipping a particular *name* here is incorrect. Maybe we should be skipping a particular *uuid* (since that's the thing that's guaranteed to never change).
Anyway, I think for now this patch can stand as-is. Are you convinced?
Thanks for the explanation Laine. Given possible future plans, I agree it makes sense to have this check for now. ACK
participants (2)
-
Laine Stump -
Shivaprasad bhat