[libvirt] [PATCH 00/17] nwfilter adjustments for common object
As noted in the recently posted virObject changes: https://www.redhat.com/archives/libvir-list/2017-June/msg00070.html I believe I've found a way to handle the recursive lock situation that made it "difficult" (at best) to convert the nwfilter to the common object model. It does involve a bit of a circuitous route to "temporarily implement" the refcnt in nwfilter, but that gets removed rather quickly. Beyond that there's a bit of setup, the first few patches deal with issues seen while working through this code and then more setup for getting things to be more common with other drivers (so when they disappear a bit further into the future) into some new object it'll be obvious where/why they're there. The middle few patches deal with an insane naming scheme for a single function that seemed to keep prefixing "_" as a new function was created. So it's a bit of name change, but makes it easier to think about. The last 4 patches deal with the conversion to use a @ref, a change to a list locking hash table model, the modificiation of the recursive instantiation to use @refs rather than @locks, and finally the change to use the existing lockable object. I have run these through the various avacodo nwfilter tests that I could find, but perhaps if someone that had a more "robust configuration" and wants to be a bit adventurous and also give the patches a whirl - that would be appreciated. John Ferlan (17): nwfilter: Fix return value comparison for virNWFilterTriggerVMFilterRebuild nwfilter: Fix possible corruption on failure path during LoadConfig nwfilter: Fix possible locking problem in LoadConfig error path nwfilter: Remove need for virNWFilterSaveXML nwfilter: Move virNWFilterSaveConfig virnwfilterobj nwfilter: Add configFile into virNWFilterObj nwfilter: Add @def into virNWFilterObjNew nwfilter: Clean up a couple nwfilter_driver error paths nwfilter: Consistently name virNWFilterPtr in driver nwfilter: Rename virNWFilterInstantiate nwfilter: Rename __virNWFilterInstantiateFilter nwfilter: Rename _virNWFilterInstantiateFilter nwfilter: Introduce virNWFilterObjListFindInstantiateFilter nwfilter: Add @refs logic to __virNWFilterObj nwfilter: Convert _virNWFilterObjList to be a virObjectLockable nwfilter: Remove recursive locking for nwfilter instantiation nwfilter: Convert virNWFilterObj to use virObjectLockable src/conf/nwfilter_conf.c | 43 --- src/conf/nwfilter_conf.h | 9 - src/conf/virnwfilterobj.c | 550 +++++++++++++++++++++++---------- src/conf/virnwfilterobj.h | 19 +- src/libvirt_private.syms | 6 +- src/nwfilter/nwfilter_driver.c | 51 ++- src/nwfilter/nwfilter_gentech_driver.c | 276 +++++++---------- 7 files changed, 542 insertions(+), 412 deletions(-) -- 2.9.4
Should compare < 0 to be correct. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virnwfilterobj.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/conf/virnwfilterobj.c b/src/conf/virnwfilterobj.c index b5aaa6b..0027d45 100644 --- a/src/conf/virnwfilterobj.c +++ b/src/conf/virnwfilterobj.c @@ -253,7 +253,7 @@ virNWFilterObjTestUnassignDef(virNWFilterObjPtr obj) obj->wantRemoved = true; /* trigger the update on VMs referencing the filter */ - if (virNWFilterTriggerVMFilterRebuild()) + if (virNWFilterTriggerVMFilterRebuild() < 0) rc = -1; obj->wantRemoved = false; @@ -345,7 +345,7 @@ virNWFilterObjListAssignDef(virNWFilterObjListPtr nwfilters, obj->newDef = def; /* trigger the update on VMs referencing the filter */ - if (virNWFilterTriggerVMFilterRebuild()) { + if (virNWFilterTriggerVMFilterRebuild() < 0) { obj->newDef = NULL; virNWFilterObjUnlock(obj); return NULL; -- 2.9.4
On 06/02/2017 12:25 PM, John Ferlan wrote:
Should compare < 0 to be correct.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virnwfilterobj.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
ACK. Trivial. Michal
If the virNWFilterSaveConfig in virNWFilterObjListLoadConfig, then jumping to the error label would free the @def owned by the object, but the object is still on the list. Fix this by following proper procedure to clear @def and create a new variable @objdef to handle the object's def after successful assignment. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virnwfilterobj.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/conf/virnwfilterobj.c b/src/conf/virnwfilterobj.c index 0027d45..3fb6046 100644 --- a/src/conf/virnwfilterobj.c +++ b/src/conf/virnwfilterobj.c @@ -485,6 +485,7 @@ virNWFilterObjListLoadConfig(virNWFilterObjListPtr nwfilters, { virNWFilterDefPtr def = NULL; virNWFilterObjPtr obj; + virNWFilterDefPtr objdef; char *configFile = NULL; if (!(configFile = virFileBuildPath(configDir, name, ".xml"))) @@ -503,10 +504,12 @@ virNWFilterObjListLoadConfig(virNWFilterObjListPtr nwfilters, if (!(obj = virNWFilterObjListAssignDef(nwfilters, def))) goto error; + def = NULL; + objdef = obj->def; /* We generated a UUID, make it permanent by saving the config to disk */ - if (!def->uuid_specified && - virNWFilterSaveConfig(configDir, def) < 0) + if (!objdef->uuid_specified && + virNWFilterSaveConfig(configDir, objdef) < 0) goto error; VIR_FREE(configFile); -- 2.9.4
On 06/02/2017 12:25 PM, John Ferlan wrote:
If the virNWFilterSaveConfig in virNWFilterObjListLoadConfig, then jumping
s/,/ fails,/
to the error label would free the @def owned by the object, but the object is still on the list.
Fix this by following proper procedure to clear @def and create a new variable @objdef to handle the object's def after successful assignment.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virnwfilterobj.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/src/conf/virnwfilterobj.c b/src/conf/virnwfilterobj.c index 0027d45..3fb6046 100644 --- a/src/conf/virnwfilterobj.c +++ b/src/conf/virnwfilterobj.c @@ -485,6 +485,7 @@ virNWFilterObjListLoadConfig(virNWFilterObjListPtr nwfilters, { virNWFilterDefPtr def = NULL; virNWFilterObjPtr obj; + virNWFilterDefPtr objdef; char *configFile = NULL;
if (!(configFile = virFileBuildPath(configDir, name, ".xml"))) @@ -503,10 +504,12 @@ virNWFilterObjListLoadConfig(virNWFilterObjListPtr nwfilters,
if (!(obj = virNWFilterObjListAssignDef(nwfilters, def))) goto error; + def = NULL; + objdef = obj->def;
/* We generated a UUID, make it permanent by saving the config to disk */ - if (!def->uuid_specified && - virNWFilterSaveConfig(configDir, def) < 0) + if (!objdef->uuid_specified && + virNWFilterSaveConfig(configDir, objdef) < 0) goto error;
This @objdef variable looks redundant to me. Can't we access obj->def directly? Esp. since you're introducing a variable just for a two times use. Then again, we often use obj->def->... when it comes to domain objects, why not here? ACK if you drop the @objdef variable. Michal
On 07/13/2017 10:41 AM, Michal Privoznik wrote:
On 06/02/2017 12:25 PM, John Ferlan wrote:
If the virNWFilterSaveConfig in virNWFilterObjListLoadConfig, then jumping
s/,/ fails,/
to the error label would free the @def owned by the object, but the object is still on the list.
Fix this by following proper procedure to clear @def and create a new variable @objdef to handle the object's def after successful assignment.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virnwfilterobj.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/src/conf/virnwfilterobj.c b/src/conf/virnwfilterobj.c index 0027d45..3fb6046 100644 --- a/src/conf/virnwfilterobj.c +++ b/src/conf/virnwfilterobj.c @@ -485,6 +485,7 @@ virNWFilterObjListLoadConfig(virNWFilterObjListPtr nwfilters, { virNWFilterDefPtr def = NULL; virNWFilterObjPtr obj; + virNWFilterDefPtr objdef; char *configFile = NULL;
if (!(configFile = virFileBuildPath(configDir, name, ".xml"))) @@ -503,10 +504,12 @@ virNWFilterObjListLoadConfig(virNWFilterObjListPtr nwfilters,
if (!(obj = virNWFilterObjListAssignDef(nwfilters, def))) goto error; + def = NULL; + objdef = obj->def;
/* We generated a UUID, make it permanent by saving the config to disk */ - if (!def->uuid_specified && - virNWFilterSaveConfig(configDir, def) < 0) + if (!objdef->uuid_specified && + virNWFilterSaveConfig(configDir, objdef) < 0) goto error;
This @objdef variable looks redundant to me. Can't we access obj->def directly? Esp. since you're introducing a variable just for a two times use. Then again, we often use obj->def->... when it comes to domain objects, why not here?
If obj->def ever gets "consumed" by the virObject code, then obj->def->x will fail miserably. That was the original design goal. I've since had to scale back. I guess I could do "obj->def->uuid_specified" as it doesn't really matter until the day obj->def-> is no longer accessible. As a preference - I like the extra local variable. In the long run the compiler will do away with it, but for me it just reads better that way. The deeper one gets into a->b->c->d[->e...] the more insane it gets. Forcing one level of indirection is just more readable to me. John
ACK if you drop the @objdef variable.
Michal
On 07/14/2017 01:50 AM, John Ferlan wrote:
On 07/13/2017 10:41 AM, Michal Privoznik wrote:
On 06/02/2017 12:25 PM, John Ferlan wrote:
If the virNWFilterSaveConfig in virNWFilterObjListLoadConfig, then jumping
s/,/ fails,/
to the error label would free the @def owned by the object, but the object is still on the list.
Fix this by following proper procedure to clear @def and create a new variable @objdef to handle the object's def after successful assignment.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virnwfilterobj.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/src/conf/virnwfilterobj.c b/src/conf/virnwfilterobj.c index 0027d45..3fb6046 100644 --- a/src/conf/virnwfilterobj.c +++ b/src/conf/virnwfilterobj.c @@ -485,6 +485,7 @@ virNWFilterObjListLoadConfig(virNWFilterObjListPtr nwfilters, { virNWFilterDefPtr def = NULL; virNWFilterObjPtr obj; + virNWFilterDefPtr objdef; char *configFile = NULL;
if (!(configFile = virFileBuildPath(configDir, name, ".xml"))) @@ -503,10 +504,12 @@ virNWFilterObjListLoadConfig(virNWFilterObjListPtr nwfilters,
if (!(obj = virNWFilterObjListAssignDef(nwfilters, def))) goto error; + def = NULL; + objdef = obj->def;
/* We generated a UUID, make it permanent by saving the config to disk */ - if (!def->uuid_specified && - virNWFilterSaveConfig(configDir, def) < 0) + if (!objdef->uuid_specified && + virNWFilterSaveConfig(configDir, objdef) < 0) goto error;
This @objdef variable looks redundant to me. Can't we access obj->def directly? Esp. since you're introducing a variable just for a two times use. Then again, we often use obj->def->... when it comes to domain objects, why not here?
If obj->def ever gets "consumed" by the virObject code, then obj->def->x will fail miserably. That was the original design goal. I've since had to scale back. I guess I could do "obj->def->uuid_specified" as it doesn't really matter until the day obj->def-> is no longer accessible.
As a preference - I like the extra local variable. In the long run the compiler will do away with it, but for me it just reads better that way. The deeper one gets into a->b->c->d[->e...] the more insane it gets. Forcing one level of indirection is just more readable to me.
Well, then don't run the following: libvirt.git $ git grep -np "\(\w\+\(->\|\.\)\)\{10\}\w\+" I mean, that's one extreme, but we usually have "vm->def->name" so obj->def->uuid_specified shouldn't be a problem. But I don't care that much. So your call after all. Michal
On 07/14/2017 08:09 AM, Michal Privoznik wrote:
On 07/14/2017 01:50 AM, John Ferlan wrote:
On 07/13/2017 10:41 AM, Michal Privoznik wrote:
On 06/02/2017 12:25 PM, John Ferlan wrote:
If the virNWFilterSaveConfig in virNWFilterObjListLoadConfig, then jumping
s/,/ fails,/
to the error label would free the @def owned by the object, but the object is still on the list.
Fix this by following proper procedure to clear @def and create a new variable @objdef to handle the object's def after successful assignment.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virnwfilterobj.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/src/conf/virnwfilterobj.c b/src/conf/virnwfilterobj.c index 0027d45..3fb6046 100644 --- a/src/conf/virnwfilterobj.c +++ b/src/conf/virnwfilterobj.c @@ -485,6 +485,7 @@ virNWFilterObjListLoadConfig(virNWFilterObjListPtr nwfilters, { virNWFilterDefPtr def = NULL; virNWFilterObjPtr obj; + virNWFilterDefPtr objdef; char *configFile = NULL;
if (!(configFile = virFileBuildPath(configDir, name, ".xml"))) @@ -503,10 +504,12 @@ virNWFilterObjListLoadConfig(virNWFilterObjListPtr nwfilters,
if (!(obj = virNWFilterObjListAssignDef(nwfilters, def))) goto error; + def = NULL; + objdef = obj->def;
/* We generated a UUID, make it permanent by saving the config to disk */ - if (!def->uuid_specified && - virNWFilterSaveConfig(configDir, def) < 0) + if (!objdef->uuid_specified && + virNWFilterSaveConfig(configDir, objdef) < 0) goto error;
This @objdef variable looks redundant to me. Can't we access obj->def directly? Esp. since you're introducing a variable just for a two times use. Then again, we often use obj->def->... when it comes to domain objects, why not here?
If obj->def ever gets "consumed" by the virObject code, then obj->def->x will fail miserably. That was the original design goal. I've since had to scale back. I guess I could do "obj->def->uuid_specified" as it doesn't really matter until the day obj->def-> is no longer accessible.
As a preference - I like the extra local variable. In the long run the compiler will do away with it, but for me it just reads better that way. The deeper one gets into a->b->c->d[->e...] the more insane it gets. Forcing one level of indirection is just more readable to me.
Well, then don't run the following:
libvirt.git $ git grep -np "\(\w\+\(->\|\.\)\)\{10\}\w\+"
ewww...
I mean, that's one extreme, but we usually have "vm->def->name" so obj->def->uuid_specified shouldn't be a problem. But I don't care that much. So your call after all.
After reverting as I point out in patch 3, this patch is no longer necessary. So essentially patches 2 & 3 are replaced by the revert John
After returning from virNWFilterObjListAssignDef the @obj is locked; however, if virNWFilterSaveConfig fails to save the generated UUID the code jumped to error and returns NULL meaning the caller will not call virNWFilterObjUnlock on the object leaving the object locked on list and ripe for a deadlock on any subsequent Find of all objects or object removal. So rather than jumping to error alter the comment prior to the virNWFilterSaveConfig and just provide a VIR_INFO message for anyone that cares, but allow the code to continue and a subsequent LoadConfig to once again attempt the save of a newly generated UUID. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virnwfilterobj.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/conf/virnwfilterobj.c b/src/conf/virnwfilterobj.c index 3fb6046..0343c0a 100644 --- a/src/conf/virnwfilterobj.c +++ b/src/conf/virnwfilterobj.c @@ -507,10 +507,13 @@ virNWFilterObjListLoadConfig(virNWFilterObjListPtr nwfilters, def = NULL; objdef = obj->def; - /* We generated a UUID, make it permanent by saving the config to disk */ + /* We generated a UUID, atttempt to make it permanent by saving the + * config to disk. If not successful, no need to fail or remove the + * object as a future load would regenerate a UUID and try again, + * but the existing config would still exist and can be used. */ if (!objdef->uuid_specified && virNWFilterSaveConfig(configDir, objdef) < 0) - goto error; + VIR_INFO("failed to save generated UUID for filter '%s'", objdef->name); VIR_FREE(configFile); return obj; -- 2.9.4
On 06/02/2017 12:25 PM, John Ferlan wrote:
After returning from virNWFilterObjListAssignDef the @obj is locked; however, if virNWFilterSaveConfig fails to save the generated UUID the code jumped to error and returns NULL meaning the caller will not call virNWFilterObjUnlock on the object leaving the object locked on list and ripe for a deadlock on any subsequent Find of all objects or object removal.
So rather than jumping to error alter the comment prior to the virNWFilterSaveConfig and just provide a VIR_INFO message for anyone that cares, but allow the code to continue and a subsequent LoadConfig to once again attempt the save of a newly generated UUID.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virnwfilterobj.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/src/conf/virnwfilterobj.c b/src/conf/virnwfilterobj.c index 3fb6046..0343c0a 100644 --- a/src/conf/virnwfilterobj.c +++ b/src/conf/virnwfilterobj.c @@ -507,10 +507,13 @@ virNWFilterObjListLoadConfig(virNWFilterObjListPtr nwfilters, def = NULL; objdef = obj->def;
- /* We generated a UUID, make it permanent by saving the config to disk */ + /* We generated a UUID, atttempt to make it permanent by saving the
s/ttt/tt/
+ * config to disk. If not successful, no need to fail or remove the + * object as a future load would regenerate a UUID and try again, + * but the existing config would still exist and can be used. */ if (!objdef->uuid_specified && virNWFilterSaveConfig(configDir, objdef) < 0) - goto error; + VIR_INFO("failed to save generated UUID for filter '%s'", objdef->name);
VIR_FREE(configFile); return obj;
Well, frankly I'd say that we should not even try to save the config in the first place. Load() should really just load. We shouldn't try to "fix" XML configs at load time rather then when saving it in define phase. Michal
On 07/13/2017 10:41 AM, Michal Privoznik wrote:
On 06/02/2017 12:25 PM, John Ferlan wrote:
After returning from virNWFilterObjListAssignDef the @obj is locked; however, if virNWFilterSaveConfig fails to save the generated UUID the code jumped to error and returns NULL meaning the caller will not call virNWFilterObjUnlock on the object leaving the object locked on list and ripe for a deadlock on any subsequent Find of all objects or object removal.
So rather than jumping to error alter the comment prior to the virNWFilterSaveConfig and just provide a VIR_INFO message for anyone that cares, but allow the code to continue and a subsequent LoadConfig to once again attempt the save of a newly generated UUID.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virnwfilterobj.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/src/conf/virnwfilterobj.c b/src/conf/virnwfilterobj.c index 3fb6046..0343c0a 100644 --- a/src/conf/virnwfilterobj.c +++ b/src/conf/virnwfilterobj.c @@ -507,10 +507,13 @@ virNWFilterObjListLoadConfig(virNWFilterObjListPtr nwfilters, def = NULL; objdef = obj->def;
- /* We generated a UUID, make it permanent by saving the config to disk */ + /* We generated a UUID, atttempt to make it permanent by saving the
s/ttt/tt/
+ * config to disk. If not successful, no need to fail or remove the + * object as a future load would regenerate a UUID and try again, + * but the existing config would still exist and can be used. */ if (!objdef->uuid_specified && virNWFilterSaveConfig(configDir, objdef) < 0) - goto error; + VIR_INFO("failed to save generated UUID for filter '%s'", objdef->name);
VIR_FREE(configFile); return obj;
Well, frankly I'd say that we should not even try to save the config in the first place. Load() should really just load. We shouldn't try to "fix" XML configs at load time rather then when saving it in define phase.
IIRC: this one's a bit weird since if the UUID doesn't exist we "can" generate one. If we generate one, then we should save it. However, failing to save shouldn't be called an error because having a UUID isn't required it's just something we try to "enforce" at some point in time of the nwfilter. I kind of didn't want to "adjust" that logic. That's a different patch ;-> John
Michal
On 07/14/2017 01:52 AM, John Ferlan wrote:
On 07/13/2017 10:41 AM, Michal Privoznik wrote:
On 06/02/2017 12:25 PM, John Ferlan wrote:
After returning from virNWFilterObjListAssignDef the @obj is locked; however, if virNWFilterSaveConfig fails to save the generated UUID the code jumped to error and returns NULL meaning the caller will not call virNWFilterObjUnlock on the object leaving the object locked on list and ripe for a deadlock on any subsequent Find of all objects or object removal.
So rather than jumping to error alter the comment prior to the virNWFilterSaveConfig and just provide a VIR_INFO message for anyone that cares, but allow the code to continue and a subsequent LoadConfig to once again attempt the save of a newly generated UUID.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virnwfilterobj.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/src/conf/virnwfilterobj.c b/src/conf/virnwfilterobj.c index 3fb6046..0343c0a 100644 --- a/src/conf/virnwfilterobj.c +++ b/src/conf/virnwfilterobj.c @@ -507,10 +507,13 @@ virNWFilterObjListLoadConfig(virNWFilterObjListPtr nwfilters, def = NULL; objdef = obj->def;
- /* We generated a UUID, make it permanent by saving the config to disk */ + /* We generated a UUID, atttempt to make it permanent by saving the
s/ttt/tt/
+ * config to disk. If not successful, no need to fail or remove the + * object as a future load would regenerate a UUID and try again, + * but the existing config would still exist and can be used. */ if (!objdef->uuid_specified && virNWFilterSaveConfig(configDir, objdef) < 0) - goto error; + VIR_INFO("failed to save generated UUID for filter '%s'", objdef->name);
VIR_FREE(configFile); return obj;
Well, frankly I'd say that we should not even try to save the config in the first place. Load() should really just load. We shouldn't try to "fix" XML configs at load time rather then when saving it in define phase.
IIRC: this one's a bit weird since if the UUID doesn't exist we "can" generate one. If we generate one, then we should save it. However, failing to save shouldn't be called an error because having a UUID isn't required it's just something we try to "enforce" at some point in time of the nwfilter. I kind of didn't want to "adjust" that logic. That's a different patch ;->
Ah, so I've dug out the commit that introduced this behaviour: 441e881e. It's fairly recent and it indeed fixes a problem we have. Well, I still rather see it as a separate operation than during config load. It could run right after we load configs. But whatever. So the commit says there are troubles if we generate new UUIDs each time. If that's the case I don't think that failing to save should be ignored. It would lead to the same problem that the commit tried to fix, wouldn't it? Michal
On 07/14/2017 08:09 AM, Michal Privoznik wrote:
On 07/14/2017 01:52 AM, John Ferlan wrote:
On 07/13/2017 10:41 AM, Michal Privoznik wrote:
On 06/02/2017 12:25 PM, John Ferlan wrote:
After returning from virNWFilterObjListAssignDef the @obj is locked; however, if virNWFilterSaveConfig fails to save the generated UUID the code jumped to error and returns NULL meaning the caller will not call virNWFilterObjUnlock on the object leaving the object locked on list and ripe for a deadlock on any subsequent Find of all objects or object removal.
So rather than jumping to error alter the comment prior to the virNWFilterSaveConfig and just provide a VIR_INFO message for anyone that cares, but allow the code to continue and a subsequent LoadConfig to once again attempt the save of a newly generated UUID.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virnwfilterobj.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/src/conf/virnwfilterobj.c b/src/conf/virnwfilterobj.c index 3fb6046..0343c0a 100644 --- a/src/conf/virnwfilterobj.c +++ b/src/conf/virnwfilterobj.c @@ -507,10 +507,13 @@ virNWFilterObjListLoadConfig(virNWFilterObjListPtr nwfilters, def = NULL; objdef = obj->def;
- /* We generated a UUID, make it permanent by saving the config to disk */ + /* We generated a UUID, atttempt to make it permanent by saving the
s/ttt/tt/
+ * config to disk. If not successful, no need to fail or remove the + * object as a future load would regenerate a UUID and try again, + * but the existing config would still exist and can be used. */ if (!objdef->uuid_specified && virNWFilterSaveConfig(configDir, objdef) < 0) - goto error; + VIR_INFO("failed to save generated UUID for filter '%s'", objdef->name);
VIR_FREE(configFile); return obj;
Well, frankly I'd say that we should not even try to save the config in the first place. Load() should really just load. We shouldn't try to "fix" XML configs at load time rather then when saving it in define phase.
IIRC: this one's a bit weird since if the UUID doesn't exist we "can" generate one. If we generate one, then we should save it. However, failing to save shouldn't be called an error because having a UUID isn't required it's just something we try to "enforce" at some point in time of the nwfilter. I kind of didn't want to "adjust" that logic. That's a different patch ;->
Ah, so I've dug out the commit that introduced this behaviour: 441e881e. It's fairly recent and it indeed fixes a problem we have. Well, I still rather see it as a separate operation than during config load. It could run right after we load configs. But whatever.
So the commit says there are troubles if we generate new UUIDs each time. If that's the case I don't think that failing to save should be ignored. It would lead to the same problem that the commit tried to fix, wouldn't it?
Ahhh, I see. Interesting commit... Then of course there's b3e71a8830 which is the actual self inflicted shot. I need to revert that one ASAP (and do the same for the 3.3, 3.4, and 3.5 -maint trees because there's an awful double free bug for @def). John
Merge code into virNWFilterSaveConfig Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/nwfilter_conf.c | 30 +++++++----------------------- src/conf/nwfilter_conf.h | 5 ----- 2 files changed, 7 insertions(+), 28 deletions(-) diff --git a/src/conf/nwfilter_conf.c b/src/conf/nwfilter_conf.c index 032700c..fba792a 100644 --- a/src/conf/nwfilter_conf.c +++ b/src/conf/nwfilter_conf.c @@ -2767,13 +2767,16 @@ virNWFilterDefParseFile(const char *filename) int -virNWFilterSaveXML(const char *configDir, - virNWFilterDefPtr def, - const char *xml) +virNWFilterSaveConfig(const char *configDir, + virNWFilterDefPtr def) { + int ret = -1; + char *xml; char uuidstr[VIR_UUID_STRING_BUFLEN]; char *configFile = NULL; - int ret = -1; + + if (!(xml = virNWFilterDefFormat(def))) + goto cleanup; if (!(configFile = virFileBuildPath(configDir, def->name, ".xml"))) goto cleanup; @@ -2785,25 +2788,6 @@ virNWFilterSaveXML(const char *configDir, cleanup: VIR_FREE(configFile); - return ret; -} - - -int -virNWFilterSaveConfig(const char *configDir, - virNWFilterDefPtr def) -{ - int ret = -1; - char *xml; - - if (!(xml = virNWFilterDefFormat(def))) - goto cleanup; - - if (virNWFilterSaveXML(configDir, def, xml) < 0) - goto cleanup; - - ret = 0; - cleanup: VIR_FREE(xml); return ret; } diff --git a/src/conf/nwfilter_conf.h b/src/conf/nwfilter_conf.h index 5cac260..4bf5b7c 100644 --- a/src/conf/nwfilter_conf.h +++ b/src/conf/nwfilter_conf.h @@ -581,11 +581,6 @@ char * virNWFilterDefFormat(const virNWFilterDef *def); int -virNWFilterSaveXML(const char *configDir, - virNWFilterDefPtr def, - const char *xml); - -int virNWFilterSaveConfig(const char *configDir, virNWFilterDefPtr def); -- 2.9.4
On 06/02/2017 12:25 PM, John Ferlan wrote:
Merge code into virNWFilterSaveConfig
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/nwfilter_conf.c | 30 +++++++----------------------- src/conf/nwfilter_conf.h | 5 ----- 2 files changed, 7 insertions(+), 28 deletions(-)
ACK Michal
Move the function into nwfilterobj, rename it to virNWFilterObjSaveConfig, and alter the order of the arguments. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/nwfilter_conf.c | 27 --------------------------- src/conf/nwfilter_conf.h | 4 ---- src/conf/virnwfilterobj.c | 30 +++++++++++++++++++++++++++++- src/conf/virnwfilterobj.h | 4 ++++ src/libvirt_private.syms | 2 +- src/nwfilter/nwfilter_driver.c | 2 +- 6 files changed, 35 insertions(+), 34 deletions(-) diff --git a/src/conf/nwfilter_conf.c b/src/conf/nwfilter_conf.c index fba792a..bcff4b6 100644 --- a/src/conf/nwfilter_conf.c +++ b/src/conf/nwfilter_conf.c @@ -2766,33 +2766,6 @@ virNWFilterDefParseFile(const char *filename) } -int -virNWFilterSaveConfig(const char *configDir, - virNWFilterDefPtr def) -{ - int ret = -1; - char *xml; - char uuidstr[VIR_UUID_STRING_BUFLEN]; - char *configFile = NULL; - - if (!(xml = virNWFilterDefFormat(def))) - goto cleanup; - - if (!(configFile = virFileBuildPath(configDir, def->name, ".xml"))) - goto cleanup; - - virUUIDFormat(def->uuid, uuidstr); - ret = virXMLSaveFile(configFile, - virXMLPickShellSafeComment(def->name, uuidstr), - "nwfilter-edit", xml); - - cleanup: - VIR_FREE(configFile); - VIR_FREE(xml); - return ret; -} - - int nCallbackDriver; #define MAX_CALLBACK_DRIVER 10 static virNWFilterCallbackDriverPtr callbackDrvArray[MAX_CALLBACK_DRIVER]; diff --git a/src/conf/nwfilter_conf.h b/src/conf/nwfilter_conf.h index 4bf5b7c..ac6aee9 100644 --- a/src/conf/nwfilter_conf.h +++ b/src/conf/nwfilter_conf.h @@ -580,10 +580,6 @@ virNWFilterDefParseNode(xmlDocPtr xml, char * virNWFilterDefFormat(const virNWFilterDef *def); -int -virNWFilterSaveConfig(const char *configDir, - virNWFilterDefPtr def); - virNWFilterDefPtr virNWFilterDefParseString(const char *xml); diff --git a/src/conf/virnwfilterobj.c b/src/conf/virnwfilterobj.c index 0343c0a..5834b9d 100644 --- a/src/conf/virnwfilterobj.c +++ b/src/conf/virnwfilterobj.c @@ -478,6 +478,34 @@ virNWFilterObjListExport(virConnectPtr conn, } +int +virNWFilterObjSaveConfig(virNWFilterObjPtr obj, + const char *configDir) +{ + virNWFilterDefPtr def = obj->def; + int ret = -1; + char *xml; + char uuidstr[VIR_UUID_STRING_BUFLEN]; + char *configFile = NULL; + + if (!(xml = virNWFilterDefFormat(def))) + goto cleanup; + + if (!(configFile = virFileBuildPath(configDir, def->name, ".xml"))) + goto cleanup; + + virUUIDFormat(def->uuid, uuidstr); + ret = virXMLSaveFile(configFile, + virXMLPickShellSafeComment(def->name, uuidstr), + "nwfilter-edit", xml); + + cleanup: + VIR_FREE(configFile); + VIR_FREE(xml); + return ret; +} + + static virNWFilterObjPtr virNWFilterObjListLoadConfig(virNWFilterObjListPtr nwfilters, const char *configDir, @@ -512,7 +540,7 @@ virNWFilterObjListLoadConfig(virNWFilterObjListPtr nwfilters, * object as a future load would regenerate a UUID and try again, * but the existing config would still exist and can be used. */ if (!objdef->uuid_specified && - virNWFilterSaveConfig(configDir, objdef) < 0) + virNWFilterObjSaveConfig(objdef, configDir) < 0) VIR_INFO("failed to save generated UUID for filter '%s'", objdef->name); VIR_FREE(configFile); diff --git a/src/conf/virnwfilterobj.h b/src/conf/virnwfilterobj.h index 29d9086..16f4e1b 100644 --- a/src/conf/virnwfilterobj.h +++ b/src/conf/virnwfilterobj.h @@ -98,6 +98,10 @@ virNWFilterObjListExport(virConnectPtr conn, virNWFilterObjListFilter aclfilter); int +virNWFilterObjSaveConfig(virNWFilterObjPtr obj, + const char *configDir); + +int virNWFilterObjListLoadAllConfigs(virNWFilterObjListPtr nwfilters, const char *configDir); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 429b095..cda5f92 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -744,7 +744,6 @@ virNWFilterRuleIsProtocolEthernet; virNWFilterRuleIsProtocolIPv4; virNWFilterRuleIsProtocolIPv6; virNWFilterRuleProtocolTypeToString; -virNWFilterSaveConfig; virNWFilterTriggerVMFilterRebuild; virNWFilterUnlockFilterUpdates; virNWFilterUnRegisterCallbackDriver; @@ -979,6 +978,7 @@ virNWFilterObjListNew; virNWFilterObjListNumOfNWFilters; virNWFilterObjListRemove; virNWFilterObjLock; +virNWFilterObjSaveConfig; virNWFilterObjTestUnassignDef; virNWFilterObjUnlock; virNWFilterObjWantRemoved; diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c index 5e62023..c6139d9 100644 --- a/src/nwfilter/nwfilter_driver.c +++ b/src/nwfilter/nwfilter_driver.c @@ -517,7 +517,7 @@ nwfilterDefineXML(virConnectPtr conn, def = NULL; objdef = virNWFilterObjGetDef(obj); - if (virNWFilterSaveConfig(driver->configDir, objdef) < 0) { + if (virNWFilterObjSaveConfig(obj, driver->configDir) < 0) { virNWFilterObjListRemove(driver->nwfilters, obj); goto cleanup; } -- 2.9.4
On 06/02/2017 12:25 PM, John Ferlan wrote:
Move the function into nwfilterobj, rename it to virNWFilterObjSaveConfig, and alter the order of the arguments.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/nwfilter_conf.c | 27 --------------------------- src/conf/nwfilter_conf.h | 4 ---- src/conf/virnwfilterobj.c | 30 +++++++++++++++++++++++++++++- src/conf/virnwfilterobj.h | 4 ++++ src/libvirt_private.syms | 2 +- src/nwfilter/nwfilter_driver.c | 2 +- 6 files changed, 35 insertions(+), 34 deletions(-)
diff --git a/src/conf/nwfilter_conf.c b/src/conf/nwfilter_conf.c index fba792a..bcff4b6 100644 --- a/src/conf/nwfilter_conf.c +++ b/src/conf/nwfilter_conf.c @@ -2766,33 +2766,6 @@ virNWFilterDefParseFile(const char *filename) }
-int -virNWFilterSaveConfig(const char *configDir, - virNWFilterDefPtr def) -{ - int ret = -1; - char *xml; - char uuidstr[VIR_UUID_STRING_BUFLEN]; - char *configFile = NULL; - - if (!(xml = virNWFilterDefFormat(def))) - goto cleanup; - - if (!(configFile = virFileBuildPath(configDir, def->name, ".xml"))) - goto cleanup; - - virUUIDFormat(def->uuid, uuidstr); - ret = virXMLSaveFile(configFile, - virXMLPickShellSafeComment(def->name, uuidstr), - "nwfilter-edit", xml); - - cleanup: - VIR_FREE(configFile); - VIR_FREE(xml); - return ret; -} - - int nCallbackDriver; #define MAX_CALLBACK_DRIVER 10 static virNWFilterCallbackDriverPtr callbackDrvArray[MAX_CALLBACK_DRIVER]; diff --git a/src/conf/nwfilter_conf.h b/src/conf/nwfilter_conf.h index 4bf5b7c..ac6aee9 100644 --- a/src/conf/nwfilter_conf.h +++ b/src/conf/nwfilter_conf.h @@ -580,10 +580,6 @@ virNWFilterDefParseNode(xmlDocPtr xml, char * virNWFilterDefFormat(const virNWFilterDef *def);
-int -virNWFilterSaveConfig(const char *configDir, - virNWFilterDefPtr def); - virNWFilterDefPtr virNWFilterDefParseString(const char *xml);
diff --git a/src/conf/virnwfilterobj.c b/src/conf/virnwfilterobj.c index 0343c0a..5834b9d 100644 --- a/src/conf/virnwfilterobj.c +++ b/src/conf/virnwfilterobj.c @@ -478,6 +478,34 @@ virNWFilterObjListExport(virConnectPtr conn, }
+int +virNWFilterObjSaveConfig(virNWFilterObjPtr obj, + const char *configDir) +{ + virNWFilterDefPtr def = obj->def; + int ret = -1; + char *xml; + char uuidstr[VIR_UUID_STRING_BUFLEN]; + char *configFile = NULL; + + if (!(xml = virNWFilterDefFormat(def))) + goto cleanup; + + if (!(configFile = virFileBuildPath(configDir, def->name, ".xml"))) + goto cleanup; + + virUUIDFormat(def->uuid, uuidstr); + ret = virXMLSaveFile(configFile, + virXMLPickShellSafeComment(def->name, uuidstr), + "nwfilter-edit", xml); + + cleanup: + VIR_FREE(configFile); + VIR_FREE(xml); + return ret; +} + + static virNWFilterObjPtr virNWFilterObjListLoadConfig(virNWFilterObjListPtr nwfilters, const char *configDir, @@ -512,7 +540,7 @@ virNWFilterObjListLoadConfig(virNWFilterObjListPtr nwfilters, * object as a future load would regenerate a UUID and try again, * but the existing config would still exist and can be used. */ if (!objdef->uuid_specified && - virNWFilterSaveConfig(configDir, objdef) < 0) + virNWFilterObjSaveConfig(objdef, configDir) < 0)
How can this work? objdef is pointer to def not obj. Anyway, since we are not going to save config here: ACK Michal
On 07/13/2017 10:41 AM, Michal Privoznik wrote:
On 06/02/2017 12:25 PM, John Ferlan wrote:
Move the function into nwfilterobj, rename it to virNWFilterObjSaveConfig, and alter the order of the arguments.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/nwfilter_conf.c | 27 --------------------------- src/conf/nwfilter_conf.h | 4 ---- src/conf/virnwfilterobj.c | 30 +++++++++++++++++++++++++++++- src/conf/virnwfilterobj.h | 4 ++++ src/libvirt_private.syms | 2 +- src/nwfilter/nwfilter_driver.c | 2 +- 6 files changed, 35 insertions(+), 34 deletions(-)
diff --git a/src/conf/nwfilter_conf.c b/src/conf/nwfilter_conf.c index fba792a..bcff4b6 100644 --- a/src/conf/nwfilter_conf.c +++ b/src/conf/nwfilter_conf.c @@ -2766,33 +2766,6 @@ virNWFilterDefParseFile(const char *filename) }
-int -virNWFilterSaveConfig(const char *configDir, - virNWFilterDefPtr def) -{ - int ret = -1; - char *xml; - char uuidstr[VIR_UUID_STRING_BUFLEN]; - char *configFile = NULL; - - if (!(xml = virNWFilterDefFormat(def))) - goto cleanup; - - if (!(configFile = virFileBuildPath(configDir, def->name, ".xml"))) - goto cleanup; - - virUUIDFormat(def->uuid, uuidstr); - ret = virXMLSaveFile(configFile, - virXMLPickShellSafeComment(def->name, uuidstr), - "nwfilter-edit", xml); - - cleanup: - VIR_FREE(configFile); - VIR_FREE(xml); - return ret; -} - - int nCallbackDriver; #define MAX_CALLBACK_DRIVER 10 static virNWFilterCallbackDriverPtr callbackDrvArray[MAX_CALLBACK_DRIVER]; diff --git a/src/conf/nwfilter_conf.h b/src/conf/nwfilter_conf.h index 4bf5b7c..ac6aee9 100644 --- a/src/conf/nwfilter_conf.h +++ b/src/conf/nwfilter_conf.h @@ -580,10 +580,6 @@ virNWFilterDefParseNode(xmlDocPtr xml, char * virNWFilterDefFormat(const virNWFilterDef *def);
-int -virNWFilterSaveConfig(const char *configDir, - virNWFilterDefPtr def); - virNWFilterDefPtr virNWFilterDefParseString(const char *xml);
diff --git a/src/conf/virnwfilterobj.c b/src/conf/virnwfilterobj.c index 0343c0a..5834b9d 100644 --- a/src/conf/virnwfilterobj.c +++ b/src/conf/virnwfilterobj.c @@ -478,6 +478,34 @@ virNWFilterObjListExport(virConnectPtr conn, }
+int +virNWFilterObjSaveConfig(virNWFilterObjPtr obj, + const char *configDir) +{ + virNWFilterDefPtr def = obj->def; + int ret = -1; + char *xml; + char uuidstr[VIR_UUID_STRING_BUFLEN]; + char *configFile = NULL; + + if (!(xml = virNWFilterDefFormat(def))) + goto cleanup; + + if (!(configFile = virFileBuildPath(configDir, def->name, ".xml"))) + goto cleanup; + + virUUIDFormat(def->uuid, uuidstr); + ret = virXMLSaveFile(configFile, + virXMLPickShellSafeComment(def->name, uuidstr), + "nwfilter-edit", xml); + + cleanup: + VIR_FREE(configFile); + VIR_FREE(xml); + return ret; +} + + static virNWFilterObjPtr virNWFilterObjListLoadConfig(virNWFilterObjListPtr nwfilters, const char *configDir, @@ -512,7 +540,7 @@ virNWFilterObjListLoadConfig(virNWFilterObjListPtr nwfilters, * object as a future load would regenerate a UUID and try again, * but the existing config would still exist and can be used. */ if (!objdef->uuid_specified && - virNWFilterSaveConfig(configDir, objdef) < 0) + virNWFilterObjSaveConfig(objdef, configDir) < 0)
How can this work? objdef is pointer to def not obj. Anyway, since we are not going to save config here:
Oh right that should have been @obj; otherwise, git bisect will fail. I'll adjust... It's fixed by the next patch anyway. John
ACK
Michal
On 07/13/2017 10:41 AM, Michal Privoznik wrote:
On 06/02/2017 12:25 PM, John Ferlan wrote:
Move the function into nwfilterobj, rename it to virNWFilterObjSaveConfig, and alter the order of the arguments.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/nwfilter_conf.c | 27 --------------------------- src/conf/nwfilter_conf.h | 4 ---- src/conf/virnwfilterobj.c | 30 +++++++++++++++++++++++++++++- src/conf/virnwfilterobj.h | 4 ++++ src/libvirt_private.syms | 2 +- src/nwfilter/nwfilter_driver.c | 2 +- 6 files changed, 35 insertions(+), 34 deletions(-)
Since patches 2 & 3 are going to be dropped and patch 6 is deemed unnecessary, I'm going to drop this one too. John
diff --git a/src/conf/nwfilter_conf.c b/src/conf/nwfilter_conf.c index fba792a..bcff4b6 100644 --- a/src/conf/nwfilter_conf.c +++ b/src/conf/nwfilter_conf.c @@ -2766,33 +2766,6 @@ virNWFilterDefParseFile(const char *filename) }
-int -virNWFilterSaveConfig(const char *configDir, - virNWFilterDefPtr def) -{ - int ret = -1; - char *xml; - char uuidstr[VIR_UUID_STRING_BUFLEN]; - char *configFile = NULL; - - if (!(xml = virNWFilterDefFormat(def))) - goto cleanup; - - if (!(configFile = virFileBuildPath(configDir, def->name, ".xml"))) - goto cleanup; - - virUUIDFormat(def->uuid, uuidstr); - ret = virXMLSaveFile(configFile, - virXMLPickShellSafeComment(def->name, uuidstr), - "nwfilter-edit", xml); - - cleanup: - VIR_FREE(configFile); - VIR_FREE(xml); - return ret; -} - - int nCallbackDriver; #define MAX_CALLBACK_DRIVER 10 static virNWFilterCallbackDriverPtr callbackDrvArray[MAX_CALLBACK_DRIVER]; diff --git a/src/conf/nwfilter_conf.h b/src/conf/nwfilter_conf.h index 4bf5b7c..ac6aee9 100644 --- a/src/conf/nwfilter_conf.h +++ b/src/conf/nwfilter_conf.h @@ -580,10 +580,6 @@ virNWFilterDefParseNode(xmlDocPtr xml, char * virNWFilterDefFormat(const virNWFilterDef *def);
-int -virNWFilterSaveConfig(const char *configDir, - virNWFilterDefPtr def); - virNWFilterDefPtr virNWFilterDefParseString(const char *xml);
diff --git a/src/conf/virnwfilterobj.c b/src/conf/virnwfilterobj.c index 0343c0a..5834b9d 100644 --- a/src/conf/virnwfilterobj.c +++ b/src/conf/virnwfilterobj.c @@ -478,6 +478,34 @@ virNWFilterObjListExport(virConnectPtr conn, }
+int +virNWFilterObjSaveConfig(virNWFilterObjPtr obj, + const char *configDir) +{ + virNWFilterDefPtr def = obj->def; + int ret = -1; + char *xml; + char uuidstr[VIR_UUID_STRING_BUFLEN]; + char *configFile = NULL; + + if (!(xml = virNWFilterDefFormat(def))) + goto cleanup; + + if (!(configFile = virFileBuildPath(configDir, def->name, ".xml"))) + goto cleanup; + + virUUIDFormat(def->uuid, uuidstr); + ret = virXMLSaveFile(configFile, + virXMLPickShellSafeComment(def->name, uuidstr), + "nwfilter-edit", xml); + + cleanup: + VIR_FREE(configFile); + VIR_FREE(xml); + return ret; +} + + static virNWFilterObjPtr virNWFilterObjListLoadConfig(virNWFilterObjListPtr nwfilters, const char *configDir, @@ -512,7 +540,7 @@ virNWFilterObjListLoadConfig(virNWFilterObjListPtr nwfilters, * object as a future load would regenerate a UUID and try again, * but the existing config would still exist and can be used. */ if (!objdef->uuid_specified && - virNWFilterSaveConfig(configDir, objdef) < 0) + virNWFilterObjSaveConfig(objdef, configDir) < 0)
How can this work? objdef is pointer to def not obj. Anyway, since we are not going to save config here:
ACK
Michal
When creating an object, save the configFile name in the object rather than needing to build it up each time for the SaveConfig. This involves adding a @configDir parameter to virNWFilterObjListAssignDef and removing the @configDir from virNWFilterObjSaveConfig. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virnwfilterobj.c | 38 ++++++++++++++++++++------------------ src/conf/virnwfilterobj.h | 6 +++--- src/nwfilter/nwfilter_driver.c | 5 +++-- 3 files changed, 26 insertions(+), 23 deletions(-) diff --git a/src/conf/virnwfilterobj.c b/src/conf/virnwfilterobj.c index 5834b9d..ac99f47 100644 --- a/src/conf/virnwfilterobj.c +++ b/src/conf/virnwfilterobj.c @@ -40,6 +40,8 @@ struct _virNWFilterObj { virNWFilterDefPtr def; virNWFilterDefPtr newDef; + + char *configFile; }; struct _virNWFilterObjList { @@ -95,6 +97,7 @@ virNWFilterObjFree(virNWFilterObjPtr obj) if (!obj) return; + VIR_FREE(obj->configFile); virNWFilterDefFree(obj->def); virNWFilterDefFree(obj->newDef); @@ -296,7 +299,8 @@ virNWFilterDefEqual(const virNWFilterDef *def1, virNWFilterObjPtr virNWFilterObjListAssignDef(virNWFilterObjListPtr nwfilters, - virNWFilterDefPtr def) + virNWFilterDefPtr def, + const char *configDir) { virNWFilterObjPtr obj; virNWFilterDefPtr objdef; @@ -360,15 +364,20 @@ virNWFilterObjListAssignDef(virNWFilterObjListPtr nwfilters, if (!(obj = virNWFilterObjNew())) return NULL; - if (VIR_APPEND_ELEMENT_COPY(nwfilters->objs, - nwfilters->count, obj) < 0) { - virNWFilterObjUnlock(obj); - virNWFilterObjFree(obj); - return NULL; - } + if (!(obj->configFile = virFileBuildPath(configDir, def->name, ".xml"))) + goto error; + + if (VIR_APPEND_ELEMENT_COPY(nwfilters->objs, nwfilters->count, obj) < 0) + goto error; + obj->def = def; return obj; + + error: + virNWFilterObjUnlock(obj); + virNWFilterObjFree(obj); + return NULL; } @@ -479,28 +488,22 @@ virNWFilterObjListExport(virConnectPtr conn, int -virNWFilterObjSaveConfig(virNWFilterObjPtr obj, - const char *configDir) +virNWFilterObjSaveConfig(virNWFilterObjPtr obj) { virNWFilterDefPtr def = obj->def; int ret = -1; char *xml; char uuidstr[VIR_UUID_STRING_BUFLEN]; - char *configFile = NULL; if (!(xml = virNWFilterDefFormat(def))) goto cleanup; - if (!(configFile = virFileBuildPath(configDir, def->name, ".xml"))) - goto cleanup; - virUUIDFormat(def->uuid, uuidstr); - ret = virXMLSaveFile(configFile, + ret = virXMLSaveFile(obj->configFile, virXMLPickShellSafeComment(def->name, uuidstr), "nwfilter-edit", xml); cleanup: - VIR_FREE(configFile); VIR_FREE(xml); return ret; } @@ -530,7 +533,7 @@ virNWFilterObjListLoadConfig(virNWFilterObjListPtr nwfilters, goto error; } - if (!(obj = virNWFilterObjListAssignDef(nwfilters, def))) + if (!(obj = virNWFilterObjListAssignDef(nwfilters, def, configDir))) goto error; def = NULL; objdef = obj->def; @@ -539,8 +542,7 @@ virNWFilterObjListLoadConfig(virNWFilterObjListPtr nwfilters, * config to disk. If not successful, no need to fail or remove the * object as a future load would regenerate a UUID and try again, * but the existing config would still exist and can be used. */ - if (!objdef->uuid_specified && - virNWFilterObjSaveConfig(objdef, configDir) < 0) + if (!objdef->uuid_specified && virNWFilterObjSaveConfig(obj) < 0) VIR_INFO("failed to save generated UUID for filter '%s'", objdef->name); VIR_FREE(configFile); diff --git a/src/conf/virnwfilterobj.h b/src/conf/virnwfilterobj.h index 16f4e1b..e4dadda 100644 --- a/src/conf/virnwfilterobj.h +++ b/src/conf/virnwfilterobj.h @@ -70,7 +70,8 @@ virNWFilterObjListFindByName(virNWFilterObjListPtr nwfilters, virNWFilterObjPtr virNWFilterObjListAssignDef(virNWFilterObjListPtr nwfilters, - virNWFilterDefPtr def); + virNWFilterDefPtr def, + const char *configDir); int virNWFilterObjTestUnassignDef(virNWFilterObjPtr obj); @@ -98,8 +99,7 @@ virNWFilterObjListExport(virConnectPtr conn, virNWFilterObjListFilter aclfilter); int -virNWFilterObjSaveConfig(virNWFilterObjPtr obj, - const char *configDir); +virNWFilterObjSaveConfig(virNWFilterObjPtr obj); int virNWFilterObjListLoadAllConfigs(virNWFilterObjListPtr nwfilters, diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c index c6139d9..c740c53 100644 --- a/src/nwfilter/nwfilter_driver.c +++ b/src/nwfilter/nwfilter_driver.c @@ -512,12 +512,13 @@ nwfilterDefineXML(virConnectPtr conn, if (virNWFilterDefineXMLEnsureACL(conn, def) < 0) goto cleanup; - if (!(obj = virNWFilterObjListAssignDef(driver->nwfilters, def))) + if (!(obj = virNWFilterObjListAssignDef(driver->nwfilters, def, + driver->configDir))) goto cleanup; def = NULL; objdef = virNWFilterObjGetDef(obj); - if (virNWFilterObjSaveConfig(obj, driver->configDir) < 0) { + if (virNWFilterObjSaveConfig(obj) < 0) { virNWFilterObjListRemove(driver->nwfilters, obj); goto cleanup; } -- 2.9.4
On 06/02/2017 12:25 PM, John Ferlan wrote:
When creating an object, save the configFile name in the object rather than needing to build it up each time for the SaveConfig. This involves adding a @configDir parameter to virNWFilterObjListAssignDef and removing the @configDir from virNWFilterObjSaveConfig.
Why? This keeps the path in memory for the whole life time of the object even though it's needed just occasionally - at the beginning and probably at the end.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virnwfilterobj.c | 38 ++++++++++++++++++++------------------ src/conf/virnwfilterobj.h | 6 +++--- src/nwfilter/nwfilter_driver.c | 5 +++-- 3 files changed, 26 insertions(+), 23 deletions(-)
Michal
On 07/13/2017 10:41 AM, Michal Privoznik wrote:
On 06/02/2017 12:25 PM, John Ferlan wrote:
When creating an object, save the configFile name in the object rather than needing to build it up each time for the SaveConfig. This involves adding a @configDir parameter to virNWFilterObjListAssignDef and removing the @configDir from virNWFilterObjSaveConfig.
Why? This keeps the path in memory for the whole life time of the object even though it's needed just occasionally - at the beginning and probably at the end.
OK - again this is something that was originally from the broader scheme where a ConfigName could be saved in a more opaque object. We do it for secrets and storage, so this was a bit of monkey-see, monkey-do. John (I'm done for the day - I'll pick up looking at the rest of the review comments tomorrow).
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virnwfilterobj.c | 38 ++++++++++++++++++++------------------ src/conf/virnwfilterobj.h | 6 +++--- src/nwfilter/nwfilter_driver.c | 5 +++-- 3 files changed, 26 insertions(+), 23 deletions(-)
Michal
On 07/13/2017 10:41 AM, Michal Privoznik wrote:
On 06/02/2017 12:25 PM, John Ferlan wrote:
When creating an object, save the configFile name in the object rather than needing to build it up each time for the SaveConfig. This involves adding a @configDir parameter to virNWFilterObjListAssignDef and removing the @configDir from virNWFilterObjSaveConfig.
Why? This keeps the path in memory for the whole life time of the object even though it's needed just occasionally - at the beginning and probably at the end.
I'm going to drop this one. John
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virnwfilterobj.c | 38 ++++++++++++++++++++------------------ src/conf/virnwfilterobj.h | 6 +++--- src/nwfilter/nwfilter_driver.c | 5 +++-- 3 files changed, 26 insertions(+), 23 deletions(-)
Michal
Modify the virNWFilterObjNew to take @def as a parameter and consume it. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virnwfilterobj.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/src/conf/virnwfilterobj.c b/src/conf/virnwfilterobj.c index ac99f47..c86b1a9 100644 --- a/src/conf/virnwfilterobj.c +++ b/src/conf/virnwfilterobj.c @@ -51,7 +51,7 @@ struct _virNWFilterObjList { static virNWFilterObjPtr -virNWFilterObjNew(void) +virNWFilterObjNew(virNWFilterDefPtr def) { virNWFilterObjPtr obj; @@ -66,6 +66,8 @@ virNWFilterObjNew(void) } virNWFilterObjLock(obj); + obj->def = def; + return obj; } @@ -361,20 +363,21 @@ virNWFilterObjListAssignDef(virNWFilterObjListPtr nwfilters, return obj; } - if (!(obj = virNWFilterObjNew())) + if (!(obj = virNWFilterObjNew(def))) return NULL; + def = NULL; + objdef = obj->def; - if (!(obj->configFile = virFileBuildPath(configDir, def->name, ".xml"))) + if (!(obj->configFile = virFileBuildPath(configDir, objdef->name, ".xml"))) goto error; if (VIR_APPEND_ELEMENT_COPY(nwfilters->objs, nwfilters->count, obj) < 0) goto error; - obj->def = def; - return obj; error: + obj->def = NULL; virNWFilterObjUnlock(obj); virNWFilterObjFree(obj); return NULL; -- 2.9.4
On 06/02/2017 12:25 PM, John Ferlan wrote:
Modify the virNWFilterObjNew to take @def as a parameter and consume it.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virnwfilterobj.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-)
As I've stated in the other review for secret patch set, I don't see much value in this patch. What your reasoning for it? We don't usually do it this way. Michal
On 07/13/2017 10:41 AM, Michal Privoznik wrote:
On 06/02/2017 12:25 PM, John Ferlan wrote:
Modify the virNWFilterObjNew to take @def as a parameter and consume it.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virnwfilterobj.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-)
As I've stated in the other review for secret patch set, I don't see much value in this patch. What your reasoning for it? We don't usually do it this way.
Michal
OK consider this one dropped, too. John
No need to goto cleanup and check "if (obj)" if we know (obj) isn't there, so just return immediately. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/nwfilter/nwfilter_driver.c | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c index c740c53..d527769 100644 --- a/src/nwfilter/nwfilter_driver.c +++ b/src/nwfilter/nwfilter_driver.c @@ -391,7 +391,7 @@ nwfilterLookupByUUID(virConnectPtr conn, nwfilterDriverUnlock(); if (!obj) - goto cleanup; + return NULL; def = virNWFilterObjGetDef(obj); if (virNWFilterLookupByUUIDEnsureACL(conn, def) < 0) @@ -400,8 +400,7 @@ nwfilterLookupByUUID(virConnectPtr conn, ret = virGetNWFilter(conn, def->name, def->uuid); cleanup: - if (obj) - virNWFilterObjUnlock(obj); + virNWFilterObjUnlock(obj); return ret; } @@ -421,7 +420,7 @@ nwfilterLookupByName(virConnectPtr conn, if (!obj) { virReportError(VIR_ERR_NO_NWFILTER, _("no nwfilter with matching name '%s'"), name); - goto cleanup; + return NULL; } def = virNWFilterObjGetDef(obj); @@ -431,8 +430,7 @@ nwfilterLookupByName(virConnectPtr conn, ret = virGetNWFilter(conn, def->name, def->uuid); cleanup: - if (obj) - virNWFilterObjUnlock(obj); + virNWFilterObjUnlock(obj); return ret; } @@ -595,7 +593,7 @@ nwfilterGetXMLDesc(virNWFilterPtr nwfilter, nwfilterDriverUnlock(); if (!obj) - goto cleanup; + return NULL; def = virNWFilterObjGetDef(obj); if (virNWFilterGetXMLDescEnsureACL(nwfilter->conn, def) < 0) @@ -604,8 +602,7 @@ nwfilterGetXMLDesc(virNWFilterPtr nwfilter, ret = virNWFilterDefFormat(def); cleanup: - if (obj) - virNWFilterObjUnlock(obj); + virNWFilterObjUnlock(obj); return ret; } -- 2.9.4
On 06/02/2017 12:25 PM, John Ferlan wrote:
No need to goto cleanup and check "if (obj)" if we know (obj) isn't there, so just return immediately.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/nwfilter/nwfilter_driver.c | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-)
ACK Michal
Use @nwfilter always for the name Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/nwfilter/nwfilter_driver.c | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c index d527769..7db4f87 100644 --- a/src/nwfilter/nwfilter_driver.c +++ b/src/nwfilter/nwfilter_driver.c @@ -384,7 +384,7 @@ nwfilterLookupByUUID(virConnectPtr conn, { virNWFilterObjPtr obj; virNWFilterDefPtr def; - virNWFilterPtr ret = NULL; + virNWFilterPtr nwfilter = NULL; nwfilterDriverLock(); obj = nwfilterObjFromNWFilter(uuid); @@ -397,11 +397,11 @@ nwfilterLookupByUUID(virConnectPtr conn, if (virNWFilterLookupByUUIDEnsureACL(conn, def) < 0) goto cleanup; - ret = virGetNWFilter(conn, def->name, def->uuid); + nwfilter = virGetNWFilter(conn, def->name, def->uuid); cleanup: virNWFilterObjUnlock(obj); - return ret; + return nwfilter; } @@ -411,7 +411,7 @@ nwfilterLookupByName(virConnectPtr conn, { virNWFilterObjPtr obj; virNWFilterDefPtr def; - virNWFilterPtr ret = NULL; + virNWFilterPtr nwfilter = NULL; nwfilterDriverLock(); obj = virNWFilterObjListFindByName(driver->nwfilters, name); @@ -427,11 +427,11 @@ nwfilterLookupByName(virConnectPtr conn, if (virNWFilterLookupByNameEnsureACL(conn, def) < 0) goto cleanup; - ret = virGetNWFilter(conn, def->name, def->uuid); + nwfilter = virGetNWFilter(conn, def->name, def->uuid); cleanup: virNWFilterObjUnlock(obj); - return ret; + return nwfilter; } @@ -467,7 +467,7 @@ nwfilterConnectListNWFilters(virConnectPtr conn, static int nwfilterConnectListAllNWFilters(virConnectPtr conn, - virNWFilterPtr **filters, + virNWFilterPtr **nwfilters, unsigned int flags) { int ret; @@ -478,7 +478,7 @@ nwfilterConnectListAllNWFilters(virConnectPtr conn, return -1; nwfilterDriverLock(); - ret = virNWFilterObjListExport(conn, driver->nwfilters, filters, + ret = virNWFilterObjListExport(conn, driver->nwfilters, nwfilters, virConnectListAllNWFiltersCheckACL); nwfilterDriverUnlock(); @@ -492,7 +492,7 @@ nwfilterDefineXML(virConnectPtr conn, virNWFilterDefPtr def; virNWFilterObjPtr obj = NULL; virNWFilterDefPtr objdef; - virNWFilterPtr ret = NULL; + virNWFilterPtr nwfilter = NULL; if (!driver->privileged) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", @@ -521,7 +521,7 @@ nwfilterDefineXML(virConnectPtr conn, goto cleanup; } - ret = virGetNWFilter(conn, objdef->name, objdef->uuid); + nwfilter = virGetNWFilter(conn, objdef->name, objdef->uuid); cleanup: virNWFilterDefFree(def); @@ -531,7 +531,7 @@ nwfilterDefineXML(virConnectPtr conn, virNWFilterCallbackDriversUnlock(); virNWFilterUnlockFilterUpdates(); nwfilterDriverUnlock(); - return ret; + return nwfilter; } -- 2.9.4
On 06/02/2017 12:25 PM, John Ferlan wrote:
Use @nwfilter always for the name
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/nwfilter/nwfilter_driver.c | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-)
Okay, not much value, but at least we are consistent now :-) ACK Michal
Rename to virNWFilterDoInstantiate to better describe the action. Also fix the @vmuuid parameter to not have the ATTRIBUTE_UNUSED since it is used in the call to virNWFilterDHCPSnoopReq. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/nwfilter/nwfilter_gentech_driver.c | 46 +++++++++++++++------------------- 1 file changed, 20 insertions(+), 26 deletions(-) diff --git a/src/nwfilter/nwfilter_gentech_driver.c b/src/nwfilter/nwfilter_gentech_driver.c index 82e20de..9c11cb3 100644 --- a/src/nwfilter/nwfilter_gentech_driver.c +++ b/src/nwfilter/nwfilter_gentech_driver.c @@ -59,7 +59,7 @@ static virNWFilterTechDriverPtr filter_tech_drivers[] = { /* Serializes instantiation of filters. This is necessary * to avoid lock ordering deadlocks. eg __virNWFilterInstantiateFilter * will hold a lock on a virNWFilterObjPtr. This in turn invokes - * virNWFilterInstantiate which invokes virNWFilterDetermineMissingVarsRec + * virNWFilterDoInstantiate which invokes virNWFilterDetermineMissingVarsRec * which invokes virNWFilterObjListFindByName. This iterates over every single * virNWFilterObjPtr in the list. So if 2 threads try to instantiate a * filter in parallel, they'll both hold 1 lock at the top level in @@ -604,7 +604,7 @@ virNWFilterDetermineMissingVarsRec(virNWFilterDefPtr filter, /** - * virNWFilterInstantiate: + * virNWFilterDoInstantiate: * @vmuuid: The UUID of the VM * @techdriver: The driver to use for instantiation * @filter: The filter to instantiate @@ -624,18 +624,19 @@ virNWFilterDetermineMissingVarsRec(virNWFilterDefPtr filter, * Call this function while holding the NWFilter filter update lock */ static int -virNWFilterInstantiate(const unsigned char *vmuuid ATTRIBUTE_UNUSED, - virNWFilterTechDriverPtr techdriver, - virNWFilterDefPtr filter, - const char *ifname, - int ifindex, - const char *linkdev, - virNWFilterHashTablePtr vars, - enum instCase useNewFilter, bool *foundNewFilter, - bool teardownOld, - const virMacAddr *macaddr, - virNWFilterDriverStatePtr driver, - bool forceWithPendingReq) +virNWFilterDoInstantiate(const unsigned char *vmuuid, + virNWFilterTechDriverPtr techdriver, + virNWFilterDefPtr filter, + const char *ifname, + int ifindex, + const char *linkdev, + virNWFilterHashTablePtr vars, + enum instCase useNewFilter, + bool *foundNewFilter, + bool teardownOld, + const virMacAddr *macaddr, + virNWFilterDriverStatePtr driver, + bool forceWithPendingReq) { int rc; virNWFilterInst inst; @@ -867,18 +868,11 @@ __virNWFilterInstantiateFilter(virNWFilterDriverStatePtr driver, break; } - rc = virNWFilterInstantiate(vmuuid, - techdriver, - filter, - ifname, - ifindex, - linkdev, - vars, - useNewFilter, foundNewFilter, - teardownOld, - macaddr, - driver, - forceWithPendingReq); + rc = virNWFilterDoInstantiate(vmuuid, techdriver, filter, + ifname, ifindex, linkdev, + vars, useNewFilter, foundNewFilter, + teardownOld, macaddr, driver, + forceWithPendingReq); virNWFilterHashTableFree(vars); -- 2.9.4
On 06/02/2017 12:25 PM, John Ferlan wrote:
Rename to virNWFilterDoInstantiate to better describe the action.
Also fix the @vmuuid parameter to not have the ATTRIBUTE_UNUSED since it is used in the call to virNWFilterDHCPSnoopReq.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/nwfilter/nwfilter_gentech_driver.c | 46 +++++++++++++++------------------- 1 file changed, 20 insertions(+), 26 deletions(-)
ACK Michal
Rename to virNWFilterInstantiateFilterUpdate and alter the callers to not have one parameter per line. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/nwfilter/nwfilter_gentech_driver.c | 62 +++++++++++++--------------------- 1 file changed, 24 insertions(+), 38 deletions(-) diff --git a/src/nwfilter/nwfilter_gentech_driver.c b/src/nwfilter/nwfilter_gentech_driver.c index 9c11cb3..81b12bb 100644 --- a/src/nwfilter/nwfilter_gentech_driver.c +++ b/src/nwfilter/nwfilter_gentech_driver.c @@ -57,13 +57,13 @@ static virNWFilterTechDriverPtr filter_tech_drivers[] = { }; /* Serializes instantiation of filters. This is necessary - * to avoid lock ordering deadlocks. eg __virNWFilterInstantiateFilter + * to avoid lock ordering deadlocks. eg virNWFilterInstantiateFilterUpdate * will hold a lock on a virNWFilterObjPtr. This in turn invokes * virNWFilterDoInstantiate which invokes virNWFilterDetermineMissingVarsRec * which invokes virNWFilterObjListFindByName. This iterates over every single * virNWFilterObjPtr in the list. So if 2 threads try to instantiate a * filter in parallel, they'll both hold 1 lock at the top level in - * __virNWFilterInstantiateFilter which will cause the other thread + * virNWFilterInstantiateFilterUpdate which will cause the other thread * to deadlock in virNWFilterObjListFindByName. * * XXX better long term solution is to make virNWFilterObjList use a @@ -776,18 +776,18 @@ virNWFilterDoInstantiate(const unsigned char *vmuuid, * Call this function while holding the NWFilter filter update lock */ static int -__virNWFilterInstantiateFilter(virNWFilterDriverStatePtr driver, - const unsigned char *vmuuid, - bool teardownOld, - const char *ifname, - int ifindex, - const char *linkdev, - const virMacAddr *macaddr, - const char *filtername, - virNWFilterHashTablePtr filterparams, - enum instCase useNewFilter, - bool forceWithPendingReq, - bool *foundNewFilter) +virNWFilterInstantiateFilterUpdate(virNWFilterDriverStatePtr driver, + const unsigned char *vmuuid, + bool teardownOld, + const char *ifname, + int ifindex, + const char *linkdev, + const virMacAddr *macaddr, + const char *filtername, + virNWFilterHashTablePtr filterparams, + enum instCase useNewFilter, + bool forceWithPendingReq, + bool *foundNewFilter) { int rc; const char *drvname = EBIPTABLES_DRIVER_ID; @@ -917,18 +917,11 @@ _virNWFilterInstantiateFilter(virNWFilterDriverStatePtr driver, goto cleanup; } - rc = __virNWFilterInstantiateFilter(driver, - vmuuid, - teardownOld, - net->ifname, - ifindex, - linkdev, - &net->mac, - net->filter, - net->filterparams, - useNewFilter, - false, - foundNewFilter); + rc = virNWFilterInstantiateFilterUpdate(driver, vmuuid, teardownOld, + net->ifname, ifindex, linkdev, + &net->mac, net->filter, + net->filterparams, useNewFilter, + false, foundNewFilter); cleanup: virMutexUnlock(&updateMutex); @@ -953,18 +946,11 @@ virNWFilterInstantiateFilterLate(virNWFilterDriverStatePtr driver, virNWFilterReadLockFilterUpdates(); virMutexLock(&updateMutex); - rc = __virNWFilterInstantiateFilter(driver, - vmuuid, - true, - ifname, - ifindex, - linkdev, - macaddr, - filtername, - filterparams, - INSTANTIATE_ALWAYS, - true, - &foundNewFilter); + rc = virNWFilterInstantiateFilterUpdate(driver, vmuuid, true, + ifname, ifindex, linkdev, + macaddr, filtername, filterparams, + INSTANTIATE_ALWAYS, true, + &foundNewFilter); if (rc < 0) { /* something went wrong... 'DOWN' the interface */ if ((virNetDevValidateConfig(ifname, NULL, ifindex) <= 0) || -- 2.9.4
On 06/02/2017 12:25 PM, John Ferlan wrote:
Rename to virNWFilterInstantiateFilterUpdate and alter the callers to not have one parameter per line.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/nwfilter/nwfilter_gentech_driver.c | 62 +++++++++++++--------------------- 1 file changed, 24 insertions(+), 38 deletions(-)
ACK BTW I'm curious why the "Update" suffix? Michal
On 07/13/2017 10:40 AM, Michal Privoznik wrote:
On 06/02/2017 12:25 PM, John Ferlan wrote:
Rename to virNWFilterInstantiateFilterUpdate and alter the callers to not have one parameter per line.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/nwfilter/nwfilter_gentech_driver.c | 62 +++++++++++++--------------------- 1 file changed, 24 insertions(+), 38 deletions(-)
ACK
BTW I'm curious why the "Update" suffix?
No special reason - although IIRC this is where all the work is done and it's way better than the existing name. John
New API will be virNWFilterInstantiateFilterInternal as it's called from the virNWFilterInstantiateFilter and virNWFilterUpdateInstantiateFilter. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/nwfilter/nwfilter_gentech_driver.c | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/src/nwfilter/nwfilter_gentech_driver.c b/src/nwfilter/nwfilter_gentech_driver.c index 81b12bb..5798371 100644 --- a/src/nwfilter/nwfilter_gentech_driver.c +++ b/src/nwfilter/nwfilter_gentech_driver.c @@ -890,12 +890,12 @@ virNWFilterInstantiateFilterUpdate(virNWFilterDriverStatePtr driver, static int -_virNWFilterInstantiateFilter(virNWFilterDriverStatePtr driver, - const unsigned char *vmuuid, - const virDomainNetDef *net, - bool teardownOld, - enum instCase useNewFilter, - bool *foundNewFilter) +virNWFilterInstantiateFilterInternal(virNWFilterDriverStatePtr driver, + const unsigned char *vmuuid, + const virDomainNetDef *net, + bool teardownOld, + enum instCase useNewFilter, + bool *foundNewFilter) { const char *linkdev = (net->type == VIR_DOMAIN_NET_TYPE_DIRECT) ? net->data.direct.linkdev @@ -975,10 +975,10 @@ virNWFilterInstantiateFilter(virNWFilterDriverStatePtr driver, { bool foundNewFilter = false; - return _virNWFilterInstantiateFilter(driver, vmuuid, net, - 1, - INSTANTIATE_ALWAYS, - &foundNewFilter); + return virNWFilterInstantiateFilterInternal(driver, vmuuid, net, + 1, + INSTANTIATE_ALWAYS, + &foundNewFilter); } @@ -990,10 +990,10 @@ virNWFilterUpdateInstantiateFilter(virNWFilterDriverStatePtr driver, { bool foundNewFilter = false; - int rc = _virNWFilterInstantiateFilter(driver, vmuuid, net, - 0, - INSTANTIATE_FOLLOW_NEWFILTER, - &foundNewFilter); + int rc = virNWFilterInstantiateFilterInternal(driver, vmuuid, net, + 0, + INSTANTIATE_FOLLOW_NEWFILTER, + &foundNewFilter); *skipIface = !foundNewFilter; return rc; -- 2.9.4
On 06/02/2017 12:25 PM, John Ferlan wrote:
New API will be virNWFilterInstantiateFilterInternal as it's called from the virNWFilterInstantiateFilter and virNWFilterUpdateInstantiateFilter.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/nwfilter/nwfilter_gentech_driver.c | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-)
ACK Michal
Create a common API to handle the instantiation path filter lookup. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virnwfilterobj.c | 23 +++++++ src/conf/virnwfilterobj.h | 4 ++ src/libvirt_private.syms | 1 + src/nwfilter/nwfilter_gentech_driver.c | 119 ++++++++++++--------------------- 4 files changed, 70 insertions(+), 77 deletions(-) diff --git a/src/conf/virnwfilterobj.c b/src/conf/virnwfilterobj.c index c86b1a9..b21b570 100644 --- a/src/conf/virnwfilterobj.c +++ b/src/conf/virnwfilterobj.c @@ -195,6 +195,29 @@ virNWFilterObjListFindByName(virNWFilterObjListPtr nwfilters, } +virNWFilterObjPtr +virNWFilterObjListFindInstantiateFilter(virNWFilterObjListPtr nwfilters, + const char *filtername) +{ + virNWFilterObjPtr obj; + + if (!(obj = virNWFilterObjListFindByName(nwfilters, filtername))) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("referenced filter '%s' is missing"), filtername); + return NULL; + } + + if (virNWFilterObjWantRemoved(obj)) { + virReportError(VIR_ERR_NO_NWFILTER, + _("Filter '%s' is in use."), filtername); + virNWFilterObjUnlock(obj); + return NULL; + } + + return obj; +} + + static int _virNWFilterObjListDefLoopDetect(virNWFilterObjListPtr nwfilters, virNWFilterDefPtr def, diff --git a/src/conf/virnwfilterobj.h b/src/conf/virnwfilterobj.h index e4dadda..85c8ead 100644 --- a/src/conf/virnwfilterobj.h +++ b/src/conf/virnwfilterobj.h @@ -69,6 +69,10 @@ virNWFilterObjListFindByName(virNWFilterObjListPtr nwfilters, const char *name); virNWFilterObjPtr +virNWFilterObjListFindInstantiateFilter(virNWFilterObjListPtr nwfilters, + const char *filtername); + +virNWFilterObjPtr virNWFilterObjListAssignDef(virNWFilterObjListPtr nwfilters, virNWFilterDefPtr def, const char *configDir); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index cda5f92..ee19cb9 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -971,6 +971,7 @@ virNWFilterObjListAssignDef; virNWFilterObjListExport; virNWFilterObjListFindByName; virNWFilterObjListFindByUUID; +virNWFilterObjListFindInstantiateFilter; virNWFilterObjListFree; virNWFilterObjListGetNames; virNWFilterObjListLoadAllConfigs; diff --git a/src/nwfilter/nwfilter_gentech_driver.c b/src/nwfilter/nwfilter_gentech_driver.c index 5798371..68a2ddb 100644 --- a/src/nwfilter/nwfilter_gentech_driver.c +++ b/src/nwfilter/nwfilter_gentech_driver.c @@ -60,11 +60,11 @@ static virNWFilterTechDriverPtr filter_tech_drivers[] = { * to avoid lock ordering deadlocks. eg virNWFilterInstantiateFilterUpdate * will hold a lock on a virNWFilterObjPtr. This in turn invokes * virNWFilterDoInstantiate which invokes virNWFilterDetermineMissingVarsRec - * which invokes virNWFilterObjListFindByName. This iterates over every single - * virNWFilterObjPtr in the list. So if 2 threads try to instantiate a - * filter in parallel, they'll both hold 1 lock at the top level in - * virNWFilterInstantiateFilterUpdate which will cause the other thread - * to deadlock in virNWFilterObjListFindByName. + * which invokes virNWFilterObjListFindInstantiateFilter. This iterates over + * every single virNWFilterObjPtr in the list. So if 2 threads try to + * instantiate a filter in parallel, they'll both hold 1 lock at the top level + * in virNWFilterInstantiateFilterUpdate which will cause the other thread + * to deadlock in virNWFilterObjListFindInstantiateFilter. * * XXX better long term solution is to make virNWFilterObjList use a * hash table as is done for virDomainObjList. You can then get @@ -383,20 +383,9 @@ virNWFilterIncludeDefToRuleInst(virNWFilterDriverStatePtr driver, int ret = -1; VIR_DEBUG("Instantiating filter %s", inc->filterref); - obj = virNWFilterObjListFindByName(driver->nwfilters, - inc->filterref); - if (!obj) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("referenced filter '%s' is missing"), - inc->filterref); - goto cleanup; - } - if (virNWFilterObjWantRemoved(obj)) { - virReportError(VIR_ERR_NO_NWFILTER, - _("Filter '%s' is in use."), - inc->filterref); + if (!(obj = virNWFilterObjListFindInstantiateFilter(driver->nwfilters, + inc->filterref))) goto cleanup; - } /* create a temporary hashmap for depth-first tree traversal */ if (!(tmpvars = virNWFilterCreateVarsFrom(inc->params, @@ -545,58 +534,46 @@ virNWFilterDetermineMissingVarsRec(virNWFilterDefPtr filter, break; } else if (inc) { VIR_DEBUG("Following filter %s", inc->filterref); - obj = virNWFilterObjListFindByName(driver->nwfilters, inc->filterref); - if (obj) { - - if (virNWFilterObjWantRemoved(obj)) { - virReportError(VIR_ERR_NO_NWFILTER, - _("Filter '%s' is in use."), - inc->filterref); - rc = -1; - virNWFilterObjUnlock(obj); - break; - } + if (!(obj = + virNWFilterObjListFindInstantiateFilter(driver->nwfilters, + inc->filterref))) { + rc = -1; + break; + } - /* create a temporary hashmap for depth-first tree traversal */ - virNWFilterHashTablePtr tmpvars = - virNWFilterCreateVarsFrom(inc->params, - vars); - if (!tmpvars) { - rc = -1; - virNWFilterObjUnlock(obj); - break; - } + /* create a temporary hashmap for depth-first tree traversal */ + virNWFilterHashTablePtr tmpvars = + virNWFilterCreateVarsFrom(inc->params, + vars); + if (!tmpvars) { + rc = -1; + virNWFilterObjUnlock(obj); + break; + } - next_filter = virNWFilterObjGetDef(obj); + next_filter = virNWFilterObjGetDef(obj); - switch (useNewFilter) { - case INSTANTIATE_FOLLOW_NEWFILTER: - newNext_filter = virNWFilterObjGetNewDef(obj); - if (newNext_filter) - next_filter = newNext_filter; - break; - case INSTANTIATE_ALWAYS: - break; - } + switch (useNewFilter) { + case INSTANTIATE_FOLLOW_NEWFILTER: + newNext_filter = virNWFilterObjGetNewDef(obj); + if (newNext_filter) + next_filter = newNext_filter; + break; + case INSTANTIATE_ALWAYS: + break; + } - rc = virNWFilterDetermineMissingVarsRec(next_filter, - tmpvars, - missing_vars, - useNewFilter, - driver); + rc = virNWFilterDetermineMissingVarsRec(next_filter, + tmpvars, + missing_vars, + useNewFilter, + driver); - virNWFilterHashTableFree(tmpvars); + virNWFilterHashTableFree(tmpvars); - virNWFilterObjUnlock(obj); - if (rc < 0) - break; - } else { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("referenced filter '%s' is missing"), - inc->filterref); - rc = -1; + virNWFilterObjUnlock(obj); + if (rc < 0) break; - } } } return rc; @@ -813,21 +790,9 @@ virNWFilterInstantiateFilterUpdate(virNWFilterDriverStatePtr driver, VIR_DEBUG("filter name: %s", filtername); - obj = virNWFilterObjListFindByName(driver->nwfilters, filtername); - if (!obj) { - virReportError(VIR_ERR_NO_NWFILTER, - _("Could not find filter '%s'"), - filtername); + if (!(obj = virNWFilterObjListFindInstantiateFilter(driver->nwfilters, + filtername))) return -1; - } - - if (virNWFilterObjWantRemoved(obj)) { - virReportError(VIR_ERR_NO_NWFILTER, - _("Filter '%s' is in use."), - filtername); - rc = -1; - goto err_exit; - } virMacAddrFormat(macaddr, vmmacaddr); if (VIR_STRDUP(str_macaddr, vmmacaddr) < 0) { -- 2.9.4
On 06/02/2017 12:25 PM, John Ferlan wrote:
Create a common API to handle the instantiation path filter lookup.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virnwfilterobj.c | 23 +++++++ src/conf/virnwfilterobj.h | 4 ++ src/libvirt_private.syms | 1 + src/nwfilter/nwfilter_gentech_driver.c | 119 ++++++++++++--------------------- 4 files changed, 70 insertions(+), 77 deletions(-)
diff --git a/src/conf/virnwfilterobj.c b/src/conf/virnwfilterobj.c index c86b1a9..b21b570 100644 --- a/src/conf/virnwfilterobj.c +++ b/src/conf/virnwfilterobj.c @@ -195,6 +195,29 @@ virNWFilterObjListFindByName(virNWFilterObjListPtr nwfilters, }
+virNWFilterObjPtr +virNWFilterObjListFindInstantiateFilter(virNWFilterObjListPtr nwfilters, + const char *filtername) +{ + virNWFilterObjPtr obj; + + if (!(obj = virNWFilterObjListFindByName(nwfilters, filtername))) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("referenced filter '%s' is missing"), filtername); + return NULL; + } + + if (virNWFilterObjWantRemoved(obj)) { + virReportError(VIR_ERR_NO_NWFILTER, + _("Filter '%s' is in use."), filtername); + virNWFilterObjUnlock(obj); + return NULL; + } + + return obj; +} + + static int _virNWFilterObjListDefLoopDetect(virNWFilterObjListPtr nwfilters, virNWFilterDefPtr def, diff --git a/src/conf/virnwfilterobj.h b/src/conf/virnwfilterobj.h index e4dadda..85c8ead 100644 --- a/src/conf/virnwfilterobj.h +++ b/src/conf/virnwfilterobj.h @@ -69,6 +69,10 @@ virNWFilterObjListFindByName(virNWFilterObjListPtr nwfilters, const char *name);
virNWFilterObjPtr +virNWFilterObjListFindInstantiateFilter(virNWFilterObjListPtr nwfilters, + const char *filtername); + +virNWFilterObjPtr virNWFilterObjListAssignDef(virNWFilterObjListPtr nwfilters, virNWFilterDefPtr def, const char *configDir); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index cda5f92..ee19cb9 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -971,6 +971,7 @@ virNWFilterObjListAssignDef; virNWFilterObjListExport; virNWFilterObjListFindByName; virNWFilterObjListFindByUUID; +virNWFilterObjListFindInstantiateFilter; virNWFilterObjListFree; virNWFilterObjListGetNames; virNWFilterObjListLoadAllConfigs; diff --git a/src/nwfilter/nwfilter_gentech_driver.c b/src/nwfilter/nwfilter_gentech_driver.c index 5798371..68a2ddb 100644 --- a/src/nwfilter/nwfilter_gentech_driver.c +++ b/src/nwfilter/nwfilter_gentech_driver.c @@ -60,11 +60,11 @@ static virNWFilterTechDriverPtr filter_tech_drivers[] = { * to avoid lock ordering deadlocks. eg virNWFilterInstantiateFilterUpdate * will hold a lock on a virNWFilterObjPtr. This in turn invokes * virNWFilterDoInstantiate which invokes virNWFilterDetermineMissingVarsRec - * which invokes virNWFilterObjListFindByName. This iterates over every single - * virNWFilterObjPtr in the list. So if 2 threads try to instantiate a - * filter in parallel, they'll both hold 1 lock at the top level in - * virNWFilterInstantiateFilterUpdate which will cause the other thread - * to deadlock in virNWFilterObjListFindByName. + * which invokes virNWFilterObjListFindInstantiateFilter. This iterates over + * every single virNWFilterObjPtr in the list. So if 2 threads try to + * instantiate a filter in parallel, they'll both hold 1 lock at the top level + * in virNWFilterInstantiateFilterUpdate which will cause the other thread + * to deadlock in virNWFilterObjListFindInstantiateFilter. * * XXX better long term solution is to make virNWFilterObjList use a * hash table as is done for virDomainObjList. You can then get @@ -383,20 +383,9 @@ virNWFilterIncludeDefToRuleInst(virNWFilterDriverStatePtr driver, int ret = -1;
VIR_DEBUG("Instantiating filter %s", inc->filterref); - obj = virNWFilterObjListFindByName(driver->nwfilters, - inc->filterref); - if (!obj) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("referenced filter '%s' is missing"), - inc->filterref); - goto cleanup; - } - if (virNWFilterObjWantRemoved(obj)) { - virReportError(VIR_ERR_NO_NWFILTER, - _("Filter '%s' is in use."), - inc->filterref); + if (!(obj = virNWFilterObjListFindInstantiateFilter(driver->nwfilters, + inc->filterref))) goto cleanup; - }
/* create a temporary hashmap for depth-first tree traversal */ if (!(tmpvars = virNWFilterCreateVarsFrom(inc->params, @@ -545,58 +534,46 @@ virNWFilterDetermineMissingVarsRec(virNWFilterDefPtr filter, break; } else if (inc) { VIR_DEBUG("Following filter %s", inc->filterref); - obj = virNWFilterObjListFindByName(driver->nwfilters, inc->filterref); - if (obj) { - - if (virNWFilterObjWantRemoved(obj)) { - virReportError(VIR_ERR_NO_NWFILTER, - _("Filter '%s' is in use."), - inc->filterref); - rc = -1; - virNWFilterObjUnlock(obj); - break; - } + if (!(obj = + virNWFilterObjListFindInstantiateFilter(driver->nwfilters, + inc->filterref))) {
No, please move the function call onto the same line as if(!(obj =. 83 chars long line a not EOW (as we know it)
+ rc = -1; + break; + }
- /* create a temporary hashmap for depth-first tree traversal */ - virNWFilterHashTablePtr tmpvars = - virNWFilterCreateVarsFrom(inc->params, - vars);
Nor 84 chars long line.
- if (!tmpvars) { - rc = -1; - virNWFilterObjUnlock(obj); - break; - }
ACK if you fix it. Michal
"Simple" conversion to the virObjectLockable object isn't quite possible yet because the nwfilter lock requires usage of recursive locking due to algorithms handing filter "<rule>"'s and "<filterref>"'s. The list search logic would also benefit from using hash tables for lookups. So this patch is step 1 in the process - add the @refs to _virNWFilterObj and modify the algorithms to use (a temporary) virNWFilterObj{Ref|Unref} in order to set things up for the list logic to utilize virObjectLockable hash tables. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virnwfilterobj.c | 75 ++++++++++++++++++++++++++-------- src/conf/virnwfilterobj.h | 15 ++++--- src/libvirt_private.syms | 5 ++- src/nwfilter/nwfilter_driver.c | 13 +++--- src/nwfilter/nwfilter_gentech_driver.c | 11 +++-- 5 files changed, 80 insertions(+), 39 deletions(-) diff --git a/src/conf/virnwfilterobj.c b/src/conf/virnwfilterobj.c index b21b570..99be59c 100644 --- a/src/conf/virnwfilterobj.c +++ b/src/conf/virnwfilterobj.c @@ -23,6 +23,7 @@ #include "datatypes.h" #include "viralloc.h" +#include "viratomic.h" #include "virerror.h" #include "virfile.h" #include "virlog.h" @@ -33,8 +34,15 @@ VIR_LOG_INIT("conf.virnwfilterobj"); +static void +virNWFilterObjLock(virNWFilterObjPtr obj); + +static void +virNWFilterObjUnlock(virNWFilterObjPtr obj); + struct _virNWFilterObj { virMutex lock; + int refs; bool wantRemoved; @@ -67,11 +75,24 @@ virNWFilterObjNew(virNWFilterDefPtr def) virNWFilterObjLock(obj); obj->def = def; + virAtomicIntSet(&obj->refs, 1); return obj; } +void +virNWFilterObjEndAPI(virNWFilterObjPtr *obj) +{ + if (!*obj) + return; + + virNWFilterObjUnlock(*obj); + virNWFilterObjUnref(*obj); + *obj = NULL; +} + + virNWFilterDefPtr virNWFilterObjGetDef(virNWFilterObjPtr obj) { @@ -109,12 +130,33 @@ virNWFilterObjFree(virNWFilterObjPtr obj) } +virNWFilterObjPtr +virNWFilterObjRef(virNWFilterObjPtr obj) +{ + if (obj) + virAtomicIntInc(&obj->refs); + return obj; +} + + +bool +virNWFilterObjUnref(virNWFilterObjPtr obj) +{ + bool lastRef; + if (obj) + return false; + if ((lastRef = virAtomicIntDecAndTest(&obj->refs))) + virNWFilterObjFree(obj); + return !lastRef; +} + + void virNWFilterObjListFree(virNWFilterObjListPtr nwfilters) { size_t i; for (i = 0; i < nwfilters->count; i++) - virNWFilterObjFree(nwfilters->objs[i]); + virNWFilterObjUnref(nwfilters->objs[i]); VIR_FREE(nwfilters->objs); VIR_FREE(nwfilters); } @@ -143,7 +185,7 @@ virNWFilterObjListRemove(virNWFilterObjListPtr nwfilters, virNWFilterObjLock(nwfilters->objs[i]); if (nwfilters->objs[i] == obj) { virNWFilterObjUnlock(nwfilters->objs[i]); - virNWFilterObjFree(nwfilters->objs[i]); + virNWFilterObjUnref(nwfilters->objs[i]); VIR_DELETE_ELEMENT(nwfilters->objs, i, nwfilters->count); break; @@ -166,7 +208,7 @@ virNWFilterObjListFindByUUID(virNWFilterObjListPtr nwfilters, virNWFilterObjLock(obj); def = obj->def; if (!memcmp(def->uuid, uuid, VIR_UUID_BUFLEN)) - return obj; + return virNWFilterObjRef(obj); virNWFilterObjUnlock(obj); } @@ -187,7 +229,7 @@ virNWFilterObjListFindByName(virNWFilterObjListPtr nwfilters, virNWFilterObjLock(obj); def = obj->def; if (STREQ_NULLABLE(def->name, name)) - return obj; + return virNWFilterObjRef(obj); virNWFilterObjUnlock(obj); } @@ -210,7 +252,7 @@ virNWFilterObjListFindInstantiateFilter(virNWFilterObjListPtr nwfilters, if (virNWFilterObjWantRemoved(obj)) { virReportError(VIR_ERR_NO_NWFILTER, _("Filter '%s' is in use."), filtername); - virNWFilterObjUnlock(obj); + virNWFilterObjEndAPI(&obj); return NULL; } @@ -245,7 +287,7 @@ _virNWFilterObjListDefLoopDetect(virNWFilterObjListPtr nwfilters, if (obj) { rc = _virNWFilterObjListDefLoopDetect(nwfilters, obj->def, filtername); - virNWFilterObjUnlock(obj); + virNWFilterObjEndAPI(&obj); if (rc < 0) break; } @@ -338,10 +380,10 @@ virNWFilterObjListAssignDef(virNWFilterObjListPtr nwfilters, _("filter with same UUID but different name " "('%s') already exists"), objdef->name); - virNWFilterObjUnlock(obj); + virNWFilterObjEndAPI(&obj); return NULL; } - virNWFilterObjUnlock(obj); + virNWFilterObjEndAPI(&obj); } else { if ((obj = virNWFilterObjListFindByName(nwfilters, def->name))) { char uuidstr[VIR_UUID_STRING_BUFLEN]; @@ -351,7 +393,7 @@ virNWFilterObjListAssignDef(virNWFilterObjListPtr nwfilters, virReportError(VIR_ERR_OPERATION_FAILED, _("filter '%s' already exists with uuid %s"), def->name, uuidstr); - virNWFilterObjUnlock(obj); + virNWFilterObjEndAPI(&obj); return NULL; } } @@ -362,7 +404,6 @@ virNWFilterObjListAssignDef(virNWFilterObjListPtr nwfilters, return NULL; } - if ((obj = virNWFilterObjListFindByName(nwfilters, def->name))) { objdef = obj->def; @@ -376,7 +417,7 @@ virNWFilterObjListAssignDef(virNWFilterObjListPtr nwfilters, /* trigger the update on VMs referencing the filter */ if (virNWFilterTriggerVMFilterRebuild() < 0) { obj->newDef = NULL; - virNWFilterObjUnlock(obj); + virNWFilterObjEndAPI(&obj); return NULL; } @@ -397,12 +438,12 @@ virNWFilterObjListAssignDef(virNWFilterObjListPtr nwfilters, if (VIR_APPEND_ELEMENT_COPY(nwfilters->objs, nwfilters->count, obj) < 0) goto error; - return obj; + return virNWFilterObjRef(obj); error: obj->def = NULL; virNWFilterObjUnlock(obj); - virNWFilterObjFree(obj); + virNWFilterObjUnref(obj); return NULL; } @@ -600,8 +641,8 @@ virNWFilterObjListLoadAllConfigs(virNWFilterObjListPtr nwfilters, continue; obj = virNWFilterObjListLoadConfig(nwfilters, configDir, entry->d_name); - if (obj) - virNWFilterObjUnlock(obj); + + virNWFilterObjEndAPI(&obj); } VIR_DIR_CLOSE(dir); @@ -609,14 +650,14 @@ virNWFilterObjListLoadAllConfigs(virNWFilterObjListPtr nwfilters, } -void +static void virNWFilterObjLock(virNWFilterObjPtr obj) { virMutexLock(&obj->lock); } -void +static void virNWFilterObjUnlock(virNWFilterObjPtr obj) { virMutexUnlock(&obj->lock); diff --git a/src/conf/virnwfilterobj.h b/src/conf/virnwfilterobj.h index 85c8ead..31aa345 100644 --- a/src/conf/virnwfilterobj.h +++ b/src/conf/virnwfilterobj.h @@ -41,6 +41,9 @@ struct _virNWFilterDriverState { bool watchingFirewallD; }; +void +virNWFilterObjEndAPI(virNWFilterObjPtr *obj); + virNWFilterDefPtr virNWFilterObjGetDef(virNWFilterObjPtr obj); @@ -50,6 +53,12 @@ virNWFilterObjGetNewDef(virNWFilterObjPtr obj); bool virNWFilterObjWantRemoved(virNWFilterObjPtr obj); +virNWFilterObjPtr +virNWFilterObjRef(virNWFilterObjPtr obj); + +bool +virNWFilterObjUnref(virNWFilterObjPtr obj); + virNWFilterObjListPtr virNWFilterObjListNew(void); @@ -109,10 +118,4 @@ int virNWFilterObjListLoadAllConfigs(virNWFilterObjListPtr nwfilters, const char *configDir); -void -virNWFilterObjLock(virNWFilterObjPtr obj); - -void -virNWFilterObjUnlock(virNWFilterObjPtr obj); - #endif /* VIRNWFILTEROBJ_H */ diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index ee19cb9..ac0507e 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -965,6 +965,7 @@ virNodeDeviceObjUnlock; # conf/virnwfilterobj.h +virNWFilterObjEndAPI; virNWFilterObjGetDef; virNWFilterObjGetNewDef; virNWFilterObjListAssignDef; @@ -978,10 +979,10 @@ virNWFilterObjListLoadAllConfigs; virNWFilterObjListNew; virNWFilterObjListNumOfNWFilters; virNWFilterObjListRemove; -virNWFilterObjLock; +virNWFilterObjRef; virNWFilterObjSaveConfig; virNWFilterObjTestUnassignDef; -virNWFilterObjUnlock; +virNWFilterObjUnref; virNWFilterObjWantRemoved; diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c index 7db4f87..b6e11e6 100644 --- a/src/nwfilter/nwfilter_driver.c +++ b/src/nwfilter/nwfilter_driver.c @@ -400,7 +400,7 @@ nwfilterLookupByUUID(virConnectPtr conn, nwfilter = virGetNWFilter(conn, def->name, def->uuid); cleanup: - virNWFilterObjUnlock(obj); + virNWFilterObjEndAPI(&obj); return nwfilter; } @@ -430,7 +430,7 @@ nwfilterLookupByName(virConnectPtr conn, nwfilter = virGetNWFilter(conn, def->name, def->uuid); cleanup: - virNWFilterObjUnlock(obj); + virNWFilterObjEndAPI(&obj); return nwfilter; } @@ -525,8 +525,7 @@ nwfilterDefineXML(virConnectPtr conn, cleanup: virNWFilterDefFree(def); - if (obj) - virNWFilterObjUnlock(obj); + virNWFilterObjEndAPI(&obj); virNWFilterCallbackDriversUnlock(); virNWFilterUnlockFilterUpdates(); @@ -564,12 +563,10 @@ nwfilterUndefine(virNWFilterPtr nwfilter) goto cleanup; virNWFilterObjListRemove(driver->nwfilters, obj); - obj = NULL; ret = 0; cleanup: - if (obj) - virNWFilterObjUnlock(obj); + virNWFilterObjEndAPI(&obj); virNWFilterCallbackDriversUnlock(); virNWFilterUnlockFilterUpdates(); @@ -602,7 +599,7 @@ nwfilterGetXMLDesc(virNWFilterPtr nwfilter, ret = virNWFilterDefFormat(def); cleanup: - virNWFilterObjUnlock(obj); + virNWFilterObjEndAPI(&obj); return ret; } diff --git a/src/nwfilter/nwfilter_gentech_driver.c b/src/nwfilter/nwfilter_gentech_driver.c index 68a2ddb..4c228ea 100644 --- a/src/nwfilter/nwfilter_gentech_driver.c +++ b/src/nwfilter/nwfilter_gentech_driver.c @@ -315,7 +315,7 @@ virNWFilterInstReset(virNWFilterInstPtr inst) size_t i; for (i = 0; i < inst->nfilters; i++) - virNWFilterObjUnlock(inst->filters[i]); + virNWFilterObjEndAPI(&inst->filters[i]); VIR_FREE(inst->filters); inst->nfilters = 0; @@ -425,8 +425,7 @@ virNWFilterIncludeDefToRuleInst(virNWFilterDriverStatePtr driver, if (ret < 0) virNWFilterInstReset(inst); virNWFilterHashTableFree(tmpvars); - if (obj) - virNWFilterObjUnlock(obj); + virNWFilterObjEndAPI(&obj); return ret; } @@ -547,7 +546,7 @@ virNWFilterDetermineMissingVarsRec(virNWFilterDefPtr filter, vars); if (!tmpvars) { rc = -1; - virNWFilterObjUnlock(obj); + virNWFilterObjEndAPI(&obj); break; } @@ -571,7 +570,7 @@ virNWFilterDetermineMissingVarsRec(virNWFilterDefPtr filter, virNWFilterHashTableFree(tmpvars); - virNWFilterObjUnlock(obj); + virNWFilterObjEndAPI(&obj); if (rc < 0) break; } @@ -845,7 +844,7 @@ virNWFilterInstantiateFilterUpdate(virNWFilterDriverStatePtr driver, virNWFilterHashTableFree(vars1); err_exit: - virNWFilterObjUnlock(obj); + virNWFilterObjEndAPI(&obj); VIR_FREE(str_ipaddr); VIR_FREE(str_macaddr); -- 2.9.4
On 06/02/2017 12:25 PM, John Ferlan wrote:
"Simple" conversion to the virObjectLockable object isn't quite possible yet because the nwfilter lock requires usage of recursive locking due to algorithms handing filter "<rule>"'s and "<filterref>"'s. The list search logic would also benefit from using hash tables for lookups. So this patch is step 1 in the process - add the @refs to _virNWFilterObj and modify the algorithms to use (a temporary) virNWFilterObj{Ref|Unref} in order to set things up for the list logic to utilize virObjectLockable hash tables.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virnwfilterobj.c | 75 ++++++++++++++++++++++++++-------- src/conf/virnwfilterobj.h | 15 ++++--- src/libvirt_private.syms | 5 ++- src/nwfilter/nwfilter_driver.c | 13 +++--- src/nwfilter/nwfilter_gentech_driver.c | 11 +++-- 5 files changed, 80 insertions(+), 39 deletions(-)
diff --git a/src/conf/virnwfilterobj.c b/src/conf/virnwfilterobj.c index b21b570..99be59c 100644 --- a/src/conf/virnwfilterobj.c +++ b/src/conf/virnwfilterobj.c @@ -23,6 +23,7 @@ #include "datatypes.h"
#include "viralloc.h" +#include "viratomic.h" #include "virerror.h" #include "virfile.h" #include "virlog.h" @@ -33,8 +34,15 @@
VIR_LOG_INIT("conf.virnwfilterobj");
+static void +virNWFilterObjLock(virNWFilterObjPtr obj); + +static void +virNWFilterObjUnlock(virNWFilterObjPtr obj); + struct _virNWFilterObj { virMutex lock; + int refs;
bool wantRemoved;
@@ -67,11 +75,24 @@ virNWFilterObjNew(virNWFilterDefPtr def)
virNWFilterObjLock(obj); obj->def = def; + virAtomicIntSet(&obj->refs, 1);
Technically, this doesn't need to be virAtomic. Bare assignment would work as: a) the object is locked at this point, b) there's no other reference to the object (we are just creating the first one). But I'm fine with leaving this as is, just wanted to point out my comment.
return obj; }
+void +virNWFilterObjEndAPI(virNWFilterObjPtr *obj) +{ + if (!*obj) + return; + + virNWFilterObjUnlock(*obj); + virNWFilterObjUnref(*obj); + *obj = NULL; +} + + virNWFilterDefPtr virNWFilterObjGetDef(virNWFilterObjPtr obj) { @@ -109,12 +130,33 @@ virNWFilterObjFree(virNWFilterObjPtr obj) }
+virNWFilterObjPtr +virNWFilterObjRef(virNWFilterObjPtr obj) +{ + if (obj) + virAtomicIntInc(&obj->refs); + return obj; +} + + +bool +virNWFilterObjUnref(virNWFilterObjPtr obj) +{ + bool lastRef; + if (obj) + return false;
This can hardly work. The condition needs to be inverted.
+ if ((lastRef = virAtomicIntDecAndTest(&obj->refs))) + virNWFilterObjFree(obj); + return !lastRef; +} + + void virNWFilterObjListFree(virNWFilterObjListPtr nwfilters) { size_t i; for (i = 0; i < nwfilters->count; i++) - virNWFilterObjFree(nwfilters->objs[i]); + virNWFilterObjUnref(nwfilters->objs[i]); VIR_FREE(nwfilters->objs); VIR_FREE(nwfilters); } @@ -143,7 +185,7 @@ virNWFilterObjListRemove(virNWFilterObjListPtr nwfilters, virNWFilterObjLock(nwfilters->objs[i]); if (nwfilters->objs[i] == obj) { virNWFilterObjUnlock(nwfilters->objs[i]); - virNWFilterObjFree(nwfilters->objs[i]); + virNWFilterObjUnref(nwfilters->objs[i]);
VIR_DELETE_ELEMENT(nwfilters->objs, i, nwfilters->count); break; @@ -166,7 +208,7 @@ virNWFilterObjListFindByUUID(virNWFilterObjListPtr nwfilters, virNWFilterObjLock(obj); def = obj->def; if (!memcmp(def->uuid, uuid, VIR_UUID_BUFLEN)) - return obj; + return virNWFilterObjRef(obj); virNWFilterObjUnlock(obj); }
@@ -187,7 +229,7 @@ virNWFilterObjListFindByName(virNWFilterObjListPtr nwfilters, virNWFilterObjLock(obj); def = obj->def; if (STREQ_NULLABLE(def->name, name)) - return obj; + return virNWFilterObjRef(obj); virNWFilterObjUnlock(obj); }
@@ -210,7 +252,7 @@ virNWFilterObjListFindInstantiateFilter(virNWFilterObjListPtr nwfilters, if (virNWFilterObjWantRemoved(obj)) { virReportError(VIR_ERR_NO_NWFILTER, _("Filter '%s' is in use."), filtername); - virNWFilterObjUnlock(obj); + virNWFilterObjEndAPI(&obj); return NULL;
Can we remove this "return NULL" line and rely on "return obj" which follow immediately (not to be seen in the context here though)?
}
@@ -245,7 +287,7 @@ _virNWFilterObjListDefLoopDetect(virNWFilterObjListPtr nwfilters, if (obj) { rc = _virNWFilterObjListDefLoopDetect(nwfilters, obj->def, filtername); - virNWFilterObjUnlock(obj); + virNWFilterObjEndAPI(&obj); if (rc < 0) break; } @@ -338,10 +380,10 @@ virNWFilterObjListAssignDef(virNWFilterObjListPtr nwfilters, _("filter with same UUID but different name " "('%s') already exists"), objdef->name); - virNWFilterObjUnlock(obj); + virNWFilterObjEndAPI(&obj); return NULL; } - virNWFilterObjUnlock(obj); + virNWFilterObjEndAPI(&obj); } else { if ((obj = virNWFilterObjListFindByName(nwfilters, def->name))) { char uuidstr[VIR_UUID_STRING_BUFLEN]; @@ -351,7 +393,7 @@ virNWFilterObjListAssignDef(virNWFilterObjListPtr nwfilters, virReportError(VIR_ERR_OPERATION_FAILED, _("filter '%s' already exists with uuid %s"), def->name, uuidstr); - virNWFilterObjUnlock(obj); + virNWFilterObjEndAPI(&obj); return NULL; } } @@ -362,7 +404,6 @@ virNWFilterObjListAssignDef(virNWFilterObjListPtr nwfilters, return NULL; }
- if ((obj = virNWFilterObjListFindByName(nwfilters, def->name))) {
objdef = obj->def;
I've got an unrelated question: here, after this line you can find the following code: if (virNWFilterDefEqual(def, objdef, false)) { virNWFilterDefFree(objdef); obj->def = def; return obj; } Firstly, I think we can s/false/true/ because if UUIDs were not the same, we would have errored out way sooner. But more importantly, if definition is equal what's the point in replacing it?
@@ -376,7 +417,7 @@ virNWFilterObjListAssignDef(virNWFilterObjListPtr nwfilters, /* trigger the update on VMs referencing the filter */ if (virNWFilterTriggerVMFilterRebuild() < 0) { obj->newDef = NULL; - virNWFilterObjUnlock(obj); + virNWFilterObjEndAPI(&obj); return NULL; }
@@ -397,12 +438,12 @@ virNWFilterObjListAssignDef(virNWFilterObjListPtr nwfilters, if (VIR_APPEND_ELEMENT_COPY(nwfilters->objs, nwfilters->count, obj) < 0) goto error;
- return obj; + return virNWFilterObjRef(obj);
error: obj->def = NULL; virNWFilterObjUnlock(obj); - virNWFilterObjFree(obj); + virNWFilterObjUnref(obj); return NULL; }
@@ -600,8 +641,8 @@ virNWFilterObjListLoadAllConfigs(virNWFilterObjListPtr nwfilters, continue;
obj = virNWFilterObjListLoadConfig(nwfilters, configDir, entry->d_name); - if (obj) - virNWFilterObjUnlock(obj); + + virNWFilterObjEndAPI(&obj); }
VIR_DIR_CLOSE(dir); @@ -609,14 +650,14 @@ virNWFilterObjListLoadAllConfigs(virNWFilterObjListPtr nwfilters, }
-void +static void virNWFilterObjLock(virNWFilterObjPtr obj) { virMutexLock(&obj->lock); }
-void +static void virNWFilterObjUnlock(virNWFilterObjPtr obj) { virMutexUnlock(&obj->lock); diff --git a/src/conf/virnwfilterobj.h b/src/conf/virnwfilterobj.h index 85c8ead..31aa345 100644 --- a/src/conf/virnwfilterobj.h +++ b/src/conf/virnwfilterobj.h @@ -41,6 +41,9 @@ struct _virNWFilterDriverState { bool watchingFirewallD; };
+void +virNWFilterObjEndAPI(virNWFilterObjPtr *obj); + virNWFilterDefPtr virNWFilterObjGetDef(virNWFilterObjPtr obj);
@@ -50,6 +53,12 @@ virNWFilterObjGetNewDef(virNWFilterObjPtr obj); bool virNWFilterObjWantRemoved(virNWFilterObjPtr obj);
+virNWFilterObjPtr +virNWFilterObjRef(virNWFilterObjPtr obj); + +bool +virNWFilterObjUnref(virNWFilterObjPtr obj); +
ACK Michal
On 07/13/2017 10:40 AM, Michal Privoznik wrote:
On 06/02/2017 12:25 PM, John Ferlan wrote:
"Simple" conversion to the virObjectLockable object isn't quite possible yet because the nwfilter lock requires usage of recursive locking due to algorithms handing filter "<rule>"'s and "<filterref>"'s. The list search logic would also benefit from using hash tables for lookups. So this patch is step 1 in the process - add the @refs to _virNWFilterObj and modify the algorithms to use (a temporary) virNWFilterObj{Ref|Unref} in order to set things up for the list logic to utilize virObjectLockable hash tables.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virnwfilterobj.c | 75 ++++++++++++++++++++++++++-------- src/conf/virnwfilterobj.h | 15 ++++--- src/libvirt_private.syms | 5 ++- src/nwfilter/nwfilter_driver.c | 13 +++--- src/nwfilter/nwfilter_gentech_driver.c | 11 +++-- 5 files changed, 80 insertions(+), 39 deletions(-)
FWIW: I've pushed 1, 4, and 8-13 since they were ACK. Since 2, 3, and 5-7 are being dropped and I want to insert a revert prior to here (separate patch posted) - I'll respond to the queries from 14-17, but they will get reposted as a v2 once the revert is in.
diff --git a/src/conf/virnwfilterobj.c b/src/conf/virnwfilterobj.c index b21b570..99be59c 100644 --- a/src/conf/virnwfilterobj.c +++ b/src/conf/virnwfilterobj.c @@ -23,6 +23,7 @@ #include "datatypes.h"
#include "viralloc.h" +#include "viratomic.h" #include "virerror.h" #include "virfile.h" #include "virlog.h" @@ -33,8 +34,15 @@
VIR_LOG_INIT("conf.virnwfilterobj");
+static void +virNWFilterObjLock(virNWFilterObjPtr obj); + +static void +virNWFilterObjUnlock(virNWFilterObjPtr obj); + struct _virNWFilterObj { virMutex lock; + int refs;
bool wantRemoved;
@@ -67,11 +75,24 @@ virNWFilterObjNew(virNWFilterDefPtr def)
virNWFilterObjLock(obj); obj->def = def; + virAtomicIntSet(&obj->refs, 1);
Technically, this doesn't need to be virAtomic. Bare assignment would work as: a) the object is locked at this point, b) there's no other reference to the object (we are just creating the first one). But I'm fine with leaving this as is, just wanted to point out my comment.
True, but in order to "show" the progression from point A to point B *and* because of that recursive locking algorithm currently in place, I essentially lifted code from virobject.c (this is from virObjectNew and virObjectLockableNew).
return obj; }
+void +virNWFilterObjEndAPI(virNWFilterObjPtr *obj) +{ + if (!*obj) + return; + + virNWFilterObjUnlock(*obj); + virNWFilterObjUnref(*obj); + *obj = NULL; +} + + virNWFilterDefPtr virNWFilterObjGetDef(virNWFilterObjPtr obj) { @@ -109,12 +130,33 @@ virNWFilterObjFree(virNWFilterObjPtr obj) }
+virNWFilterObjPtr +virNWFilterObjRef(virNWFilterObjPtr obj) +{ + if (obj) + virAtomicIntInc(&obj->refs); + return obj; +} + + +bool +virNWFilterObjUnref(virNWFilterObjPtr obj) +{ + bool lastRef; + if (obj) + return false;
This can hardly work. The condition needs to be inverted.
Oh right.... Good eyes! My testing wouldn't see it because I tested the whole series which would essentially replace this.
+ if ((lastRef = virAtomicIntDecAndTest(&obj->refs))) + virNWFilterObjFree(obj); + return !lastRef; +} + + void virNWFilterObjListFree(virNWFilterObjListPtr nwfilters) { size_t i; for (i = 0; i < nwfilters->count; i++) - virNWFilterObjFree(nwfilters->objs[i]); + virNWFilterObjUnref(nwfilters->objs[i]); VIR_FREE(nwfilters->objs); VIR_FREE(nwfilters); } @@ -143,7 +185,7 @@ virNWFilterObjListRemove(virNWFilterObjListPtr nwfilters, virNWFilterObjLock(nwfilters->objs[i]); if (nwfilters->objs[i] == obj) { virNWFilterObjUnlock(nwfilters->objs[i]); - virNWFilterObjFree(nwfilters->objs[i]); + virNWFilterObjUnref(nwfilters->objs[i]);
VIR_DELETE_ELEMENT(nwfilters->objs, i, nwfilters->count); break; @@ -166,7 +208,7 @@ virNWFilterObjListFindByUUID(virNWFilterObjListPtr nwfilters, virNWFilterObjLock(obj); def = obj->def; if (!memcmp(def->uuid, uuid, VIR_UUID_BUFLEN)) - return obj; + return virNWFilterObjRef(obj); virNWFilterObjUnlock(obj); }
@@ -187,7 +229,7 @@ virNWFilterObjListFindByName(virNWFilterObjListPtr nwfilters, virNWFilterObjLock(obj); def = obj->def; if (STREQ_NULLABLE(def->name, name)) - return obj; + return virNWFilterObjRef(obj); virNWFilterObjUnlock(obj); }
@@ -210,7 +252,7 @@ virNWFilterObjListFindInstantiateFilter(virNWFilterObjListPtr nwfilters, if (virNWFilterObjWantRemoved(obj)) { virReportError(VIR_ERR_NO_NWFILTER, _("Filter '%s' is in use."), filtername); - virNWFilterObjUnlock(obj); + virNWFilterObjEndAPI(&obj); return NULL;
Can we remove this "return NULL" line and rely on "return obj" which follow immediately (not to be seen in the context here though)?
Sure that's fine, same result.
}
@@ -245,7 +287,7 @@ _virNWFilterObjListDefLoopDetect(virNWFilterObjListPtr nwfilters, if (obj) { rc = _virNWFilterObjListDefLoopDetect(nwfilters, obj->def, filtername); - virNWFilterObjUnlock(obj); + virNWFilterObjEndAPI(&obj); if (rc < 0) break; } @@ -338,10 +380,10 @@ virNWFilterObjListAssignDef(virNWFilterObjListPtr nwfilters, _("filter with same UUID but different name " "('%s') already exists"), objdef->name); - virNWFilterObjUnlock(obj); + virNWFilterObjEndAPI(&obj); return NULL; } - virNWFilterObjUnlock(obj); + virNWFilterObjEndAPI(&obj); } else { if ((obj = virNWFilterObjListFindByName(nwfilters, def->name))) { char uuidstr[VIR_UUID_STRING_BUFLEN]; @@ -351,7 +393,7 @@ virNWFilterObjListAssignDef(virNWFilterObjListPtr nwfilters, virReportError(VIR_ERR_OPERATION_FAILED, _("filter '%s' already exists with uuid %s"), def->name, uuidstr); - virNWFilterObjUnlock(obj); + virNWFilterObjEndAPI(&obj); return NULL; } } @@ -362,7 +404,6 @@ virNWFilterObjListAssignDef(virNWFilterObjListPtr nwfilters, return NULL; }
- if ((obj = virNWFilterObjListFindByName(nwfilters, def->name))) {
objdef = obj->def;
I've got an unrelated question: here, after this line you can find the following code:
if (virNWFilterDefEqual(def, objdef, false)) { virNWFilterDefFree(objdef); obj->def = def; return obj; }
Firstly, I think we can s/false/true/ because if UUIDs were not the same, we would have errored out way sooner. But more importantly, if definition is equal what's the point in replacing it?
Let's see, looks like commit id '46a811db07' added the if we cannot find by UUID, then let's find by Name and if the name exists, then cause failure because we have a defined Name with a different UUID. But it didn't adjust this algorithm; however, rather than change to passing true, since this is the only caller, the @cmpUUIDs should be removed from virNWFilterDefEqual. In a separate followup patch of course. As noted above - I'll fix stuff up here, add the patch, and post a v2 Tks - John
@@ -376,7 +417,7 @@ virNWFilterObjListAssignDef(virNWFilterObjListPtr nwfilters, /* trigger the update on VMs referencing the filter */ if (virNWFilterTriggerVMFilterRebuild() < 0) { obj->newDef = NULL; - virNWFilterObjUnlock(obj); + virNWFilterObjEndAPI(&obj); return NULL; }
@@ -397,12 +438,12 @@ virNWFilterObjListAssignDef(virNWFilterObjListPtr nwfilters, if (VIR_APPEND_ELEMENT_COPY(nwfilters->objs, nwfilters->count, obj) < 0) goto error;
- return obj; + return virNWFilterObjRef(obj);
error: obj->def = NULL; virNWFilterObjUnlock(obj); - virNWFilterObjFree(obj); + virNWFilterObjUnref(obj); return NULL; }
@@ -600,8 +641,8 @@ virNWFilterObjListLoadAllConfigs(virNWFilterObjListPtr nwfilters, continue;
obj = virNWFilterObjListLoadConfig(nwfilters, configDir, entry->d_name); - if (obj) - virNWFilterObjUnlock(obj); + + virNWFilterObjEndAPI(&obj); }
VIR_DIR_CLOSE(dir); @@ -609,14 +650,14 @@ virNWFilterObjListLoadAllConfigs(virNWFilterObjListPtr nwfilters, }
-void +static void virNWFilterObjLock(virNWFilterObjPtr obj) { virMutexLock(&obj->lock); }
-void +static void virNWFilterObjUnlock(virNWFilterObjPtr obj) { virMutexUnlock(&obj->lock); diff --git a/src/conf/virnwfilterobj.h b/src/conf/virnwfilterobj.h index 85c8ead..31aa345 100644 --- a/src/conf/virnwfilterobj.h +++ b/src/conf/virnwfilterobj.h @@ -41,6 +41,9 @@ struct _virNWFilterDriverState { bool watchingFirewallD; };
+void +virNWFilterObjEndAPI(virNWFilterObjPtr *obj); + virNWFilterDefPtr virNWFilterObjGetDef(virNWFilterObjPtr obj);
@@ -50,6 +53,12 @@ virNWFilterObjGetNewDef(virNWFilterObjPtr obj); bool virNWFilterObjWantRemoved(virNWFilterObjPtr obj);
+virNWFilterObjPtr +virNWFilterObjRef(virNWFilterObjPtr obj); + +bool +virNWFilterObjUnref(virNWFilterObjPtr obj); +
ACK
Michal
Perhaps a bit out of order from the normal convert driver object to virObjectLockable, then convert the driver object list. However, as it turns out nwfilter objects have been using a recursive mutex for which the common virObject code does not want to use. So, if we convert the nwfilter object list to use virObjectLockable, then it will be more possible to make the necessary adjustments to the virNWFilterObjListFindInstantiateFilter algorithm in order to handle a recursive lock operation required as part of the <rule> and <filterref> (or "inc" list) processing. This patch takes the plunge, modifying the nwfilter object list handling code to utilize hash tables for both the UUID and Name for which an nwfilter def would utilize. This makes lookup by either "key" possible without needing to first lock the object in order to find a match as long as of course the object list itself is locked. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virnwfilterobj.c | 395 +++++++++++++++++++++++++++++------------ src/nwfilter/nwfilter_driver.c | 4 + 2 files changed, 286 insertions(+), 113 deletions(-) diff --git a/src/conf/virnwfilterobj.c b/src/conf/virnwfilterobj.c index 99be59c..84ef7d1 100644 --- a/src/conf/virnwfilterobj.c +++ b/src/conf/virnwfilterobj.c @@ -53,10 +53,34 @@ struct _virNWFilterObj { }; struct _virNWFilterObjList { - size_t count; - virNWFilterObjPtr *objs; + virObjectLockable parent; + + /* uuid string -> virNWFilterObj mapping + * for O(1), lockless lookup-by-uuid */ + virHashTable *objs; + + /* name -> virNWFilterObj mapping for O(1), + * lockless lookup-by-name */ + virHashTable *objsName; }; +static virClassPtr virNWFilterObjListClass; +static void virNWFilterObjListDispose(void *opaque); + +static int +virNWFilterObjOnceInit(void) +{ + if (!(virNWFilterObjListClass = virClassNew(virClassForObjectLockable(), + "virNWFilterObjList", + sizeof(virNWFilterObjList), + virNWFilterObjListDispose))) + return -1; + + return 0; +} + +VIR_ONCE_GLOBAL_INIT(virNWFilterObj) + static virNWFilterObjPtr virNWFilterObjNew(virNWFilterDefPtr def) @@ -151,14 +175,30 @@ virNWFilterObjUnref(virNWFilterObjPtr obj) } +static void +virNWFilterObjListDispose(void *opaque) +{ + virNWFilterObjListPtr nwfilters = opaque; + + virHashFree(nwfilters->objs); + virHashFree(nwfilters->objsName); +} + + void virNWFilterObjListFree(virNWFilterObjListPtr nwfilters) { - size_t i; - for (i = 0; i < nwfilters->count; i++) - virNWFilterObjUnref(nwfilters->objs[i]); - VIR_FREE(nwfilters->objs); - VIR_FREE(nwfilters); + virObjectUnref(nwfilters); +} + + +static void +virNWFilterObjListObjsFreeData(void *payload, + const void *name ATTRIBUTE_UNUSED) +{ + virNWFilterObjPtr obj = payload; + + virNWFilterObjUnref(obj); } @@ -167,8 +207,24 @@ virNWFilterObjListNew(void) { virNWFilterObjListPtr nwfilters; - if (VIR_ALLOC(nwfilters) < 0) + if (virNWFilterObjInitialize() < 0) + return NULL; + + if (!(nwfilters = virObjectLockableNew(virNWFilterObjListClass))) + return NULL; + + if (!(nwfilters->objs = virHashCreate(10, virNWFilterObjListObjsFreeData))) { + virObjectUnref(nwfilters); + return NULL; + } + + if (!(nwfilters->objsName = + virHashCreate(10, virNWFilterObjListObjsFreeData))) { + virHashFree(nwfilters->objs); + virObjectUnref(nwfilters); return NULL; + } + return nwfilters; } @@ -177,21 +233,34 @@ void virNWFilterObjListRemove(virNWFilterObjListPtr nwfilters, virNWFilterObjPtr obj) { - size_t i; + char uuidstr[VIR_UUID_STRING_BUFLEN]; + virNWFilterDefPtr def; + if (!obj) + return; + def = obj->def; + + virUUIDFormat(def->uuid, uuidstr); + virNWFilterObjRef(obj); virNWFilterObjUnlock(obj); + virObjectLock(nwfilters); + virNWFilterObjLock(obj); + virHashRemoveEntry(nwfilters->objs, uuidstr); + virHashRemoveEntry(nwfilters->objsName, def->name); + virNWFilterObjUnlock(obj); + virNWFilterObjUnref(obj); + virObjectUnlock(nwfilters); +} - for (i = 0; i < nwfilters->count; i++) { - virNWFilterObjLock(nwfilters->objs[i]); - if (nwfilters->objs[i] == obj) { - virNWFilterObjUnlock(nwfilters->objs[i]); - virNWFilterObjUnref(nwfilters->objs[i]); - VIR_DELETE_ELEMENT(nwfilters->objs, i, nwfilters->count); - break; - } - virNWFilterObjUnlock(nwfilters->objs[i]); - } +static virNWFilterObjPtr +virNWFilterObjListFindByUUIDLocked(virNWFilterObjListPtr nwfilters, + const unsigned char *uuid) +{ + char uuidstr[VIR_UUID_STRING_BUFLEN]; + + virUUIDFormat(uuid, uuidstr); + return virNWFilterObjRef(virHashLookup(nwfilters->objs, uuidstr)); } @@ -199,20 +268,22 @@ virNWFilterObjPtr virNWFilterObjListFindByUUID(virNWFilterObjListPtr nwfilters, const unsigned char *uuid) { - size_t i; virNWFilterObjPtr obj; - virNWFilterDefPtr def; - for (i = 0; i < nwfilters->count; i++) { - obj = nwfilters->objs[i]; + virObjectLock(nwfilters); + obj = virNWFilterObjListFindByUUIDLocked(nwfilters, uuid); + virObjectUnlock(nwfilters); + if (obj) virNWFilterObjLock(obj); - def = obj->def; - if (!memcmp(def->uuid, uuid, VIR_UUID_BUFLEN)) - return virNWFilterObjRef(obj); - virNWFilterObjUnlock(obj); - } + return obj; +} - return NULL; + +static virNWFilterObjPtr +virNWFilterObjListFindByNameLocked(virNWFilterObjListPtr nwfilters, + const char *name) +{ + return virNWFilterObjRef(virHashLookup(nwfilters->objsName, name)); } @@ -220,20 +291,15 @@ virNWFilterObjPtr virNWFilterObjListFindByName(virNWFilterObjListPtr nwfilters, const char *name) { - size_t i; virNWFilterObjPtr obj; - virNWFilterDefPtr def; - for (i = 0; i < nwfilters->count; i++) { - obj = nwfilters->objs[i]; + virObjectLock(nwfilters); + obj = virNWFilterObjListFindByNameLocked(nwfilters, name); + virObjectUnlock(nwfilters); + if (obj) virNWFilterObjLock(obj); - def = obj->def; - if (STREQ_NULLABLE(def->name, name)) - return virNWFilterObjRef(obj); - virNWFilterObjUnlock(obj); - } - return NULL; + return obj; } @@ -282,9 +348,10 @@ _virNWFilterObjListDefLoopDetect(virNWFilterObjListPtr nwfilters, break; } - obj = virNWFilterObjListFindByName(nwfilters, - entry->include->filterref); + obj = virNWFilterObjListFindByNameLocked(nwfilters, + entry->include->filterref); if (obj) { + virObjectLock(obj); rc = _virNWFilterObjListDefLoopDetect(nwfilters, obj->def, filtername); virNWFilterObjEndAPI(&obj); @@ -306,6 +373,8 @@ _virNWFilterObjListDefLoopDetect(virNWFilterObjListPtr nwfilters, * Detect a loop introduced through the filters being able to * reference each other. * + * NB: Enter with nwfilters locked + * * Returns 0 in case no loop was detected, -1 otherwise. */ static int @@ -371,8 +440,12 @@ virNWFilterObjListAssignDef(virNWFilterObjListPtr nwfilters, { virNWFilterObjPtr obj; virNWFilterDefPtr objdef; + char uuidstr[VIR_UUID_STRING_BUFLEN]; - if ((obj = virNWFilterObjListFindByUUID(nwfilters, def->uuid))) { + virObjectLock(nwfilters); + + if ((obj = virNWFilterObjListFindByUUIDLocked(nwfilters, def->uuid))) { + virNWFilterObjLock(obj); objdef = obj->def; if (STRNEQ(def->name, objdef->name)) { @@ -381,19 +454,21 @@ virNWFilterObjListAssignDef(virNWFilterObjListPtr nwfilters, "('%s') already exists"), objdef->name); virNWFilterObjEndAPI(&obj); + virObjectUnlock(nwfilters); return NULL; } virNWFilterObjEndAPI(&obj); } else { - if ((obj = virNWFilterObjListFindByName(nwfilters, def->name))) { - char uuidstr[VIR_UUID_STRING_BUFLEN]; + if ((obj = virNWFilterObjListFindByNameLocked(nwfilters, def->name))) { + virNWFilterObjLock(obj); objdef = obj->def; virUUIDFormat(objdef->uuid, uuidstr); virReportError(VIR_ERR_OPERATION_FAILED, _("filter '%s' already exists with uuid %s"), def->name, uuidstr); virNWFilterObjEndAPI(&obj); + virObjectUnlock(nwfilters); return NULL; } } @@ -401,16 +476,18 @@ virNWFilterObjListAssignDef(virNWFilterObjListPtr nwfilters, if (virNWFilterObjListDefLoopDetect(nwfilters, def) < 0) { virReportError(VIR_ERR_OPERATION_FAILED, "%s", _("filter would introduce a loop")); + virObjectUnlock(nwfilters); return NULL; } - if ((obj = virNWFilterObjListFindByName(nwfilters, def->name))) { + if ((obj = virNWFilterObjListFindByNameLocked(nwfilters, def->name))) { + virNWFilterObjLock(obj); objdef = obj->def; if (virNWFilterDefEqual(def, objdef, false)) { virNWFilterDefFree(objdef); obj->def = def; - return obj; + goto cleanup; } obj->newDef = def; @@ -418,13 +495,14 @@ virNWFilterObjListAssignDef(virNWFilterObjListPtr nwfilters, if (virNWFilterTriggerVMFilterRebuild() < 0) { obj->newDef = NULL; virNWFilterObjEndAPI(&obj); + virObjectUnlock(nwfilters); return NULL; } virNWFilterDefFree(objdef); obj->def = def; obj->newDef = NULL; - return obj; + goto cleanup; } if (!(obj = virNWFilterObjNew(def))) @@ -435,36 +513,107 @@ virNWFilterObjListAssignDef(virNWFilterObjListPtr nwfilters, if (!(obj->configFile = virFileBuildPath(configDir, objdef->name, ".xml"))) goto error; - if (VIR_APPEND_ELEMENT_COPY(nwfilters->objs, nwfilters->count, obj) < 0) + virUUIDFormat(def->uuid, uuidstr); + if (virHashAddEntry(nwfilters->objs, uuidstr, obj) < 0) + goto error; + virNWFilterObjRef(obj); + + if (virHashAddEntry(nwfilters->objsName, objdef->name, obj) < 0) { + virHashRemoveEntry(nwfilters->objs, uuidstr); goto error; + } + virNWFilterObjRef(obj); - return virNWFilterObjRef(obj); + cleanup: + virObjectUnlock(nwfilters); + return obj; error: obj->def = NULL; virNWFilterObjUnlock(obj); virNWFilterObjUnref(obj); + virObjectUnlock(nwfilters); return NULL; } +struct virNWFilterCountData { + virConnectPtr conn; + virNWFilterObjListFilter aclfilter; + int nelems; +}; + +static int +virNWFilterObjListNumOfNWFiltersCallback(void *payload, + const void *name ATTRIBUTE_UNUSED, + void *opaque) +{ + virNWFilterObjPtr obj = payload; + struct virNWFilterCountData *data = opaque; + + virObjectLock(obj); + if (!data->aclfilter || data->aclfilter(data->conn, obj->def)) + data->nelems++; + virObjectUnlock(obj); + return 0; +} + + int virNWFilterObjListNumOfNWFilters(virNWFilterObjListPtr nwfilters, virConnectPtr conn, virNWFilterObjListFilter aclfilter) { - size_t i; - int nfilters = 0; + struct virNWFilterCountData data = { .conn = conn, + .aclfilter = aclfilter, .nelems = 0 }; - for (i = 0; i < nwfilters->count; i++) { - virNWFilterObjPtr obj = nwfilters->objs[i]; - virNWFilterObjLock(obj); - if (!aclfilter || aclfilter(conn, obj->def)) - nfilters++; - virNWFilterObjUnlock(obj); + virObjectLock(nwfilters); + virHashForEach(nwfilters->objs, virNWFilterObjListNumOfNWFiltersCallback, + &data); + virObjectUnlock(nwfilters); + + return data.nelems; +} + + +struct virNWFilterListData { + virConnectPtr conn; + virNWFilterObjListFilter aclfilter; + int nelems; + char **elems; + int maxelems; + bool error; +}; + +static int +virNWFilterObjListGetNamesCallback(void *payload, + const void *name ATTRIBUTE_UNUSED, + void *opaque) +{ + virNWFilterObjPtr obj = payload; + virNWFilterDefPtr def; + struct virNWFilterListData *data = opaque; + + if (data->error) + return 0; + + if (data->maxelems >= 0 && data->nelems == data->maxelems) + return 0; + + virObjectLock(obj); + def = obj->def; + + if (!data->aclfilter || data->aclfilter(data->conn, def)) { + if (VIR_STRDUP(data->elems[data->nelems], def->name) < 0) { + data->error = true; + goto cleanup; + } + data->nelems++; } - return nfilters; + cleanup: + virObjectUnlock(obj); + return 0; } @@ -475,82 +624,102 @@ virNWFilterObjListGetNames(virNWFilterObjListPtr nwfilters, char **const names, int maxnames) { - int nnames = 0; - size_t i; - virNWFilterDefPtr def; + struct virNWFilterListData data = { .conn = conn, .aclfilter = aclfilter, + .nelems = 0, .elems = names, .maxelems = maxnames, .error = false }; - for (i = 0; i < nwfilters->count && nnames < maxnames; i++) { - virNWFilterObjPtr obj = nwfilters->objs[i]; - virNWFilterObjLock(obj); - def = obj->def; - if (!aclfilter || aclfilter(conn, def)) { - if (VIR_STRDUP(names[nnames], def->name) < 0) { - virNWFilterObjUnlock(obj); - goto failure; - } - nnames++; - } - virNWFilterObjUnlock(obj); - } + virObjectLock(nwfilters); + virHashForEach(nwfilters->objs, virNWFilterObjListGetNamesCallback, &data); + virObjectUnlock(nwfilters); - return nnames; + if (data.error) + goto error; - failure: - while (--nnames >= 0) - VIR_FREE(names[nnames]); + return data.nelems; + error: + while (--data.nelems >= 0) + VIR_FREE(data.elems[data.nelems]); return -1; } -int -virNWFilterObjListExport(virConnectPtr conn, - virNWFilterObjListPtr nwfilters, - virNWFilterPtr **filters, - virNWFilterObjListFilter aclfilter) +struct virNWFilterExportData { + virConnectPtr conn; + virNWFilterObjListFilter aclfilter; + virNWFilterPtr *filters; + int nfilters; + bool error; +}; + +static int +virNWFilterObjListExportCallback(void *payload, + const void *name ATTRIBUTE_UNUSED, + void *opaque) { - virNWFilterPtr *tmp_filters = NULL; - int nfilters = 0; - virNWFilterPtr filter = NULL; - virNWFilterObjPtr obj = NULL; + virNWFilterObjPtr obj = payload; virNWFilterDefPtr def; - size_t i; - int ret = -1; + struct virNWFilterExportData *data = opaque; + virNWFilterPtr nwfilter; + + if (data->error) + return 0; + + virObjectLock(obj); + def = obj->def; - if (!filters) { - ret = nwfilters->count; + if (data->aclfilter && !data->aclfilter(data->conn, def)) + goto cleanup; + + if (!data->filters) { + data->nfilters++; goto cleanup; } - if (VIR_ALLOC_N(tmp_filters, nwfilters->count + 1) < 0) + if (!(nwfilter = virGetNWFilter(data->conn, def->name, def->uuid))) { + data->error = true; goto cleanup; + } + data->filters[data->nfilters++] = nwfilter; - for (i = 0; i < nwfilters->count; i++) { - obj = nwfilters->objs[i]; - virNWFilterObjLock(obj); - def = obj->def; - if (!aclfilter || aclfilter(conn, def)) { - if (!(filter = virGetNWFilter(conn, def->name, def->uuid))) { - virNWFilterObjUnlock(obj); - goto cleanup; - } - tmp_filters[nfilters++] = filter; - } - virNWFilterObjUnlock(obj); + cleanup: + virObjectUnlock(obj); + return 0; +} + + +int +virNWFilterObjListExport(virConnectPtr conn, + virNWFilterObjListPtr nwfilters, + virNWFilterPtr **filters, + virNWFilterObjListFilter aclfilter) +{ + struct virNWFilterExportData data = { .conn = conn, .aclfilter = aclfilter, + .filters = NULL, .nfilters = 0, .error = false }; + + virObjectLock(nwfilters); + if (filters && + VIR_ALLOC_N(data.filters, virHashSize(nwfilters->objs) + 1) < 0) { + virObjectUnlock(nwfilters); + return -1; } - *filters = tmp_filters; - tmp_filters = NULL; - ret = nfilters; + virHashForEach(nwfilters->objs, virNWFilterObjListExportCallback, &data); + virObjectUnlock(nwfilters); - cleanup: - if (tmp_filters) { - for (i = 0; i < nfilters; i ++) - virObjectUnref(tmp_filters[i]); + if (data.error) + goto cleanup; + + if (data.filters) { + /* trim the array to the final size */ + ignore_value(VIR_REALLOC_N(data.filters, data.nfilters + 1)); + *filters = data.filters; } - VIR_FREE(tmp_filters); - return ret; + return data.nfilters; + + cleanup: + virObjectListFree(data.filters); + return -1; } diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c index b6e11e6..3f05797 100644 --- a/src/nwfilter/nwfilter_driver.c +++ b/src/nwfilter/nwfilter_driver.c @@ -518,6 +518,8 @@ nwfilterDefineXML(virConnectPtr conn, if (virNWFilterObjSaveConfig(obj) < 0) { virNWFilterObjListRemove(driver->nwfilters, obj); + virNWFilterObjUnref(obj); + obj = NULL; goto cleanup; } @@ -563,6 +565,8 @@ nwfilterUndefine(virNWFilterPtr nwfilter) goto cleanup; virNWFilterObjListRemove(driver->nwfilters, obj); + virNWFilterObjUnref(obj); + obj = NULL; ret = 0; cleanup: -- 2.9.4
On 06/02/2017 12:25 PM, John Ferlan wrote:
Perhaps a bit out of order from the normal convert driver object to virObjectLockable, then convert the driver object list. However, as it turns out nwfilter objects have been using a recursive mutex for which the common virObject code does not want to use.
So, if we convert the nwfilter object list to use virObjectLockable, then it will be more possible to make the necessary adjustments to the virNWFilterObjListFindInstantiateFilter algorithm in order to handle a recursive lock operation required as part of the <rule> and <filterref> (or "inc" list) processing.
This patch takes the plunge, modifying the nwfilter object list handling code to utilize hash tables for both the UUID and Name for which an nwfilter def would utilize. This makes lookup by either "key" possible without needing to first lock the object in order to find a match as long as of course the object list itself is locked.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virnwfilterobj.c | 395 +++++++++++++++++++++++++++++------------ src/nwfilter/nwfilter_driver.c | 4 + 2 files changed, 286 insertions(+), 113 deletions(-)
diff --git a/src/conf/virnwfilterobj.c b/src/conf/virnwfilterobj.c index 99be59c..84ef7d1 100644 --- a/src/conf/virnwfilterobj.c +++ b/src/conf/virnwfilterobj.c @@ -53,10 +53,34 @@ struct _virNWFilterObj { };
struct _virNWFilterObjList { - size_t count; - virNWFilterObjPtr *objs; + virObjectLockable parent; + + /* uuid string -> virNWFilterObj mapping + * for O(1), lockless lookup-by-uuid */ + virHashTable *objs; + + /* name -> virNWFilterObj mapping for O(1), + * lockless lookup-by-name */ + virHashTable *objsName; };
+static virClassPtr virNWFilterObjListClass; +static void virNWFilterObjListDispose(void *opaque); + +static int +virNWFilterObjOnceInit(void) +{ + if (!(virNWFilterObjListClass = virClassNew(virClassForObjectLockable(), + "virNWFilterObjList", + sizeof(virNWFilterObjList), + virNWFilterObjListDispose))) + return -1; + + return 0; +} + +VIR_ONCE_GLOBAL_INIT(virNWFilterObj) +
static virNWFilterObjPtr virNWFilterObjNew(virNWFilterDefPtr def) @@ -151,14 +175,30 @@ virNWFilterObjUnref(virNWFilterObjPtr obj) }
+static void +virNWFilterObjListDispose(void *opaque) +{ + virNWFilterObjListPtr nwfilters = opaque; + + virHashFree(nwfilters->objs); + virHashFree(nwfilters->objsName); +} + + void virNWFilterObjListFree(virNWFilterObjListPtr nwfilters) { - size_t i; - for (i = 0; i < nwfilters->count; i++) - virNWFilterObjUnref(nwfilters->objs[i]); - VIR_FREE(nwfilters->objs); - VIR_FREE(nwfilters); + virObjectUnref(nwfilters); +} + + +static void +virNWFilterObjListObjsFreeData(void *payload, + const void *name ATTRIBUTE_UNUSED) +{ + virNWFilterObjPtr obj = payload; + + virNWFilterObjUnref(obj); }
@@ -167,8 +207,24 @@ virNWFilterObjListNew(void) { virNWFilterObjListPtr nwfilters;
- if (VIR_ALLOC(nwfilters) < 0) + if (virNWFilterObjInitialize() < 0) + return NULL; + + if (!(nwfilters = virObjectLockableNew(virNWFilterObjListClass))) + return NULL; + + if (!(nwfilters->objs = virHashCreate(10, virNWFilterObjListObjsFreeData))) { + virObjectUnref(nwfilters); + return NULL; + } + + if (!(nwfilters->objsName = + virHashCreate(10, virNWFilterObjListObjsFreeData))) {
Again, 86 characters is not that much ;-)
+ virHashFree(nwfilters->objs); + virObjectUnref(nwfilters); return NULL; + } + return nwfilters; }
Otherwise looking good. ACK Michal
The current algorithm required usage of recursive locks because there was no other mechanism available to ensure that the object wasn't deleted whilst the instantiation was taking place. Now that we have object references, lets use those to ensure the object isn't deleted whilst we're working through the instantiation thus removing the need for recursive locks. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virnwfilterobj.c | 13 +++++++++-- src/nwfilter/nwfilter_gentech_driver.c | 40 +++++++++++++++++++--------------- 2 files changed, 34 insertions(+), 19 deletions(-) diff --git a/src/conf/virnwfilterobj.c b/src/conf/virnwfilterobj.c index 84ef7d1..ea1323d 100644 --- a/src/conf/virnwfilterobj.c +++ b/src/conf/virnwfilterobj.c @@ -303,13 +303,22 @@ virNWFilterObjListFindByName(virNWFilterObjListPtr nwfilters, } +/** + * To avoid the need to have recursive locks as a result of how the + * virNWFilterInstantiateFilter processing works, this API will not + * lock the returned object, instead just increase the refcnt of the + * object to the caller to allow the caller to handle. + */ virNWFilterObjPtr virNWFilterObjListFindInstantiateFilter(virNWFilterObjListPtr nwfilters, const char *filtername) { virNWFilterObjPtr obj; - if (!(obj = virNWFilterObjListFindByName(nwfilters, filtername))) { + virObjectLock(nwfilters); + obj = virNWFilterObjListFindByNameLocked(nwfilters, filtername); + virObjectUnlock(nwfilters); + if (!obj) { virReportError(VIR_ERR_INTERNAL_ERROR, _("referenced filter '%s' is missing"), filtername); return NULL; @@ -318,7 +327,7 @@ virNWFilterObjListFindInstantiateFilter(virNWFilterObjListPtr nwfilters, if (virNWFilterObjWantRemoved(obj)) { virReportError(VIR_ERR_NO_NWFILTER, _("Filter '%s' is in use."), filtername); - virNWFilterObjEndAPI(&obj); + virNWFilterObjUnref(obj); return NULL; } diff --git a/src/nwfilter/nwfilter_gentech_driver.c b/src/nwfilter/nwfilter_gentech_driver.c index 4c228ea..40da4cc 100644 --- a/src/nwfilter/nwfilter_gentech_driver.c +++ b/src/nwfilter/nwfilter_gentech_driver.c @@ -56,19 +56,25 @@ static virNWFilterTechDriverPtr filter_tech_drivers[] = { NULL }; -/* Serializes instantiation of filters. This is necessary - * to avoid lock ordering deadlocks. eg virNWFilterInstantiateFilterUpdate - * will hold a lock on a virNWFilterObjPtr. This in turn invokes - * virNWFilterDoInstantiate which invokes virNWFilterDetermineMissingVarsRec - * which invokes virNWFilterObjListFindInstantiateFilter. This iterates over - * every single virNWFilterObjPtr in the list. So if 2 threads try to - * instantiate a filter in parallel, they'll both hold 1 lock at the top level - * in virNWFilterInstantiateFilterUpdate which will cause the other thread - * to deadlock in virNWFilterObjListFindInstantiateFilter. +/* NB: Usage of virNWFilterObjListFindInstantiateFilter will only not lock + * the returned object - only increase the refcnt. This is necessary to avoid + * lock ordering deadlocks or the need for recursive locks for filter objects. + * Since the objects are only used to access the @def for <filterref> and + * <rule> elements and having the @def disappear would not be good, but + * keeping the locks causes too many problems. * - * XXX better long term solution is to make virNWFilterObjList use a - * hash table as is done for virDomainObjList. You can then get - * lockless lookup of objects by name. + * During virNWFilterDefToInst processing an object is fetched and placed + * into the inst->filters lookaside list during virNWFilterIncludeDefToRuleInst + * processing which then recursively calls virNWFilterDefToInst. When the + * object is removed during virNWFilterInstReset, the refcnt is decremented. + * + * Since virNWFilterDefToInst is called during virNWFilterDoInstantiate + * processing which can also invoke virNWFilterDetermineMissingVarsRec + * which invokes virNWFilterObjListFindInstantiateFilter also taking + * reference. In addition the virNWFilterInstantiateFilterUpdate could + * have caused locking collisions, so removing the locks from the + * equation and replacing with just references ensures that the object + * cannot be removed whilst this instantiation is taking place. */ static virMutex updateMutex; @@ -315,7 +321,7 @@ virNWFilterInstReset(virNWFilterInstPtr inst) size_t i; for (i = 0; i < inst->nfilters; i++) - virNWFilterObjEndAPI(&inst->filters[i]); + virNWFilterObjUnref(inst->filters[i]); VIR_FREE(inst->filters); inst->nfilters = 0; @@ -425,7 +431,7 @@ virNWFilterIncludeDefToRuleInst(virNWFilterDriverStatePtr driver, if (ret < 0) virNWFilterInstReset(inst); virNWFilterHashTableFree(tmpvars); - virNWFilterObjEndAPI(&obj); + virNWFilterObjUnref(obj); return ret; } @@ -546,7 +552,7 @@ virNWFilterDetermineMissingVarsRec(virNWFilterDefPtr filter, vars); if (!tmpvars) { rc = -1; - virNWFilterObjEndAPI(&obj); + virNWFilterObjUnref(obj); break; } @@ -570,7 +576,7 @@ virNWFilterDetermineMissingVarsRec(virNWFilterDefPtr filter, virNWFilterHashTableFree(tmpvars); - virNWFilterObjEndAPI(&obj); + virNWFilterObjUnref(obj); if (rc < 0) break; } @@ -844,7 +850,7 @@ virNWFilterInstantiateFilterUpdate(virNWFilterDriverStatePtr driver, virNWFilterHashTableFree(vars1); err_exit: - virNWFilterObjEndAPI(&obj); + virNWFilterObjUnref(obj); VIR_FREE(str_ipaddr); VIR_FREE(str_macaddr); -- 2.9.4
On 06/02/2017 12:25 PM, John Ferlan wrote:
The current algorithm required usage of recursive locks because there was no other mechanism available to ensure that the object wasn't deleted whilst the instantiation was taking place.
Now that we have object references, lets use those to ensure the object isn't deleted whilst we're working through the instantiation thus removing the need for recursive locks.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virnwfilterobj.c | 13 +++++++++-- src/nwfilter/nwfilter_gentech_driver.c | 40 +++++++++++++++++++--------------- 2 files changed, 34 insertions(+), 19 deletions(-)
diff --git a/src/conf/virnwfilterobj.c b/src/conf/virnwfilterobj.c index 84ef7d1..ea1323d 100644 --- a/src/conf/virnwfilterobj.c +++ b/src/conf/virnwfilterobj.c @@ -303,13 +303,22 @@ virNWFilterObjListFindByName(virNWFilterObjListPtr nwfilters, }
+/** + * To avoid the need to have recursive locks as a result of how the + * virNWFilterInstantiateFilter processing works, this API will not + * lock the returned object, instead just increase the refcnt of the + * object to the caller to allow the caller to handle. + */ virNWFilterObjPtr virNWFilterObjListFindInstantiateFilter(virNWFilterObjListPtr nwfilters, const char *filtername) { virNWFilterObjPtr obj;
- if (!(obj = virNWFilterObjListFindByName(nwfilters, filtername))) { + virObjectLock(nwfilters); + obj = virNWFilterObjListFindByNameLocked(nwfilters, filtername); + virObjectUnlock(nwfilters); + if (!obj) { virReportError(VIR_ERR_INTERNAL_ERROR, _("referenced filter '%s' is missing"), filtername); return NULL; @@ -318,7 +327,7 @@ virNWFilterObjListFindInstantiateFilter(virNWFilterObjListPtr nwfilters, if (virNWFilterObjWantRemoved(obj)) { virReportError(VIR_ERR_NO_NWFILTER, _("Filter '%s' is in use."), filtername); - virNWFilterObjEndAPI(&obj); + virNWFilterObjUnref(obj); return NULL; }
So now an unlocked obj is returned. This feels wrong ... So for instance: virNWFilterIncludeDefToRuleInst() calls virNWFilterObjListFindInstantiateFilter(). So it obtains ref'd but unlocked object. Then it calls virNWFilterObjGetDef() over it. Well, we are reading without lock, so we might as well be accessing stale pointer. For instance the following might happen: 1) threadA is in virNWFilterIncludeDefToRuleInst() and calls virNWFilterObjGetDef(). It gets pointer to current definition of nwfilter. 2) threadB starts to update the definition of the object. Since the object is not locked, nothing stops it from doing so. As part of the process, current definition is freed. 3) threadA touches freed data. Michal
On 07/13/2017 10:40 AM, Michal Privoznik wrote:
On 06/02/2017 12:25 PM, John Ferlan wrote:
The current algorithm required usage of recursive locks because there was no other mechanism available to ensure that the object wasn't deleted whilst the instantiation was taking place.
Now that we have object references, lets use those to ensure the object isn't deleted whilst we're working through the instantiation thus removing the need for recursive locks.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virnwfilterobj.c | 13 +++++++++-- src/nwfilter/nwfilter_gentech_driver.c | 40 +++++++++++++++++++--------------- 2 files changed, 34 insertions(+), 19 deletions(-)
diff --git a/src/conf/virnwfilterobj.c b/src/conf/virnwfilterobj.c index 84ef7d1..ea1323d 100644 --- a/src/conf/virnwfilterobj.c +++ b/src/conf/virnwfilterobj.c @@ -303,13 +303,22 @@ virNWFilterObjListFindByName(virNWFilterObjListPtr nwfilters, }
+/** + * To avoid the need to have recursive locks as a result of how the + * virNWFilterInstantiateFilter processing works, this API will not + * lock the returned object, instead just increase the refcnt of the + * object to the caller to allow the caller to handle. + */ virNWFilterObjPtr virNWFilterObjListFindInstantiateFilter(virNWFilterObjListPtr nwfilters, const char *filtername) { virNWFilterObjPtr obj;
- if (!(obj = virNWFilterObjListFindByName(nwfilters, filtername))) { + virObjectLock(nwfilters); + obj = virNWFilterObjListFindByNameLocked(nwfilters, filtername); + virObjectUnlock(nwfilters); + if (!obj) { virReportError(VIR_ERR_INTERNAL_ERROR, _("referenced filter '%s' is missing"), filtername); return NULL; @@ -318,7 +327,7 @@ virNWFilterObjListFindInstantiateFilter(virNWFilterObjListPtr nwfilters, if (virNWFilterObjWantRemoved(obj)) { virReportError(VIR_ERR_NO_NWFILTER, _("Filter '%s' is in use."), filtername); - virNWFilterObjEndAPI(&obj); + virNWFilterObjUnref(obj); return NULL; }
So now an unlocked obj is returned. This feels wrong ... So for instance: virNWFilterIncludeDefToRuleInst() calls
I wish this were fresher in my mind ;-)... But, IIRC the problem is that without recursive locks and without being able to lock the hash table (e.g. nwfilters list), then there was no way to find the filter by Name because we'd already have the lock. So my proposed solution is to skip locking altogether and use object references in order to perform the searches.
virNWFilterObjListFindInstantiateFilter(). So it obtains ref'd but unlocked object. Then it calls virNWFilterObjGetDef() over it. Well, we are reading without lock, so we might as well be accessing stale pointer. For instance the following might happen:
1) threadA is in virNWFilterIncludeDefToRuleInst() and calls virNWFilterObjGetDef(). It gets pointer to current definition of nwfilter.
2) threadB starts to update the definition of the object. Since the object is not locked, nothing stops it from doing so. As part of the process, current definition is freed.
3) threadA touches freed data.
But of course this points out the tragic flaw in the process. What if I introduce something that uses pthread_mutex_trylock. For the "short term" it's no different than pthread_mutex_lock since this is still a recursive lock. The trylock would only be called from this path thus further localizing the issue. Once converting from a recursive to a normal lock, the code would need to handle the EBUSY return indicating the lock was already taken. Since we don't know if it was taken by ourselves or some other thread, then devise a mechanism to ensure no other thread could attempt to lock whilst the processing for virNWFilterObjListFindInstantiateFilter occurs. Of course I'm typing this without having the entire algorithm in mind right now. I'll focus some more on it and hopefully come up with something so that the v2 would be "more correct". Tks- John
Now that we have a bit more control, let's convert our object into a recursive lockable object and let that magic handle the create and lock/unlock. Additionally, we introduce virNWFilterObjEndAPI. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virnwfilterobj.c | 127 ++++++++++----------------------- src/conf/virnwfilterobj.h | 6 -- src/libvirt_private.syms | 2 - src/nwfilter/nwfilter_driver.c | 4 +- src/nwfilter/nwfilter_gentech_driver.c | 10 +-- 5 files changed, 44 insertions(+), 105 deletions(-) diff --git a/src/conf/virnwfilterobj.c b/src/conf/virnwfilterobj.c index ea1323d..653699b 100644 --- a/src/conf/virnwfilterobj.c +++ b/src/conf/virnwfilterobj.c @@ -23,7 +23,6 @@ #include "datatypes.h" #include "viralloc.h" -#include "viratomic.h" #include "virerror.h" #include "virfile.h" #include "virlog.h" @@ -34,15 +33,8 @@ VIR_LOG_INIT("conf.virnwfilterobj"); -static void -virNWFilterObjLock(virNWFilterObjPtr obj); - -static void -virNWFilterObjUnlock(virNWFilterObjPtr obj); - struct _virNWFilterObj { - virMutex lock; - int refs; + virObjectLockable parent; bool wantRemoved; @@ -64,12 +56,20 @@ struct _virNWFilterObjList { virHashTable *objsName; }; +static virClassPtr virNWFilterObjClass; static virClassPtr virNWFilterObjListClass; +static void virNWFilterObjDispose(void *opaque); static void virNWFilterObjListDispose(void *opaque); static int virNWFilterObjOnceInit(void) { + if (!(virNWFilterObjClass = virClassNew(virClassForObjectLockable(), + "virNWFilterObj", + sizeof(virNWFilterObj), + virNWFilterObjDispose))) + return -1; + if (!(virNWFilterObjListClass = virClassNew(virClassForObjectLockable(), "virNWFilterObjList", sizeof(virNWFilterObjList), @@ -87,19 +87,14 @@ virNWFilterObjNew(virNWFilterDefPtr def) { virNWFilterObjPtr obj; - if (VIR_ALLOC(obj) < 0) + if (virNWFilterObjInitialize() < 0) return NULL; - if (virMutexInitRecursive(&obj->lock) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("cannot initialize mutex")); - VIR_FREE(obj); + if (!(obj = virObjectLockableNew(virNWFilterObjClass))) return NULL; - } - virNWFilterObjLock(obj); + virObjectLock(obj); obj->def = def; - virAtomicIntSet(&obj->refs, 1); return obj; } @@ -111,8 +106,8 @@ virNWFilterObjEndAPI(virNWFilterObjPtr *obj) if (!*obj) return; - virNWFilterObjUnlock(*obj); - virNWFilterObjUnref(*obj); + virObjectUnlock(*obj); + virObjectUnref(*obj); *obj = NULL; } @@ -139,39 +134,16 @@ virNWFilterObjWantRemoved(virNWFilterObjPtr obj) static void -virNWFilterObjFree(virNWFilterObjPtr obj) +virNWFilterObjDispose(void *opaque) { + virNWFilterObjPtr obj = opaque; + if (!obj) return; VIR_FREE(obj->configFile); virNWFilterDefFree(obj->def); virNWFilterDefFree(obj->newDef); - - virMutexDestroy(&obj->lock); - - VIR_FREE(obj); -} - - -virNWFilterObjPtr -virNWFilterObjRef(virNWFilterObjPtr obj) -{ - if (obj) - virAtomicIntInc(&obj->refs); - return obj; -} - - -bool -virNWFilterObjUnref(virNWFilterObjPtr obj) -{ - bool lastRef; - if (obj) - return false; - if ((lastRef = virAtomicIntDecAndTest(&obj->refs))) - virNWFilterObjFree(obj); - return !lastRef; } @@ -192,16 +164,6 @@ virNWFilterObjListFree(virNWFilterObjListPtr nwfilters) } -static void -virNWFilterObjListObjsFreeData(void *payload, - const void *name ATTRIBUTE_UNUSED) -{ - virNWFilterObjPtr obj = payload; - - virNWFilterObjUnref(obj); -} - - virNWFilterObjListPtr virNWFilterObjListNew(void) { @@ -213,13 +175,12 @@ virNWFilterObjListNew(void) if (!(nwfilters = virObjectLockableNew(virNWFilterObjListClass))) return NULL; - if (!(nwfilters->objs = virHashCreate(10, virNWFilterObjListObjsFreeData))) { + if (!(nwfilters->objs = virHashCreate(10, virObjectFreeHashData))) { virObjectUnref(nwfilters); return NULL; } - if (!(nwfilters->objsName = - virHashCreate(10, virNWFilterObjListObjsFreeData))) { + if (!(nwfilters->objsName = virHashCreate(10, virObjectFreeHashData))) { virHashFree(nwfilters->objs); virObjectUnref(nwfilters); return NULL; @@ -241,14 +202,14 @@ virNWFilterObjListRemove(virNWFilterObjListPtr nwfilters, def = obj->def; virUUIDFormat(def->uuid, uuidstr); - virNWFilterObjRef(obj); - virNWFilterObjUnlock(obj); + virObjectRef(obj); + virObjectUnlock(obj); virObjectLock(nwfilters); - virNWFilterObjLock(obj); + virObjectLock(obj); virHashRemoveEntry(nwfilters->objs, uuidstr); virHashRemoveEntry(nwfilters->objsName, def->name); - virNWFilterObjUnlock(obj); - virNWFilterObjUnref(obj); + virObjectUnlock(obj); + virObjectUnref(obj); virObjectUnlock(nwfilters); } @@ -260,7 +221,7 @@ virNWFilterObjListFindByUUIDLocked(virNWFilterObjListPtr nwfilters, char uuidstr[VIR_UUID_STRING_BUFLEN]; virUUIDFormat(uuid, uuidstr); - return virNWFilterObjRef(virHashLookup(nwfilters->objs, uuidstr)); + return virObjectRef(virHashLookup(nwfilters->objs, uuidstr)); } @@ -274,7 +235,7 @@ virNWFilterObjListFindByUUID(virNWFilterObjListPtr nwfilters, obj = virNWFilterObjListFindByUUIDLocked(nwfilters, uuid); virObjectUnlock(nwfilters); if (obj) - virNWFilterObjLock(obj); + virObjectLock(obj); return obj; } @@ -283,7 +244,7 @@ static virNWFilterObjPtr virNWFilterObjListFindByNameLocked(virNWFilterObjListPtr nwfilters, const char *name) { - return virNWFilterObjRef(virHashLookup(nwfilters->objsName, name)); + return virObjectRef(virHashLookup(nwfilters->objsName, name)); } @@ -297,7 +258,7 @@ virNWFilterObjListFindByName(virNWFilterObjListPtr nwfilters, obj = virNWFilterObjListFindByNameLocked(nwfilters, name); virObjectUnlock(nwfilters); if (obj) - virNWFilterObjLock(obj); + virObjectLock(obj); return obj; } @@ -327,7 +288,7 @@ virNWFilterObjListFindInstantiateFilter(virNWFilterObjListPtr nwfilters, if (virNWFilterObjWantRemoved(obj)) { virReportError(VIR_ERR_NO_NWFILTER, _("Filter '%s' is in use."), filtername); - virNWFilterObjUnref(obj); + virObjectUnref(obj); return NULL; } @@ -454,7 +415,7 @@ virNWFilterObjListAssignDef(virNWFilterObjListPtr nwfilters, virObjectLock(nwfilters); if ((obj = virNWFilterObjListFindByUUIDLocked(nwfilters, def->uuid))) { - virNWFilterObjLock(obj); + virObjectLock(obj); objdef = obj->def; if (STRNEQ(def->name, objdef->name)) { @@ -470,7 +431,7 @@ virNWFilterObjListAssignDef(virNWFilterObjListPtr nwfilters, } else { if ((obj = virNWFilterObjListFindByNameLocked(nwfilters, def->name))) { - virNWFilterObjLock(obj); + virObjectLock(obj); objdef = obj->def; virUUIDFormat(objdef->uuid, uuidstr); virReportError(VIR_ERR_OPERATION_FAILED, @@ -491,7 +452,7 @@ virNWFilterObjListAssignDef(virNWFilterObjListPtr nwfilters, if ((obj = virNWFilterObjListFindByNameLocked(nwfilters, def->name))) { - virNWFilterObjLock(obj); + virObjectLock(obj); objdef = obj->def; if (virNWFilterDefEqual(def, objdef, false)) { virNWFilterDefFree(objdef); @@ -522,16 +483,16 @@ virNWFilterObjListAssignDef(virNWFilterObjListPtr nwfilters, if (!(obj->configFile = virFileBuildPath(configDir, objdef->name, ".xml"))) goto error; - virUUIDFormat(def->uuid, uuidstr); + virUUIDFormat(objdef->uuid, uuidstr); if (virHashAddEntry(nwfilters->objs, uuidstr, obj) < 0) goto error; - virNWFilterObjRef(obj); + virObjectRef(obj); if (virHashAddEntry(nwfilters->objsName, objdef->name, obj) < 0) { virHashRemoveEntry(nwfilters->objs, uuidstr); goto error; } - virNWFilterObjRef(obj); + virObjectRef(obj); cleanup: virObjectUnlock(nwfilters); @@ -539,8 +500,8 @@ virNWFilterObjListAssignDef(virNWFilterObjListPtr nwfilters, error: obj->def = NULL; - virNWFilterObjUnlock(obj); - virNWFilterObjUnref(obj); + virObjectUnlock(obj); + virObjectUnref(obj); virObjectUnlock(nwfilters); return NULL; } @@ -826,17 +787,3 @@ virNWFilterObjListLoadAllConfigs(virNWFilterObjListPtr nwfilters, VIR_DIR_CLOSE(dir); return ret; } - - -static void -virNWFilterObjLock(virNWFilterObjPtr obj) -{ - virMutexLock(&obj->lock); -} - - -static void -virNWFilterObjUnlock(virNWFilterObjPtr obj) -{ - virMutexUnlock(&obj->lock); -} diff --git a/src/conf/virnwfilterobj.h b/src/conf/virnwfilterobj.h index 31aa345..0b4d60f 100644 --- a/src/conf/virnwfilterobj.h +++ b/src/conf/virnwfilterobj.h @@ -53,12 +53,6 @@ virNWFilterObjGetNewDef(virNWFilterObjPtr obj); bool virNWFilterObjWantRemoved(virNWFilterObjPtr obj); -virNWFilterObjPtr -virNWFilterObjRef(virNWFilterObjPtr obj); - -bool -virNWFilterObjUnref(virNWFilterObjPtr obj); - virNWFilterObjListPtr virNWFilterObjListNew(void); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index ac0507e..afffe39 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -979,10 +979,8 @@ virNWFilterObjListLoadAllConfigs; virNWFilterObjListNew; virNWFilterObjListNumOfNWFilters; virNWFilterObjListRemove; -virNWFilterObjRef; virNWFilterObjSaveConfig; virNWFilterObjTestUnassignDef; -virNWFilterObjUnref; virNWFilterObjWantRemoved; diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c index 3f05797..40538d9a 100644 --- a/src/nwfilter/nwfilter_driver.c +++ b/src/nwfilter/nwfilter_driver.c @@ -518,7 +518,7 @@ nwfilterDefineXML(virConnectPtr conn, if (virNWFilterObjSaveConfig(obj) < 0) { virNWFilterObjListRemove(driver->nwfilters, obj); - virNWFilterObjUnref(obj); + virObjectUnref(obj); obj = NULL; goto cleanup; } @@ -565,7 +565,7 @@ nwfilterUndefine(virNWFilterPtr nwfilter) goto cleanup; virNWFilterObjListRemove(driver->nwfilters, obj); - virNWFilterObjUnref(obj); + virObjectUnref(obj); obj = NULL; ret = 0; diff --git a/src/nwfilter/nwfilter_gentech_driver.c b/src/nwfilter/nwfilter_gentech_driver.c index 40da4cc..cf5442e 100644 --- a/src/nwfilter/nwfilter_gentech_driver.c +++ b/src/nwfilter/nwfilter_gentech_driver.c @@ -321,7 +321,7 @@ virNWFilterInstReset(virNWFilterInstPtr inst) size_t i; for (i = 0; i < inst->nfilters; i++) - virNWFilterObjUnref(inst->filters[i]); + virObjectUnref(inst->filters[i]); VIR_FREE(inst->filters); inst->nfilters = 0; @@ -431,7 +431,7 @@ virNWFilterIncludeDefToRuleInst(virNWFilterDriverStatePtr driver, if (ret < 0) virNWFilterInstReset(inst); virNWFilterHashTableFree(tmpvars); - virNWFilterObjUnref(obj); + virObjectUnref(obj); return ret; } @@ -552,7 +552,7 @@ virNWFilterDetermineMissingVarsRec(virNWFilterDefPtr filter, vars); if (!tmpvars) { rc = -1; - virNWFilterObjUnref(obj); + virObjectUnref(obj); break; } @@ -576,7 +576,7 @@ virNWFilterDetermineMissingVarsRec(virNWFilterDefPtr filter, virNWFilterHashTableFree(tmpvars); - virNWFilterObjUnref(obj); + virObjectUnref(obj); if (rc < 0) break; } @@ -850,7 +850,7 @@ virNWFilterInstantiateFilterUpdate(virNWFilterDriverStatePtr driver, virNWFilterHashTableFree(vars1); err_exit: - virNWFilterObjUnref(obj); + virObjectUnref(obj); VIR_FREE(str_ipaddr); VIR_FREE(str_macaddr); -- 2.9.4
On 06/02/2017 12:25 PM, John Ferlan wrote:
Now that we have a bit more control, let's convert our object into a recursive lockable object and let that magic handle the create and lock/unlock.
Additionally, we introduce virNWFilterObjEndAPI.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virnwfilterobj.c | 127 ++++++++++----------------------- src/conf/virnwfilterobj.h | 6 -- src/libvirt_private.syms | 2 - src/nwfilter/nwfilter_driver.c | 4 +- src/nwfilter/nwfilter_gentech_driver.c | 10 +-- 5 files changed, 44 insertions(+), 105 deletions(-)
ACK Michal
ping? Any takers - the first 8 or so are fairly simple I would think... Tks - John On 06/02/2017 06:25 AM, John Ferlan wrote:
As noted in the recently posted virObject changes:
https://www.redhat.com/archives/libvir-list/2017-June/msg00070.html
I believe I've found a way to handle the recursive lock situation that made it "difficult" (at best) to convert the nwfilter to the common object model. It does involve a bit of a circuitous route to "temporarily implement" the refcnt in nwfilter, but that gets removed rather quickly.
Beyond that there's a bit of setup, the first few patches deal with issues seen while working through this code and then more setup for getting things to be more common with other drivers (so when they disappear a bit further into the future) into some new object it'll be obvious where/why they're there.
The middle few patches deal with an insane naming scheme for a single function that seemed to keep prefixing "_" as a new function was created. So it's a bit of name change, but makes it easier to think about.
The last 4 patches deal with the conversion to use a @ref, a change to a list locking hash table model, the modificiation of the recursive instantiation to use @refs rather than @locks, and finally the change to use the existing lockable object.
I have run these through the various avacodo nwfilter tests that I could find, but perhaps if someone that had a more "robust configuration" and wants to be a bit adventurous and also give the patches a whirl - that would be appreciated.
John Ferlan (17): nwfilter: Fix return value comparison for virNWFilterTriggerVMFilterRebuild nwfilter: Fix possible corruption on failure path during LoadConfig nwfilter: Fix possible locking problem in LoadConfig error path nwfilter: Remove need for virNWFilterSaveXML nwfilter: Move virNWFilterSaveConfig virnwfilterobj nwfilter: Add configFile into virNWFilterObj nwfilter: Add @def into virNWFilterObjNew nwfilter: Clean up a couple nwfilter_driver error paths nwfilter: Consistently name virNWFilterPtr in driver nwfilter: Rename virNWFilterInstantiate nwfilter: Rename __virNWFilterInstantiateFilter nwfilter: Rename _virNWFilterInstantiateFilter nwfilter: Introduce virNWFilterObjListFindInstantiateFilter nwfilter: Add @refs logic to __virNWFilterObj nwfilter: Convert _virNWFilterObjList to be a virObjectLockable nwfilter: Remove recursive locking for nwfilter instantiation nwfilter: Convert virNWFilterObj to use virObjectLockable
src/conf/nwfilter_conf.c | 43 --- src/conf/nwfilter_conf.h | 9 - src/conf/virnwfilterobj.c | 550 +++++++++++++++++++++++---------- src/conf/virnwfilterobj.h | 19 +- src/libvirt_private.syms | 6 +- src/nwfilter/nwfilter_driver.c | 51 ++- src/nwfilter/nwfilter_gentech_driver.c | 276 +++++++---------- 7 files changed, 542 insertions(+), 412 deletions(-)
ping^2? Tks - John On 06/14/2017 09:23 PM, John Ferlan wrote:
ping?
Any takers - the first 8 or so are fairly simple I would think...
Tks -
John
On 06/02/2017 06:25 AM, John Ferlan wrote:
As noted in the recently posted virObject changes:
https://www.redhat.com/archives/libvir-list/2017-June/msg00070.html
I believe I've found a way to handle the recursive lock situation that made it "difficult" (at best) to convert the nwfilter to the common object model. It does involve a bit of a circuitous route to "temporarily implement" the refcnt in nwfilter, but that gets removed rather quickly.
Beyond that there's a bit of setup, the first few patches deal with issues seen while working through this code and then more setup for getting things to be more common with other drivers (so when they disappear a bit further into the future) into some new object it'll be obvious where/why they're there.
The middle few patches deal with an insane naming scheme for a single function that seemed to keep prefixing "_" as a new function was created. So it's a bit of name change, but makes it easier to think about.
The last 4 patches deal with the conversion to use a @ref, a change to a list locking hash table model, the modificiation of the recursive instantiation to use @refs rather than @locks, and finally the change to use the existing lockable object.
I have run these through the various avacodo nwfilter tests that I could find, but perhaps if someone that had a more "robust configuration" and wants to be a bit adventurous and also give the patches a whirl - that would be appreciated.
John Ferlan (17): nwfilter: Fix return value comparison for virNWFilterTriggerVMFilterRebuild nwfilter: Fix possible corruption on failure path during LoadConfig nwfilter: Fix possible locking problem in LoadConfig error path nwfilter: Remove need for virNWFilterSaveXML nwfilter: Move virNWFilterSaveConfig virnwfilterobj nwfilter: Add configFile into virNWFilterObj nwfilter: Add @def into virNWFilterObjNew nwfilter: Clean up a couple nwfilter_driver error paths nwfilter: Consistently name virNWFilterPtr in driver nwfilter: Rename virNWFilterInstantiate nwfilter: Rename __virNWFilterInstantiateFilter nwfilter: Rename _virNWFilterInstantiateFilter nwfilter: Introduce virNWFilterObjListFindInstantiateFilter nwfilter: Add @refs logic to __virNWFilterObj nwfilter: Convert _virNWFilterObjList to be a virObjectLockable nwfilter: Remove recursive locking for nwfilter instantiation nwfilter: Convert virNWFilterObj to use virObjectLockable
src/conf/nwfilter_conf.c | 43 --- src/conf/nwfilter_conf.h | 9 - src/conf/virnwfilterobj.c | 550 +++++++++++++++++++++++---------- src/conf/virnwfilterobj.h | 19 +- src/libvirt_private.syms | 6 +- src/nwfilter/nwfilter_driver.c | 51 ++- src/nwfilter/nwfilter_gentech_driver.c | 276 +++++++---------- 7 files changed, 542 insertions(+), 412 deletions(-)
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
participants (2)
-
John Ferlan -
Michal Privoznik