Re: [libvirt] [PATCH 02/15] nwfilter: Use virNWFilterDefPtr rather than deref virNWFilterObjPtr

On 04/25/2017 09:18 AM, Pavel Hrdina wrote: > On Mon, Apr 24, 2017 at 03:18:31PM -0400, John Ferlan wrote: >> Rather than dereferencing obj->def->XXX or nwfilters->objs[i]->X >> create local virNWFilterObjPtr and virNWFilterDefPtr variables. >> >> Future adjustments will be privatizing the object more, so this just >> prepares the code for that reality. >> >> Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; >> --- >> src/conf/virnwfilterobj.c | 80 +++++++++++++++++++++++++++--------------- >> src/nwfilter/nwfilter_driver.c | 33 ++++++++++------- >> 2 files changed, 72 insertions(+), 41 deletions(-) >> >> diff --git a/src/conf/virnwfilterobj.c b/src/conf/virnwfilterobj.c >> index cb697f9..3c6bdbb 100644 >> --- a/src/conf/virnwfilterobj.c >> +++ b/src/conf/virnwfilterobj.c >> @@ -37,11 +37,16 @@ VIR_LOG_INIT("conf.virnwfilterobj"); >> void >> virNWFilterObjFree(virNWFilterObjPtr obj) >> { >> + virNWFilterDefPtr def; >> + virNWFilterDefPtr newDef; >> + >> if (!obj) >> return; >> + def = obj->def; >> + newDef = obj->newDef; >> >> - virNWFilterDefFree(obj->def); >> - virNWFilterDefFree(obj->newDef); >> + virNWFilterDefFree(def); >> + virNWFilterDefFree(newDef); > > This was discussed in the secret cleanup series. In this case it just adds > some lines to the code without any real benefit, so it's just a noise in this > case. This change makes sense for functions where the *def* is used several > times, but for those simple usages of def there is no point of having a > separate variable. > > Now I know that some future patches may benefit from this change, however > there is no guarantee that the patches will be accepted and pushed I think > that you should wait with these changes for that future series. > > Pavel > I wouldn't be the first and I won't be the last to make/post changes to code to help some future yet unposted series that has the some 'issue' w/r/t a "guarantee" that it would accepted/pushed. Like I pointed out in the secrets series - stopping progress here because it's undesirable to have @def alone doesn't mean it's not technically correct and the reality is the compiler will do whatever it wants... Again, like the secrets code I can agree if it's just "obj->def", I can restore that, but once it's "obj->def->X", then having a "def" is more desirable regardless if there's only one such reference in the code.

Pulling this patch from the entire series begins to impact (e.g. force me down the git merge path) starting at patch 5. So it's very painful to do. John -- libvir-list mailing list libvir-list(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list