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@xxxxxxxxxx>
> ---
> 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.
John
> virNetworkObjPtr virNetworkAssignDef(virNetworkObjListPtr nets,
> virNetworkDefPtr def,
> - bool live);
> + unsigned int flags);
> void virNetworkObjAssignDef(virNetworkObjPtr network,
> virNetworkDefPtr def,
> bool live);
> @@ -414,10 +418,6 @@ virNetworkObjUpdate(virNetworkObjPtr obj,
> const char *xml,
> unsigned int flags); /* virNetworkUpdateFlags */
>
> -int virNetworkObjIsDuplicate(virNetworkObjListPtr nets,
> - virNetworkDefPtr def,
> - bool check_active);
> -
> VIR_ENUM_DECL(virNetworkForward)
>
> # define VIR_CONNECT_LIST_NETWORKS_FILTERS_ACTIVE \
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 1fb42ac..02edb75 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -573,7 +573,6 @@ virNetworkObjFindByNameLocked;
> virNetworkObjFindByUUID;
> virNetworkObjFindByUUIDLocked;
> virNetworkObjGetPersistentDef;
> -virNetworkObjIsDuplicate;
> virNetworkObjListExport;
> virNetworkObjListForEach;
> virNetworkObjListGetNames;
> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
> index b3dcadc..c3bc6fe 100644
> --- a/src/network/bridge_driver.c
> +++ b/src/network/bridge_driver.c
> @@ -2748,8 +2748,7 @@ static int networkIsPersistent(virNetworkPtr net)
>
> static int
> networkValidate(virNetworkDriverStatePtr driver,
> - virNetworkDefPtr def,
> - bool check_active)
> + virNetworkDefPtr def)
> {
> size_t i, j;
> bool vlanUsed, vlanAllowed, badVlanUse = false;
> @@ -2759,10 +2758,6 @@ networkValidate(virNetworkDriverStatePtr driver,
> bool bandwidthAllowed = true;
> bool usesInterface = false, usesAddress = false;
>
> - /* 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
> */
> @@ -2980,14 +2975,16 @@ static virNetworkPtr networkCreateXML(virConnectPtr conn, const char *xml)
> if (virNetworkCreateXMLEnsureACL(conn, def) < 0)
> goto cleanup;
>
> - if (networkValidate(driver, def, true) < 0)
> + if (networkValidate(driver, def) < 0)
> goto cleanup;
>
> /* NB: even though this transient network hasn't yet been started,
> * we assign the def with live = true in anticipation that it will
> * be started momentarily.
> */
> - if (!(network = virNetworkAssignDef(driver->networks, def, true)))
> + if (!(network = virNetworkAssignDef(driver->networks, def,
> + VIR_NETWORK_OBJ_LIST_ADD_LIVE |
> + VIR_NETWORK_OBJ_LIST_ADD_CHECK_LIVE)))
> goto cleanup;
> def = NULL;
>
> @@ -3028,10 +3025,10 @@ static virNetworkPtr networkDefineXML(virConnectPtr conn, const char *xml)
> if (virNetworkDefineXMLEnsureACL(conn, def) < 0)
> goto cleanup;
>
> - if (networkValidate(driver, def, false) < 0)
> + if (networkValidate(driver, def) < 0)
> goto cleanup;
>
> - if (!(network = virNetworkAssignDef(driver->networks, def, false)))
> + if (!(network = virNetworkAssignDef(driver->networks, def, 0)))
> goto cleanup;
>
> /* def was assigned to network object */
> diff --git a/src/parallels/parallels_network.c b/src/parallels/parallels_network.c
> index 1d3b694..0711558 100644
> --- a/src/parallels/parallels_network.c
> +++ b/src/parallels/parallels_network.c
> @@ -226,7 +226,7 @@ parallelsLoadNetwork(parallelsConnPtr privconn, virJSONValuePtr jobj)
> goto cleanup;
> }
>
> - if (!(net = virNetworkAssignDef(privconn->networks, def, false)))
> + if (!(net = virNetworkAssignDef(privconn->networks, def, 0)))
> goto cleanup;
> net->active = 1;
> net->autostart = 1;
> @@ -258,7 +258,7 @@ parallelsAddRoutedNetwork(parallelsConnPtr privconn)
> }
> def->uuid_specified = 1;
>
> - if (!(net = virNetworkAssignDef(privconn->networks, def, false))) {
> + if (!(net = virNetworkAssignDef(privconn->networks, def, 0))) {
> virNetworkDefFree(def);
> goto cleanup;
> }
> diff --git a/src/test/test_driver.c b/src/test/test_driver.c
> index 1c9d573..07cc032 100644
> --- a/src/test/test_driver.c
> +++ b/src/test/test_driver.c
> @@ -782,7 +782,7 @@ testOpenDefault(virConnectPtr conn)
>
> if (!(netdef = virNetworkDefParseString(defaultNetworkXML)))
> goto error;
> - if (!(netobj = virNetworkAssignDef(privconn->networks, netdef, false))) {
> + if (!(netobj = virNetworkAssignDef(privconn->networks, netdef, 0))) {
> virNetworkDefFree(netdef);
> goto error;
> }
> @@ -1149,7 +1149,7 @@ testParseNetworks(testConnPtr privconn,
> if (!def)
> goto error;
>
> - if (!(obj = virNetworkAssignDef(privconn->networks, def, false))) {
> + if (!(obj = virNetworkAssignDef(privconn->networks, def, 0))) {
> virNetworkDefFree(def);
> goto error;
> }
> @@ -3627,7 +3627,9 @@ static virNetworkPtr testNetworkCreateXML(virConnectPtr conn, const char *xml)
> if ((def = virNetworkDefParseString(xml)) == NULL)
> goto cleanup;
>
> - if (!(net = virNetworkAssignDef(privconn->networks, def, true)))
> + if (!(net = virNetworkAssignDef(privconn->networks, def,
> + VIR_NETWORK_OBJ_LIST_ADD_LIVE |
> + VIR_NETWORK_OBJ_LIST_ADD_CHECK_LIVE)))
> goto cleanup;
> def = NULL;
> net->active = 1;
> @@ -3658,7 +3660,7 @@ virNetworkPtr testNetworkDefineXML(virConnectPtr conn, const char *xml)
> if ((def = virNetworkDefParseString(xml)) == NULL)
> goto cleanup;
>
> - if (!(net = virNetworkAssignDef(privconn->networks, def, false)))
> + if (!(net = virNetworkAssignDef(privconn->networks, def, 0)))
> goto cleanup;
> def = NULL;
>
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list