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@xxxxxxxxxx>
>>>> ---
>>>> 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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
