[Swan-dev] [Swan-commit] modularity erosion

[Andrew Cagney]

On Wed, 13 Feb 2019 at 04:41, Antony Antony <antony at phenome.org> wrote:
>
> On Tue, Feb 12, 2019 at 07:21:46PM -0500, Andrew Cagney wrote:
> > On Tue, 12 Feb 2019 at 11:25, D. Hugh Redelmeier <hugh at mimosa.com> wrote:
> > >
> > > commit 6909918af77cb8cc39bdad12c51543e16f8297a9
> > > Author: Paul Wouters <pwouters at redhat.com>
> > > Date:   Mon Feb 11 19:26:40 2019 -0500
> > >
> > >     pluto: removal all but one include of proposals.h
> > >
> > >     Since connections.h needs it, and that is included everywhere else,
> > >     there is no need for separate includes.
> > >
> > > If this becomes a trend, this is terrible.
> >
> > I suspect a lot weren't needed before this change - #includes seem to breed,
> >
> > > In this particular case, it seems forced:
> > >
> > > struct connection has:
> > >
> > >         struct ike_proposals ike_proposals;
> > >         struct child_proposals child_proposals;
> >
> > The fields are shared between IKEv1 and IKEv2.
> >
> > The old code had something like:
> >     struct {ike,esp}_alg_info {
> >       struct alg_info ai;
> >     }
> > (yes esp, was used by ah)
> >
> > which pretty much forced everything to include the header anyway so
> > that they could pass &ai.
> >
> > Anyway, a cleanup I've somewhere in my queue is to move the definition
> > of struct {ike,child}_proposal to connection.h, they're not used by
> > the proposal code proper.
>
> I am curious about what Hugh brought up here. I would love to learn more the
> issue and solutions. I think there are three methods of fixing such issues
> in our code.

> 1st approach, I think what Hugh is proposing, is sprinkle a bunch
> declarations in .c or .h. In our code sometimes they are clearly
> commented with /* forward declaration */

> 2nd approach is move around definitions of struct. I imagine this is
> Andrew's choice. Atleast in this case?

This isn't true.  The code evolution we're discussing here is truly
unique.  These are the declarations in question lifted, verbatim, from
"proposals.h":

/*
 * XXX: useful?
 */
struct ike_proposals {
        struct proposals *p;
};
struct child_proposals {
        struct proposals *p;
};

A quick examination of the rewritten proposal code, including
algparse, shows nothing is making use of these declarations.  Instead
'struct proposals' is opaque (per #1) and the header wrapped in #ifdef
(per #3).  FWIW, much of the new code follows this style (the
exception being some primitive types like monotime_t et.al.).

However, pluto does still use the declarations - they provide a way to
strongly differentiate between an IKE and CHILD proposal.  But that is
something local to pluto and connections.h, hence my note above
pointing out that it should be moved as a *cleanup*.

But "XXX: useful?"?  Well, when rewriting the parser I did consider
dumping those two declarations and instead just using 'struct
proposals *' in "connections.h".  I decided against it.  It seemed to
me that the stronger type helped with readability vis:
   ikev1_quick_pfs(const struct child_proposals proposals)


> 3rd Me sometimes and Paul, guess often, use #include foo.h
> includes every where:) No declarations for struct.
> That means to avoid "error: redefinition" add
>  #ifndef _FOO_, #define _FOO_ at the top of include file.
> I have also used forward declarations, however after long time I am not so
> sure if that is best for Libreswan.

> I am wondering what are the pros and cons of the three approaches
> now a days form Libreswan perspective. Lets focus on Libreswan perspective,
> something of this size complexity and not kernel... or apache.. or perl...
>
> Any pointers to read up would be great.
>
> What I gathered so far:
> I often wonder why 1st approach is better.
> I notice side a few effects of the first method.
> We don't clean up unused declarations. They seems to linger around when code
> move around or that specific declaration is not necessary anymore in that
> particular location. Sometimes we include ".h" anyway to get definition and
> the declaration lingers.
> The compiler do not seems to complain about duplicate declarations.
> Or linker do not complain about declarations without matching definitions
> else where after linking.
> Also when things get renamed, declarations would linger around.
> Is there an easy way to find unused local declarations? more tools?
>
> Googling suggest using forward declarations compared to include file would
> reduce compile time. Then I am curious to see numbers for a project like
> Libreswan. I am wondering is it a big issue in Libreswan in 2019?
> Another argument in favor forward declaration is name space collision.
> Again wondering is that a real issue for Libreswan. May be in some cases.
>
> Second option of moving around definitions is not so great in my opinion:)
> from what I notice there is a lot of code churn and definitions. Soon that
> tend to appear like magic coming from under the snow:) You add one line of
> code then you have more around definitions instead possibly #include new file
> or two.
>
> If it is all matter of taste, setting a best practice would help. Hopeful
> not second method:)
>
> -antony
>
> PS: a side rant name space collisions. I can understand name space
> separation between lib and pluto. However, if we start using function names
> or sturct names several times in pluto and isolate the name space across
> files it become hard to read code and debug using gdb, remember how we used
> get confused with log functions! or there would be 5 libreswanlog()
> functions, one would wonder when to use which, aslo debugging where to put
> break points. It is good to see log functions getting better!