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

[Pavel Hrdina]

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 at redhat.com>
> >> ---
> >>  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 at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20170425/c2474bb5/attachment-0001.sig>