On 04/25/2014 01:55 PM, Laine Stump wrote:
> On 04/24/2014 07:09 PM, John Ferlan wrote:
>>
>> On 04/23/2014 09:49 AM, Laine Stump wrote:
>>> Experimentation showed that if virNetworkCreateXML() was called for a
>>> network that was already defined, and then the network was
>>> subsequently shutdown, the network would continue to be persistent
>>> after the shutdown (expected/desired), but the original config would
>>> be lost in favor of the transient config sent in with
>>> virNetworkCreateXML() (which would then be the new persistent config)
>>> (obviously unexpected/not desired).
>>>
>>> To fix this, virNetworkObjAssignDef() has been changed to
>>>
>>> 1) properly save/free network->def and network->newDef for all the
>>> various combinations of live/active/persistent, including some
>>> combinations that were previously considered to be an error but didn't
>>> need to be (e.g. setting a "live" config for a network that isn't yet
>>> active but soon will be - that was previously considered an error,
>>> even though in practice it can be very useful).
>>>
>>> 2) automatically set the persistent flag whenever a new non-live
>>> config is assigned to the network (and clear it when the non-live
>>> config is set to NULL). the libvirt network driver no longer directly
>>> manipulates network->persistent, but instead relies entirely on
>>> virNetworkObjAssignDef() to do the right thing automatically.
>>>
>>> After this patch, the following sequence will behave as expected:
>>>
>>> virNetworkDefineXML(X)
>>> virNetworkCreateXML(X') (same name but some config different)
>>> virNetworkDestroy(X)
>>>
>>> At the end of these calls, the network config will remain as it was
>>> after the initial virNetworkDefine(), whereas previously it would take
>>> on the changes given during virNetworkCreateXML().
>>>
>>> Another effect of this tighter coupling between a) setting a !live def
>>> and b) setting/clearing the "persistent" flag, is that future patches
>>> which change the details of network lifecycle management
>>> (e.g. upcoming patches to fix detection of "active" networks when
>>> libvirtd is restarted) will find it much more difficult to break
>>> persistence functionality.
>>> ---
>>> src/conf/network_conf.c | 78 +++++++++++++++++++++++----------------
>>> src/conf/network_conf.h | 6 +--
>>> src/network/bridge_driver.c | 34 +++++++----------
>>> src/parallels/parallels_network.c | 4 +-
>>> src/test/test_driver.c | 5 +--
>>> 5 files changed, 64 insertions(+), 63 deletions(-)
>>>
>>> diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
>>> index 56c4a09..66945ce 100644
>>> --- a/src/conf/network_conf.c
>>> +++ b/src/conf/network_conf.c
>>> @@ -302,46 +302,63 @@ void virNetworkObjListFree(virNetworkObjListPtr nets)
>>> /*
>>> * virNetworkObjAssignDef:
>>> * @network: the network object to update
>>> - * @def: the new NetworkDef (will be consumed by this function iff successful)
>>> + * @def: the new NetworkDef (will be consumed by this function)
>>> * @live: is this new def the "live" version, or the "persistent" version
>>> *
>>> - * Replace the appropriate copy of the given network's NetworkDef
>>> + * Replace the appropriate copy of the given network's def or newDef
>>> * with def. Use "live" and current state of the network to determine
>>> - * which to replace.
>>> + * which to replace and what to do with the old defs. When a non-live
>>> + * def is set, indicate that the network is now persistent.
>>> + *
>>> + * NB: a persistent network can be made transient by calling with:
>>> + * virNetworkObjAssignDef(network, NULL, false) (i.e. set the
>>> + * persistent def to NULL)
>>> *
>>> * Returns 0 on success, -1 on failure.
>>> */
>>> -int
>>> +void
>> Well this doesn't jive with the comment above about what it returns...
>
> Yeah, when all the failure paths disappeared I changed the function to
> void, but forgot to remove that comment. I'll fix that now.
>
>>> virNetworkObjAssignDef(virNetworkObjPtr network,
>>> virNetworkDefPtr def,
>>> bool live)
>>> {
>>> - if (virNetworkObjIsActive(network)) {
>>> - if (live) {
>>> + if (live) {
>>> + /* before setting new live def, save (into newDef) any
>>> + * existing persistent (!live) def to be restored when the
>>> + * network is destroyed, unless there is one already saved.
>>> + */
>>> + if (network->persistent && !network->newDef)
>>> + network->newDef = network->def;
>>> + else
>>> virNetworkDefFree(network->def);
>>> - network->def = def;
>>> - } else if (network->persistent) {
>>> - /* save current configuration to be restored on network shutdown */
>>> - virNetworkDefFree(network->newDef);
>>> + network->def = def;
>>> + } else { /* !live */
>>> + virNetworkDefFree(network->newDef);
>>> + if (virNetworkObjIsActive(network)) {
>>> + /* save new configuration to be restored on network
>>> + * shutdown, leaving current live def alone
>>> + */
>>> network->newDef = def;
>>> - } else {
>>> - virReportError(VIR_ERR_OPERATION_INVALID,
>>> - _("cannot save persistent config of transient "
>>> - "network '%s'"), network->def->name);
>>> - return -1;
>>> + } else { /* !live and !active */
>>> + if (network->def && !network->persistent) {
>>> + /* network isn't (yet) marked active or persistent,
>>> + * but already has a "live" def set. This means we are
>>> + * currently setting the persistent def as a part of
>>> + * the process of starting the network, so we need to
>>> + * preserve the "not yet live" def in network->def.
>>> + */
>>> + network->newDef = def;
>>> + } else {
>>> + /* either there is no live def set, or this network
>>> + * was already set as persistent, so the proper thing
>>> + * is to overwrite network->def.
>>> + */
>>> + network->newDef = NULL;
>> If I'm reading correctly - the newDef is virNetworkDefFree()'d above as
>> we enter the "} else { /* !live */" clause, which is probably where this
>> statement should be moved.
>
> No, I just did some reduction of common code by moving it above the if
> statement. You'll notice that network->newDef is set to *something* in
> every path through those ifs and elses, so at the end of the function,
> it is either NULL or pointing to a valid object.
>
>>
>>> + virNetworkDefFree(network->def);
>>> + network->def = def;
>>
>> NOTE: I think this is what is causing issues in networkUndefine() -
>> that'll call this function with '!live' - thus you'll do the free the
>> 'def' and set it to NULL... Back in the caller there's all sorts of
>> touching of the network->def-> structure
>
> Yes, indirectly. But the *real* problem is that we shouldn't be calling
> this function until after we're finished using anything in network->def.
> See below for the small diff I'm squashing in to fix this.
>
>>
>>
>>> + }
>>> }
>>> - } else if (!live) {
>>> - virNetworkDefFree(network->newDef);
>>> - virNetworkDefFree(network->def);
>>> - network->newDef = NULL;
>>> - network->def = def;
>>> - } else {
>>> - virReportError(VIR_ERR_OPERATION_INVALID,
>>> - _("cannot save live config of inactive "
>>> - "network '%s'"), network->def->name);
>>> - return -1;
>>> + network->persistent = !!def;
>>> }
>>> - return 0;
>>> }
>>>
>>> /*
>>> @@ -365,10 +382,7 @@ virNetworkAssignDef(virNetworkObjListPtr nets,
>>> virNetworkObjPtr network;
>>>
>>> if ((network = virNetworkFindByName(nets, def->name))) {
>>> - if (virNetworkObjAssignDef(network, def, live) < 0) {
>>> - virNetworkObjUnlock(network);
>>> - return NULL;
>>> - }
>>> + virNetworkObjAssignDef(network, def, live);
>>> return network;
>>> }
>>>
>>> @@ -392,8 +406,9 @@ virNetworkAssignDef(virNetworkObjListPtr nets,
>>> ignore_value(virBitmapSetBit(network->class_id, 2));
>>>
>>> network->def = def;
>>> -
>>> + network->persistent = !live;
>>> return network;
>>> +
>>> error:
>>> virNetworkObjUnlock(network);
>>> virNetworkObjFree(network);
>>> @@ -3118,7 +3133,6 @@ virNetworkObjPtr virNetworkLoadConfig(virNetworkObjListPtr nets,
>>> goto error;
>>>
>>> net->autostart = autostart;
>>> - net->persistent = 1;
>>>
>>> VIR_FREE(configFile);
>>> VIR_FREE(autostartLink);
>>> diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h
>>> index 1a864de..90da16f 100644
>>> --- a/src/conf/network_conf.h
>>> +++ b/src/conf/network_conf.h
>>> @@ -335,9 +335,9 @@ typedef bool (*virNetworkObjListFilter)(virConnectPtr conn,
>>> virNetworkObjPtr virNetworkAssignDef(virNetworkObjListPtr nets,
>>> virNetworkDefPtr def,
>>> bool live);
>>> -int virNetworkObjAssignDef(virNetworkObjPtr network,
>>> - virNetworkDefPtr def,
>>> - bool live);
>>> +void virNetworkObjAssignDef(virNetworkObjPtr network,
>>> + virNetworkDefPtr def,
>>> + bool live);
>>> int virNetworkObjSetDefTransient(virNetworkObjPtr network, bool live);
>>> void virNetworkObjUnsetDefTransient(virNetworkObjPtr network);
>>> virNetworkDefPtr virNetworkObjGetPersistentDef(virNetworkObjPtr network);
>>> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
>>> index eb276cd..62d1c25 100644
>>> --- a/src/network/bridge_driver.c
>>> +++ b/src/network/bridge_driver.c
>>> @@ -2667,10 +2667,11 @@ static virNetworkPtr networkCreateXML(virConnectPtr conn, const char *xml)
>>> if (networkValidate(driver, def, true) < 0)
>>> goto cleanup;
>>>
>>> - /* NB: "live" is false because this transient network hasn't yet
>>> - * been started
>>> + /* 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, false)))
>>> + if (!(network = virNetworkAssignDef(&driver->networks, def, true)))
>>> goto cleanup;
>>> def = NULL;
>>>
>>> @@ -2719,19 +2720,10 @@ static virNetworkPtr networkDefineXML(virConnectPtr conn, const char *xml)
>>> if (networkValidate(driver, def, false) < 0)
>>> goto cleanup;
>>>
>>> - if ((network = virNetworkFindByName(&driver->networks, def->name))) {
>>> - network->persistent = 1;
>>> - if (virNetworkObjAssignDef(network, def, false) < 0)
>>> - goto cleanup;
>>> - } else {
>>> - if (!(network = virNetworkAssignDef(&driver->networks, def, false)))
>>> - goto cleanup;
>>> - }
>>> -
>>> - /* define makes the network persistent - always */
>>> - network->persistent = 1;
>>> + if (!(network = virNetworkAssignDef(&driver->networks, def, false)))
>>> + goto cleanup;
>>>
>>> - /* def was asigned */
>>> + /* def was assigned to network object */
>>> freeDef = false;
>>>
>>> if (virNetworkSaveConfig(driver->networkConfigDir, def) < 0) {
>>> @@ -2740,9 +2732,11 @@ static virNetworkPtr networkDefineXML(virConnectPtr conn, const char *xml)
>>> network = NULL;
>>> goto cleanup;
>>> }
>>> - network->persistent = 0;
>>> - virNetworkDefFree(network->newDef);
>>> - network->newDef = NULL;
>>> + /* if network was active already, just undo new persistent
>>> + * definition by making it transient.
>>> + * XXX - this isn't necessarily the correct thing to do.
>>> + */
>> Leaving the XXX comment in here leaves me wondering what is the correct
>> thing to do...
>
> The correct thing to do is... well, to exactly rollback to what you had
> before the failure.
>
> However, the current code (both before *and* after this patch) does the
> (same) wrong thing in this case - if you fail during an attempt to
> redefine an already persistent+active network, you'll end up with a
> transient+active network (so it will disappear as soon as the network is
> stopped).
>
> It would be better if such a failure caused us to revert back to the
> previous persistent def. That would require saving off the existing def
> before trying to assign the new one or writing to disk. But that is a
> separate problem and should be in a separate patch anyway. The only
> reason I touched that area of the code in this patch was that I was
> replacing direct assignment of network->persistent with calling
> virNetworkObjAssignDef() (while preserving the existing functionality).
> I did think it was worth noting the problem with the existing
> functionality while touching the code, however.
>
> So, while I think this needs fixing, it's pre-existing and I'd rather
> not have these other patches delayed because of it.
>
>>
>>> + virNetworkObjAssignDef(network, NULL, false);
>>> goto cleanup;
>>> }
>>>
>>> @@ -2794,10 +2788,8 @@ networkUndefine(virNetworkPtr net)
>>> goto cleanup;
>>>
>>> /* make the network transient */
>>> - network->persistent = 0;
>>> network->autostart = 0;
>>> - virNetworkDefFree(network->newDef);
>>> - network->newDef = NULL;
>>> + virNetworkObjAssignDef(network, NULL, false);
>> This seems to be the cause of the crash I noted in private chat. If I
>> restore just these lines and not make the ObjAssignDef() call, then
>> things work again. That includes issues I was seeing in virt-test for
>> patches 3 & 4 as well.
>
> Yep. That call needs to be moved down to below the call to
> networkRemoveInactive. I've done that and all the tests complete without
> failure (as you've also separately reported on IRC). Here is what I
> squashed into this patch:
>
> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
> index 309d967..2a54aa3 100644
> --- a/src/network/bridge_driver.c
> +++ b/src/network/bridge_driver.c
> @@ -2899,14 +2899,12 @@ networkUndefine(virNetworkPtr net)
> if (virNetworkObjIsActive(network))
> active = true;
>
> + /* remove autostart link */
> if (virNetworkDeleteConfig(driver->networkConfigDir,
> driver->networkAutostartDir,
> network) < 0)
> goto cleanup;
> -
> - /* make the network transient */
> network->autostart = 0;
> - virNetworkObjAssignDef(network, NULL, false);
>
> event = virNetworkEventLifecycleNew(network->def->name,
> network->def->uuid,
> @@ -2920,6 +2918,12 @@ networkUndefine(virNetworkPtr net)
> goto cleanup;
> }
> network = NULL;
> + } else {
> +
> + /* if the network still exists, it was active, and we need to make
> + * it transient (by deleting the persistent def
s/def/def)/
> + */
> + virNetworkObjAssignDef(network, NULL, false);
> }
>
> ret = 0;
> --
>
>>
>> Just figured I'd close the loop on this review at least...
>
> Do my responses clear up your concerns?
>
>
>
Yep - all set, thanks for the extra details...
You have an ACK with that extra ) above
John
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list