Re: [libvirt] [PATCH 03/17] nwfilter: Fix possible locking problem in LoadConfig error path

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(a)redhat.com&gt; > --- > 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