Giant code-formatting patch

[wclarie]

Initial version of a gigantic code-formatting patch for the src directory (I have done the other directories too, but let's see first how this goes).

This is an attempt at creating a somewhat consistent base, so that it's easier and more fun for everyone to work on the ossec codebase. This is not an attempt at starting a holy war about code formatting,

I've started from https://github.com/ossec/ossec-docs/blob/master/docs/development/oRFC/orfc-2.rst but found the way the formatting rules were described to be very strict and hard to read, so I tried to summarize the principles in this document, which is inspired by multiple places: https://gist.github.com/wclarie/cd05132574fb0191c58a

The summary of what I did is:

• Fixed permissions on all the files (most of the code files were executable for some reason)
• Reformatted using astyle with the settings described in https://gist.github.com/wclarie/cd05132574fb0191c58a
• Cleaned up licences
• Cleaned up use of whitespace (too much vertical whitespace + at end of lines etc)
• Cleaned up / rephrased a lot of the comments in the code (this codebase is slightly over-commented)
• Removed all the $Id$ tags
• More stuff I forgot

Testing was done on OS X 10.9.5 with clang, and on CentOS 6 64bit with gcc. On Mac there is no change to the object files whatsoever, in all normal Makefile targets. On Linux there are some differences because of gcc, but they should be purely gcc code-generation differences, no actual functional differences. I have no other platforms to test this on at the moment.

Happy to hear constructive comments, suggestions etc.

[awiddersheim]

👍

[jrossi]

Travis-ci is showing the Windows builds are failing. You can see the log details here: https://travis-ci.org/ossec/ossec-hids/jobs/47799117

I love this and reviewing it myself now - code base so needed this, but need make sure we to create issues so please work with us as we figure out the best way to review all the changes.

[wclarie]

Going to have a look if I can find the reason for the Windows breakage. I'll need to find out first how this Travis thing works. ;-)

[wclarie]

I think it's probably related to the ordering of include files - that broke some other stuff in my earlier test runs as well.

[jrossi]

travis just executes the items in https://github.com/ossec/ossec-hids/blob/master/.travis.yml file.

Key things for mingw builds of of the windows agent are the following:

sudo apt-get install aptitude && sudo aptitude -y install mingw-w64 nsis )
make V=1 TARGET=winagent

This is translated to simplify here. But this is the install of ubuntu mingw-w64 and nsis on ubuntu (don't know what they are on centos). And then make for winagent

I think it's probably related to the ordering of include files - that broke some other stuff in my earlier test runs as well.

this is common the build for windows as it is HORRID and hard to follow as it is not a makefile but really make.sh :( we need to fix this too.

[wclarie]

Haven't tested this, but I think this should fix the build, as in: no errors.

[jrossi]

ping @cgzones i know you have been busy, but I figured I would ping you on this as I know you care about style stuff.

[awiddersheim]

I started to really dig into this and the spacing bothers me some. I'll use the event channel code as an example since I've been working on it a lot lately. Here is how I spaced things out:

    if ((buffer = calloc(size, sizeof(char))) == NULL)
    {
        log2file(
            "%s: ERROR: Could not calloc() memory to save bookmark (%s) for (%s) which returned [(%d)-(%s)]",
            ARGV0,
            channel->bookmark_filename,
            channel->evt_log,
            errno,
            strerror(errno)
        );

        goto cleanup;
    }

Here it is after the changes:

    if ((buffer = calloc(size, sizeof(char))) == NULL) {
        log2file(
            "%s: ERROR: Could not calloc() memory to save bookmark (%s) for (%s) which returned [(%d)-(%s)]",
            ARGV0, channel->bookmark_filename, channel->evt_log, errno,
            strerror(errno));
        goto cleanup;
    }

I'm just not fond of the spacing and preferred my way a bit better. Not saying we should do exactly what I am doing just think the spacing could be better there IMO. Whatever the majority thinks though.

[wclarie]

Ah, that was indeed a dilemma, perhaps I should have asked. Since you're the one mostly maintaining this part, I have no objection putting the spacing back.

[wclarie]

Does Travis automatically rebuild when I push new commits into the branch for this pull request?

[jrossi]

Yes - but travis is being very very slow as of late. Right now 36abe9a is still waiting in queue to build.

[awiddersheim]

@wclarie the hard part is a lot of the code base does need better spacing while other parts already have decent spacing. Are there any better spacing options available to astyle that might be similar or just better?

[wclarie]

@awiddersheim Those changes to the eventchannel code were actually done manually. Astyle normally doesn't touch this, apart from aligning the continuation with the opening parentheses above it.

[awiddersheim]

Oh wow. You did quite a bit of work then. Sorry to sort of shit on it. You prefer your style? I just find that harder to read.

[wclarie]

Windows agent builds on CentOS now. Only step I can't check is nsis because that's a nightmare to install.

@awiddersheim Yes, I've been working on this for a couple of weeks, while I was getting to know the code. But like I said, I really have no problem putting the eventchannel stuff back to your original function argument spacing. But maybe we should do that after this pull request gets merged? I already have to merge the syslog fqdn patches manually now.

[awiddersheim]

Is that the only place you did those types of spacing changes? It's the only place I looked really. I guess what I'm trying to ascertain is what would the new standard be for such a thing? Trying to avoid past sins and letting things get out of hand again.

[wclarie]

@awiddersheim Yes, I believe that's the only place I did that kind of changes. For functions with lots of arguments, I actually think it makes more sense to line them up like you tend to do. IMO we should leave it up to the individual authors to decide what is the most readable in this case. (There are no explicit instructions in the https://gist.github.com/wclarie/cd05132574fb0191c58a about this.)

[awiddersheim]

@wclarie Sounds good. Thanks for clarifying. In that case my 👍 remains. I'll leave it up to you and @jrossi on whether or not to revert the spacing changes made in the event channel code now or in a later commit.

[wclarie]

So all seems to build properly now. I'm mostly away the next two days, and I would appreciate if you wouldn't merge any other (significant) pull requests before merging this one, because that typically requires a lot of rework. ;-)

[wclarie]

@awiddersheim: I have fixed up the eventchannel code to match your preferred style

@jrossi: How do you feel about merging this? If this is merged, we can move on with real development again...

[jrossi]

Still checking it over. Should be done tonight.