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@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.
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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list