Client: restore default Event Log settings

[RichardHaselgrove]

Fixes #3606

Description of the Change
cc_config.cpp: following discussion in https://boinc.berkeley.edu/forum_thread.php?id=13588, this seems a more normal place to put the 'static' declaration.
cc_config.h: empirical, Can't set defaults in init(), so put them here instead.

Tested on
Windows (VS2013)
Linux Mint, based on Ubuntu (configure/make)

[RichardHaselgrove]

@CharlieFenton - please check whether the related issue #3606 applies on Mac as well - I don't have the hardware to test either problem or solution.

[RichardHaselgrove]

@ALL - I think this is important enough to cherrypick to the v7.16 release branch once approved. Please also cherrypick #3607, which is related.

[codecov]

Codecov Report

Merging #3616 into master will decrease coverage by 0.02%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #3616      +/-   ##
==========================================
- Coverage   18.11%   18.09%   -0.03%     
==========================================
  Files         107      107              
  Lines        8787     8803      +16     
  Branches     1544     1542       -2     
==========================================
+ Hits         1592     1593       +1     
- Misses       7079     7094      +15     
  Partials      116      116              

Impacted Files	Coverage Δ
...c/main/java/edu/berkeley/boinc/client/Monitor.java	0.00% <0.00%> (ø)
...ava/edu/berkeley/boinc/ProjectDetailsFragment.java	0.00% <0.00%> (ø)
...ava/edu/berkeley/boinc/attach/AcctMgrFragment.java	0.00% <0.00%> (ø)
...va/edu/berkeley/boinc/rpc/AcctMgrRPCReplyParser.kt	88.46% <0.00%> (+3.84%)	⬆️

[CharlieFenton]

Yes, this problem is also present on the Mac.

This is not an area with which I am most familiar, but I did a quick compare of cc_config.cpp between the 7.14 branch and master, and I think you missed the actual cause of the problem.

In both 7.14 and master, LOG_FLAGS:init() sets these three variables:
task = true; file_xfer = true; sched_ops = true;

But (although I haven't tested it), I suspect that LOG_FLAGS:init() is never called in master as a result of the changes, because:
[1] 7.14 has the following constructor, while master has no constructor at all:
LOG_FLAGS::LOG_FLAGS() { init(); }

[2] In 7.14, the first 3 lines of LOG_FLAGS::parse() are:
int LOG_FLAGS::parse(XML_PARSER& xp) { init(); while (!xp.get_tag()) {
In master, the init() call has been removed.

Quite frankly, I've never really understood nor cared for the way these initializers were implemented after being changed from the original memset() calls. This is not the first significant problem these changes have caused. And I always felt that the use of memset() was perfectly acceptable and should have been left alone. But that is just my opinion.

[CharlieFenton]

I think these proposed changes are unnecessary and that they won't fix this problem. Please see my comment here

[CharlieFenton]

Well, my face is a bit red. I incorrectly read the file compare, but I think my analysis still applies. LOG_FLAGS::parse() does have the init() call in master; it is 7.14 that does not.

Nonetheless, I think having it in the constructor is still necessary. log_flags.init() is also called in read_config_file() in log_flags.cpp. But If there is no cc_config.xml file, will LOG_FLAGS::parse() ever be called? I suspect not, and this problem occurs only when there is no cc_config.xml file.

Note that log_flags.init() is also called in CDlgDiagnosticLogFlags::CDlgDiagnosticLogFlags(), but that is in the manager and so won't affect this issue.

[RichardHaselgrove]

The observed problem (no default log flags) only occurs at startup when no cc_config.xml is present. That's why experienced users missed it at and before launch - we all have a cc_config.xml file, and for us the client behaved normally.

If you subsequently invoke a cc_config.xml file, either by manually reading an existing file, or by using the GUI picker, it behaves normally. It's only the placement of the defaults in the initiator which is failing. My empirical solution is to move the default values into the structure definition, where they do get picked up at initiation, but don't get in the way of subsequent alterations.

[CharlieFenton]

The observed problem (no default log flags) only occurs at startup when no cc_config.xml is present.

Right. That was due to the removal of the constructor, which called init() even when there was no file to parse (so LOG_FLAGS::parse() was not invoked.)

Thanks for the explanation of your approach; I didn't really understand what you are doing; I didn't know you could put initialized variables in a struct declaration. I have a couple of questions, though:
Since LOG_FLAGS:init() is apparently never called (when there is no cc_config.xml file), that means *this = log_flag_defaults; is never called.
Does that mean none of the other instance variables are initialized?
Does that matter? If not, what is the point of even having the LOG_FLAGS:init() method?

[RichardHaselgrove]

I can see log_flags.init being called in read_config_file, at
https://github.com/BOINC/boinc/blob/master/client/log_flags.cpp#L554

This in turn is called from main, at
https://github.com/BOINC/boinc/blob/master/client/main.cpp#L237

If read_config_file fails to open the file, it bails out quickly, without doing any parsing. But it prints
cc_config.xml not found - using defaults
along the way. This is the path we are following.

The default values were set by the old, v7.14.2, 'memset' init function. They are not being set by the new, v7.16.5, init function, and the users have noticed.

Empirically - but by analogy with the ability to set default values in a SQL create table definition - I found that I could put initialized variables in a struct declaration. I haven't looked for a formal language specification reference for that, but it caused no complaint from either of the compilers I listed, and the default values are operational. I make no claim beyond that.

[CharlieFenton]

Empirically - but by analogy with the ability to set default values in a SQL create table definition

Well, I've learned something new. Thank you.

This is the path we are following.

Thank you for explaining the cause of the problem (presumably for others who might read this) in more detail than I wrote.

the default values are operational.

As long as you've tested successfully, I'm OK with it. My preference would still have been to do it in the constructor, because IMO that makes it easier (more obvious) for someone reading the code to see that those 3 variables are being set, and that would have simply reversed the original faulty modification. But I'm comfortable with whatever works.

[AenBleidd]

Currently we're still using VS2010 for Windows release. This code will not work because this type of initialization was added in c++11 only

[RichardHaselgrove]

OK, confirmed. Why didn't somebody say that before? Dug out an ancient copy of Visual Studio Express 2010, and the exact error message is

only static const integral data members can be initialized within a class

Was there any real reason for eliminating memset in this particular function? As I read it, the concern was with clearing complex structures? This one is a simple array of boolean values (fixed length), it's initialised at startup, and released when the program exits. Is that dangerous? If not, why don't we revert here?

[AenBleidd]

memset isn't initialize the members of the structure, it's just initialize a block of memory. I'll look for better solution on this weekend.

[RichardHaselgrove]

The original code is visible in the 7.14 release branch:

LOG_FLAGS::LOG_FLAGS() {
    init();
}

void LOG_FLAGS::init() {
    memset(this, 0, sizeof(LOG_FLAGS));
    // on by default (others are off by default)
    //
    task = true;
    file_xfer = true;
    sched_ops = true;
}

The memset sets all to zero, and then the three defaults are set separately.
The replacement in master uses the same default value initialisers, but they are ineffectual.

[RichardHaselgrove]

Closing in favour of #3643, for VS2010 compatibility.