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.

Sign up for GitHub

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

Jump to bottom

Fix a segfault + related refactoring #50

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

Fix a segfault + related refactoring #50

wants to merge 4 commits into from
+41 −68

Conversation

jnpkrn
Copy link

@jnpkrn jnpkrn commented Sep 13, 2016

No description provided.

@jnpkrn
Low: make find_site_by_name failure set error code
5b8a336 
At one instance (query_get_string_answer) it just flips the sign as it
is customary to return negative value upon error (for uniform
treatment).
@jnpkrn
High: ensure local site resolved for all effective actions
bcf1117 
Previously, running:

  touch /etc/booth/booth.conf
  booth grant a_ticket

would result in a segfault due to not guarding resolution of local
site properly in some circumstances, so do it at the central place.
Also error messaging is now centralized.
@jnpkrn
Low: make daemon with "-s site" (debug mode) claim "myself"
1185487
@jnpkrn
Copy link
Author

jnpkrn commented Sep 13, 2016 •
edited
Loading

Note that Travis CI runs are currently borked.
There must have been a change on their side that is causing this, I can't even successfully finish what was once passing:

May they have mangled with 127.0.0.1 address resolution?

@jnpkrn
Copy link
Author

jnpkrn commented Sep 14, 2016

There was an issue with last commit in not resolving local even if it is relied upon later on (hence introducing similar issue this PR aims to solve in the first place). That should be fixed now.

@jnpkrn
Copy link
Author

jnpkrn commented Sep 14, 2016

Well, haven't read correctly, it was actually 127.0.1.1 and the proposed workaround for the indeed changed Travis CI image is #51. You may pull that first and then rerun tests for this PR.

@jnpkrn jnpkrn force-pushed the fix-segfault branch 4 times, most recently from 29bf5a9 to 736f58d Compare September 15, 2016 17:19
@jnpkrn
Refactor: call find_site_by_name just once, up the stream
0c94ea1 
Respective logic was duplicated for all "booth list/peers/grant/revoke"
and "geostore list/get/set/del" separately, so utilize a natural control
flow to carry this once-resolved target site from here, sharing it with
the special case of "daemon" role invoked with "-s site" (debug mode).
Side effect: simpler, terser code.
@dmuhamedagic
Copy link

On Tue, Sep 13, 2016 at 12:58:56PM -0700, Jan Pokorný wrote:

  • Refactor: call find_site_by_name just once, up the stream

I did consider this before, but decided against refactoring. The
new code which ends in setup_config() is somewhat unwieldy and
difficult to grasp. Anyway, if you're bothered by this, a macro
of a "if (!find_site_by_name(...)) { ... }" would probably be a
better option.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jnpkrn @dmuhamedagic