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

Michal Privoznik mprivozn at redhat.com
Mon Mar 23 09:05:38 UTC 2015


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 at 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




More information about the libvir-list mailing list