Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Maintenance swipe: get rid of some global variables in favour of actual data flow visibility #82

Open
wants to merge 28 commits into
base: main
Choose a base branch
from

Conversation

jnpkrn
Copy link

@jnpkrn jnpkrn commented Jan 30, 2020

This is a first step in a direction towards less rigid/cumbersome
code, easing the understandability and maintenance as such.

Get rid of (some, commonly shared) global variables means:

  • less hidden, behind the curtains state, meaning less
    of surprising side-effects, better understandable data
    flow, effectively containing the impact radius of the
    functions

  • unit tests are more feasible (easier to conduct) when the
    inputs to the computation/functions are not part of said
    non-apparent global state, but rather part of the explicit
    input to the them

  • likewise, any "scope of authority" etc. types of regrouping
    of the functions in modules is not easily done when the
    data flow is partially obfuscated

  • more dynamic handling is enabled (e.g. multithreading or
    fetching a new instance of the configuration, which can then
    be conditionally promoted to the life configuration)
    if there's a good need for that

  • last but not least, using global-scoped variables is not
    considered a praised practice

For struct booth_config in particular, it was dynamically
alloc'd at heap, so there was no gain in terms of avoiding
relevant class of bugs.

Also, this PR reconciles the no-code-doc problem at the functions
being touched throughout. And this swipe helped to identify
both unused and needlessly exported functions.

This is a primitive that's easier to follow/more terse.
Looking forward, we skip some places where we want to introduce
a refactoring to drop an undesired non-transparent reliance on
booth_conf global variable prior to modifying these two macros
to that effect as well (respective places, at least, receive
white-space-around-operators sanitization).

Signed-off-by: Jan Pokorný <[email protected]>
This is to allow for better visual identification of the macro magic.

Signed-off-by: Jan Pokorný <[email protected]>
@jnpkrn
Copy link
Author

jnpkrn commented Jan 30, 2020

And not to forget ... current passing of the whole
struct booth_config is an apparent overapproximation,
where just some bits of that structure are needed inputs
for some particular functions to operate upon.

I haven't delved into that business yet, but it's one
of the most immediate steps to follow. Note that without
this rather radical intervention, such a fine-graining
would hardly be possible.

@jnpkrn
Copy link
Author

jnpkrn commented Jan 30, 2020

Wrt. fine-graining, desired access method (read-only/ok-to-modify) for
subset of that struct can now finally be expressed/guarded -- something
you can't viably achieve with an arbitrary-touch-for-everything global.

@jnpkrn
Copy link
Author

jnpkrn commented Jan 30, 2020

So it imposes structural (vs. hazy flat) patterns on multiple levels,
which is desirable.

Also, apply the same recursively to both the underlying and superjacent
call graph expansions (respective static functions).

Signed-off-by: Jan Pokorný <[email protected]>
Also, apply the same recursively to both the underlying and superjacent
call graph expansion (respective static functions for the former,
progression up to main.c, i.e. do_attr_command, for the latter).

Also, exercise some const-correctness at places being touched,
and mark unused function as such.

Signed-off-by: Jan Pokorný <[email protected]>
Also, apply the same recursively to both the underlying and superjacent
call graph expansion (respective static functions for the former,
progression up to main.c, i.e. setup_ticket, for the latter).

Signed-off-by: Jan Pokorný <[email protected]>
@jnpkrn
Copy link
Author

jnpkrn commented Jan 31, 2020

Some previous commits amended, some new added.

Known issue:

