On 20.03.2015 20:59, John Ferlan wrote:
On 03/16/2015 12:42 PM, Michal Privoznik wrote:
This function does not make any sense now, that network driver is (almost) dropped. I mean, previously, when threads were serialized, this function was there to check, if no other network with the same name or UUID exists. However, nowadays that threads can run more in parallel, this function is useless, in fact it gives misleading return values. Consider the following scenario. Two threads, both trying to define networks with same name but different UUID (e.g. because it was generated during XML parsing phase, whatever). Lets assume that both threads are about to call networkValidate() which immediately calls virNetworkObjIsDuplicate().
T1: calls virNetworkObjIsDuplicate() and since no network with given name or UUID exist, success is returned. T2: calls virNetworkObjIsDuplicate() and since no network with given name or UUID exist, success is returned.
T1: calls virNetworkAssignDef() and successfully places its network into the virNetworkObjList. T2: calls virNetworkAssignDef() and since network with the same name exists, the network definition is replaced.
Okay, this is mainly because virNetworkAssignDef() does not check whether name and UUID matches. Well, lets make it so! And drop useless function too.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/network_conf.c | 171 ++++++++++++++++++-------------------- src/conf/network_conf.h | 10 +-- src/libvirt_private.syms | 1 - src/network/bridge_driver.c | 17 ++-- src/parallels/parallels_network.c | 4 +- src/test/test_driver.c | 10 ++- 6 files changed, 99 insertions(+), 114 deletions(-)
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index d7c5dec..1fb06ef 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -504,6 +504,81 @@ virNetworkObjAssignDef(virNetworkObjPtr network, }
/* + * If flags & VIR_NETWORK_OBJ_LIST_ADD_CHECK_LIVE then this will + * refuse updating an existing def if the current def is Live
s/Live/live (or active or already running)
+ * + * If flags & VIR_NETWORK_OBJ_LIST_ADD_LIVE then the @def being + * added is assumed to represent a live config, not a future + * inactive config
* If no flags is 0....
+ */ +static virNetworkObjPtr +virNetworkAssignDefLocked(virNetworkObjListPtr nets, + virNetworkDefPtr def, + unsigned int flags) +{ + virNetworkObjPtr network; + virNetworkObjPtr ret = NULL; + char uuidstr[VIR_UUID_STRING_BUFLEN]; + + /* See if a network with matching UUID already exists */ + if ((network = virNetworkObjFindByUUIDLocked(nets, def->uuid))) { + virObjectLock(network); + /* UUID matches, but if names don't match, refuse it */ + if (STRNEQ(network->def->name, def->name)) { + virUUIDFormat(network->def->uuid, uuidstr); + virReportError(VIR_ERR_OPERATION_FAILED, + _("network '%s' is already defined with uuid %s"), + network->def->name, uuidstr); + goto cleanup; + } + + if (flags & VIR_NETWORK_OBJ_LIST_ADD_CHECK_LIVE) { + /* UUID & name match, but if network is already active, refuse it */ + if (virNetworkObjIsActive(network)) { + virReportError(VIR_ERR_OPERATION_INVALID, + _("network is already active as '%s'"), + network->def->name); + goto cleanup; + } + } + + virNetworkObjAssignDef(network, + def, + !!(flags & VIR_NETWORK_OBJ_LIST_ADD_LIVE)); + } else { + /* UUID does not match, but if a name matches, refuse it */ + if ((network = virNetworkObjFindByNameLocked(nets, def->name))) { + virObjectLock(network); + virUUIDFormat(network->def->uuid, uuidstr); + virReportError(VIR_ERR_OPERATION_FAILED, + _("network '%s' already exists with uuid %s"), + def->name, uuidstr); + goto cleanup; + } + + if (!(network = virNetworkObjNew())) + goto cleanup; + + virObjectLock(network); + + virUUIDFormat(def->uuid, uuidstr); + if (virHashAddEntry(nets->objs, uuidstr, network) < 0) + goto cleanup; + + network->def = def; + network->persistent = !(flags & VIR_NETWORK_OBJ_LIST_ADD_LIVE); + virObjectRef(network); + } + + ret = network; + network = NULL; + + cleanup: + virNetworkObjEndAPI(&network); + return ret; +} + +/* * virNetworkAssignDef: * @nets: list of all networks * @def: the new NetworkDef (will be consumed by this function iff successful)
The next line here is "@live" which is now changed - need to adjust the comment here then too and of course describe @flags (including why someone would pass 0) and be sure the description is "valid"
@@ -519,40 +594,14 @@ virNetworkObjAssignDef(virNetworkObjPtr network, virNetworkObjPtr virNetworkAssignDef(virNetworkObjListPtr nets, virNetworkDefPtr def, - bool live) + unsigned int flags) { virNetworkObjPtr network; - char uuidstr[VIR_UUID_STRING_BUFLEN];
virObjectLock(nets); - if ((network = virNetworkObjFindByNameLocked(nets, def->name))) { - virObjectUnlock(nets); - virObjectLock(network); - virNetworkObjAssignDef(network, def, live); - return network; - } - - if (!(network = virNetworkObjNew())) { - virObjectUnlock(nets); - return NULL; - } - virObjectLock(network); - - virUUIDFormat(def->uuid, uuidstr); - if (virHashAddEntry(nets->objs, uuidstr, network) < 0) - goto error; - - network->def = def; - network->persistent = !live; - virObjectRef(network); + network = virNetworkAssignDefLocked(nets, def, flags); virObjectUnlock(nets); return network; - - error: - virObjectUnlock(nets); - virNetworkObjEndAPI(&network); - return NULL; - }
/* @@ -3005,7 +3054,7 @@ virNetworkLoadState(virNetworkObjListPtr nets, }
/* create the object */ - if (!(net = virNetworkAssignDef(nets, def, true))) + if (!(net = virNetworkAssignDef(nets, def, VIR_NETWORK_OBJ_LIST_ADD_LIVE))) goto error; /* do not put any "goto error" below this comment */
@@ -3082,7 +3131,7 @@ virNetworkObjPtr virNetworkLoadConfig(virNetworkObjListPtr nets, def->mac_specified = false; }
- if (!(net = virNetworkAssignDef(nets, def, false))) + if (!(net = virNetworkAssignDef(nets, def, 0))) goto error;
net->autostart = autostart; @@ -4295,68 +4344,6 @@ virNetworkObjUpdate(virNetworkObjPtr network, return ret; }
-/* - * virNetworkObjIsDuplicate: - * @nets : virNetworkObjListPtr to search - * @def : virNetworkDefPtr definition of network to lookup - * @check_active: If true, ensure that network is not active - * - * Returns: -1 on error - * 0 if network is new - * 1 if network is a duplicate - */ -int -virNetworkObjIsDuplicate(virNetworkObjListPtr nets, - virNetworkDefPtr def, - bool check_active) -{ - int ret = -1; - virNetworkObjPtr net = NULL; - - /* See if a network with matching UUID already exists */ - net = virNetworkObjFindByUUID(nets, def->uuid); - if (net) { - /* UUID matches, but if names don't match, refuse it */ - if (STRNEQ(net->def->name, def->name)) { - char uuidstr[VIR_UUID_STRING_BUFLEN]; - virUUIDFormat(net->def->uuid, uuidstr); - virReportError(VIR_ERR_OPERATION_FAILED, - _("network '%s' is already defined with uuid %s"), - net->def->name, uuidstr); - goto cleanup; - } - - if (check_active) { - /* UUID & name match, but if network is already active, refuse it */ - if (virNetworkObjIsActive(net)) { - virReportError(VIR_ERR_OPERATION_INVALID, - _("network is already active as '%s'"), - net->def->name); - goto cleanup; - } - } - - ret = 1; - } else { - /* UUID does not match, but if a name matches, refuse it */ - net = virNetworkObjFindByName(nets, def->name); - if (net) { - char uuidstr[VIR_UUID_STRING_BUFLEN]; - virUUIDFormat(net->def->uuid, uuidstr); - virReportError(VIR_ERR_OPERATION_FAILED, - _("network '%s' already exists with uuid %s"), - def->name, uuidstr); - goto cleanup; - } - ret = 0; - } - - cleanup: - virNetworkObjEndAPI(&net); - return ret; -} - - #define MATCH(FLAG) (flags & (FLAG)) static bool virNetworkMatch(virNetworkObjPtr netobj, diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h index 3e926f7..f69d999 100644 --- a/src/conf/network_conf.h +++ b/src/conf/network_conf.h @@ -316,9 +316,13 @@ void virNetworkDefFree(virNetworkDefPtr def); typedef bool (*virNetworkObjListFilter)(virConnectPtr conn, virNetworkDefPtr def);
+enum { + VIR_NETWORK_OBJ_LIST_ADD_LIVE = (1 << 0), + VIR_NETWORK_OBJ_LIST_ADD_CHECK_LIVE = (1 << 1), +};
Not that I'm any better at names, but the two names are very close to each other, but the CHECK_LIVE seems to me to say it's only a CHECK. When looking at the code/usage it seems that it's not allowing a redefinition if the uuid/name match - so perhaps use FAIL_LIVE_REDEFINE (or something like that)?
Although it seems
0 == PERSISTENT 1 = TRANSIENT 2 = DO NOT REPLACE ALREADY DEFINED NETWORK
In any case, ACK-able with updating at least the descriptions. Changing the enum/flag names is up to you.
Thanks. I've kept the old names to keep consistency with domain_conf.c counterpart. It's gonna be easier later, when we will merge vir{Domain,Network,...}ObjList into virObjectList. Michal