util: warn about ignored recursive -includeconf calls #13197

pull kallewoof wants to merge 2 commits into bitcoin:master from kallewoof:includeconf-warn-nonrecursive changing 2 files +20 −2

kallewoof commented at 7:53 AM on May 9, 2018: member

This is a follow-up PR to #10267, and addresses #10267 (comment).

~~I am adding extra work for @jnewbery in #12755 here -- maybe I should just rebase on top of that, but not sure what the appropriate approach is here.~~
fanquake added the label Utils/log/libs on May 9, 2018
laanwj commented at 8:48 AM on May 9, 2018: member

I am adding extra work for @jnewbery in #12755 here -- maybe I should just rebase on top of that, but not sure what the appropriate approach is here.

I (personally) prefer if PRs are more or less independent. As having a web of dependencies between them makes it even harder to keep track of them. On the other hand, #10267 is now in, and #12755 should probably be next.
kallewoof commented at 8:55 AM on May 9, 2018: member

@laanwj Makes sense. I will leave this as is and adjust it once #12755 is merged.
promag commented at 9:28 AM on May 9, 2018: member

utACK 6f10309.

BTW couldn't cs_args be locked the whole time?
jnewbery commented at 2:25 PM on May 9, 2018: member

concept ACK (and quick skim utACK). Will review & test after #12755 is merged.
laanwj commented at 2:58 PM on May 9, 2018: member

#12755 is merged, please rebase

util: warn about recursive -includeconf arguments in configuration files

Since -includeconf cannot be used recursively, the user would not see feedback that an -includeconf
in an -includeconf'd file was silently ignored.

c5bcc7dbe9

test: Ensure that recursive -includeconf produces appropriate warnings 2352aa9f36
kallewoof force-pushed on May 10, 2018
kallewoof commented at 2:16 AM on May 10, 2018: member

Rebased.
kallewoof commented at 2:24 AM on May 10, 2018: member

@promag Is it better locking the whole time?
laanwj commented at 7:26 AM on May 10, 2018: member

@promag Is it better locking the whole time?

I don't know what the potential races here are, but If everything else is the same, keeping the lock closely around the section where it is needed is preferable. Also: calls such as GetArgs take their own lock, so already holding it is sloppy.
promag commented at 10:27 AM on May 10, 2018: member

@laanwj I mean, ReadConfigFiles should be an atomic operation right?
laanwj commented at 10:45 AM on May 11, 2018: member

ReadConfigFiles should be an atomic operation right?

As far as I know we don't really make any assumptions in that regard right now, but in sane API design I think that makes sense.
kallewoof commented at 10:56 AM on May 11, 2018: member

I can try moving the lock to the top of the method and remove it elsewhere. I am concerned it may cause deadlocks though.
kallewoof commented at 10:57 AM on May 11, 2018: member

ba5b9fc
promag commented at 11:01 AM on May 11, 2018: member

@kallewoof I think it's great to change that but in a different PR. Commit 2352aa9f is enough here. Sorry for asking here!
kallewoof force-pushed on May 11, 2018
kallewoof commented at 12:20 PM on May 11, 2018: member

Yeah doesn't seem as simple as I hoped. Dropping ba5b9fc.
laanwj commented at 2:34 PM on May 14, 2018: member

utACK 2352aa9f3681fbcfb89a1910d9c5529dac9ca2e9
laanwj merged this on May 14, 2018
laanwj closed this on May 14, 2018
laanwj referenced this in commit 682698970d on May 14, 2018
jasonbcox referenced this in commit 6dd7d19103 on Sep 27, 2019
kallewoof deleted the branch on Oct 17, 2019
UdjinM6 referenced this in commit 207defb4c0 on Jun 18, 2021
UdjinM6 referenced this in commit 5cf96e8fdf on Jun 24, 2021
UdjinM6 referenced this in commit 62abb11c33 on Jun 26, 2021
UdjinM6 referenced this in commit c9edd3e8ea on Jun 26, 2021
UdjinM6 referenced this in commit ec9f526cec on Jun 28, 2021
DrahtBot locked this on Dec 16, 2021

Contributors

Labels

Linked (view graph)

#10267 New -includeconf argument for including external configuration files #12755 [tests] Better stderr testing