On Tue, Apr 25, 2017 at 11:42:37AM -0400, John Ferlan wrote:
>
>
> 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@xxxxxxxxxx>
> >> ---
> >> 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.
That sounds reasonable, I can agree on that. I don't want to stop
any progress, I just didn't like the first case that will be dropped.
Pavel
> 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@xxxxxxxxxx
> https://www.redhat.com/mailman/listinfo/libvir-list
Attachment:
signature.asc
Description: Digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
