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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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




[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]