Re: [libvirt] [PATCH 3/3] network_conf: Drop virNetworkObjIsDuplicate

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(a)redhat.com&gt; > --- > 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.