boothd: transport.c:153: _find_myself: Assertion `conf_ptr->local != NULL' failed.

This is because of intentionally crafted asserts to discover exactly
this sort of naively expected guarantees.

Need to look into the call chain whether it's not malign, to begin with.

Also, apply the same recursively to both the underlying and superjacent
call graph expansion (respective static functions[*] for the former,
progression up to main.c).

[*] _find_myself gets marked static as it should have been as of
    56801bd

Signed-off-by: Jan Pokorný <[email protected]>
There's no real need since it's already carried in the "local" var[*]
carrying site data for what's resolved as a local site.  Since it's
not a straightforward expression to pick up the port stored inside
SITE->[union: either sa4 or sa6 member], another inline getter moreover
not crashing on SITE being NULL was devised: site_port.

That helper is consequently applied also at other places that deal with
fetching port number -- antithetically, they used to fetch from said
booth_conf global variable while, at the same time, other connection
relevant information used to be fetched from whole another memory
object (said "local site" global).  In effect, even more undesired
dependencies on booth_conf global were removed, in favour of unifying
the source of data at said "local site" one.

Also, insist on const-correctness with the sibling of added inline
function (so they look uniformly, the new one also follows that rule).

[*] also global but that's not of interest for the time being

Signed-off-by: Jan Pokorný <[email protected]>
Also, apply the same recursively to superjacent call graph expansion
(progression up to main.c), and apply that whenever it is not eligible
where it previously was not (because FOREACH_{NODE,TICKET} not accepting
struct booth_config * injection.

Also, exercise some const-correctness at places being touched,
and hide some functions as merely static ones.

Signed-off-by: Jan Pokorný <[email protected]>
Also, apply the same recursively to superjacent call graph expansion.

Also, exercise some const-correctness at places being touched,
and hide some functions as merely static ones.

Signed-off-by: Jan Pokorný <[email protected]>
Mark function being effectively static (and move around the file so as
not to require an extra declaration) to that effect.

Signed-off-by: Jan Pokorný <[email protected]>
Consequently, foreach_tkt_req (what said function is ultimately passed
into) that practically implements high order functional do-for-all
pattern with the same subset of arguments that itself receives, needs
to be extended with this extra argument as well.

Also, apply the same recursively to superjacent call graph expansion
(progression up to main.c).

Also mark function being effectively static.

Signed-off-by: Jan Pokorný <[email protected]>
The only exception, transport(), is kept since it is intertwined with
another global, hence dealing with it is postponed for now.

Signed-off-by: Jan Pokorný <[email protected]>
And except for said single use intermingled with another global
mentioned in the previous commit: transport() of inline-fn.h.

Signed-off-by: Jan Pokorný <[email protected]>
Even then, booth_conf global variable cannot be turned into static one,
since there is this multi-global complication that was mentioned
already: transport() of inline-fn.h.

Signed-off-by: Jan Pokorný <[email protected]>
Finish ~ finally mark static.

As for booth_transport, it is still more covenient to split definition
and actual use across two files (transport.c and main.c, respectively),
so we at least make the semi-hidden nature (no presence in any of the
actual header files) apparent with the double-underscore after initial
namespace prefix convenience.

Also fix a latent "no extern storage class specifier specified" in case
of compiling with -fno-common/default since GCC 10 (for missing
"extern" qualifier).

Signed-off-by: Jan Pokorný <[email protected]>
It is still more covenient to split definition and actual use across two
files (pacemaker.c and main.c, respectively), so we at least make the
semi-hidden nature (no presence in any of the actual header files)
apparent with the double-underscore after initial namespace prefix
convenience.

Also fix a latent "no extern storage class specifier specified" in case
of compiling with -fno-common/default since GCC 10 (for missing "extern"
qualifier).

Signed-off-by: Jan Pokorný <[email protected]>
At that occasion, make "struct booth_config" contain "path_to_self"
as it is handy to have it there and not to rely on presence of the
former struct.

Signed-off-by: Jan Pokorný <[email protected]>
Thanks to the change with the former, attaching it to the now de facto
generalized "context" in struct booth_config being passed around, we
can now simplify find_myself (both underscore-prefixed and plain) so
that no extra outbound argument propagation is needed (argument is
dropped).

Signed-off-by: Jan Pokorný <[email protected]>
This module is supposed to hold generic functionality satisfying
no-binding-but-libc.

Signed-off-by: Jan Pokorný <[email protected]>
Also, type the argument properly.

Signed-off-by: Jan Pokorný <[email protected]>
@dmuhamedagic
Copy link

It is indeed less than optimal for the booth_config to be shared as is. Thanks for bringing this up.
It would, however, be great if could have a better separation of concerns. I guess that we would automatically get a more localized data access. For instance, the consensus algorithm is run on a per ticket basis, which is most probably an overkill and it should be run on a site basis. That's the most glaring issue, but there could be more.
Splitting booth_config is very much worth considering, as you already mentioned.

@dmuhamedagic
Copy link

Hi Jan, apparently you are not involved any more with HA. Wanted to say that it is a pity that we don't have your help anymore, but I guess that it was not easy fighting against proverbial wind mills. It is my fault that I often did not have time to work on your pull request, though heaven knows that I really tried. Occasionally, it would be just too much for me. But that doesn't mean that your PRs were bad, to the contrary, they were always of excellent quality and your engagement was always very much appreciated.

@dmuhamedagic
Copy link

Having said all that, I hope that you are doing fine, wherever you work now. Cheers, Dejan (-:

@clumens
Copy link

clumens commented Jul 22, 2024

Don't mind me, just making a checklist so I know which patches I've merged and which I still have left to do.

  • no fear from a word interfere
  • foreach_{node,ticket}: apply more universally
  • rename foreach_{node,ticket} macros to uppercased variants
  • read_config to no longer rely on global booth_conf variable
  • check_config to no longer rely on global booth_conf variable
  • find_site_by_name to no longer rely on global booth_conf var
  • find_site_by_id to no longer rely on global booth_conf var
  • find_myself to no longer rely on global booth_conf variable
  • setup_udp_server to no longer rely on global booth_conf var
  • FOREACH_{NODE,TICKET} to no longer rely on global booth_conf
  • find_ticket_by_name to no longer rely on global booth_conf var
  • rest of ticket.c to no longer rely on global booth_conf var
  • notify_client to no longer rely on global booth_conf var
  • rest of transport.c to no longer rely on global booth_conf var
  • rest of raft.c to no longer rely on global booth_conf var
  • rest of inline-fn.h to no longer rely on global booth_conf var
  • get rid of any use of booth_conf global var except for main.c
  • get rid of any sparable use of booth_conf global var in main.c
  • get rid of booth_transport as a global var&finish booth_config
  • get rid of pcmk_handler as a global variable
  • get rid of "struct command_line" as a global variable
  • get rid of poll_timeout global variable
  • get rid of loval and no_leader and global variables
  • some more cleanups (unused or shall-be-static functions)
  • factor safe_copy function to an auxiliary utils module
  • move check_max_len_valid function to existing utils module
  • move find_ticket_by_name to config module, for a better git
  • systemic disposal of struct booth config

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants