honeynet / cuckooml

CuckooML: Machine Learning for Cuckoo Sandbox
https://honeynet.github.io/cuckooml/
145 stars 47 forks source link

Adding a plotting switch and changes made as suggested by So-Cool. #21

Open greninja opened 7 years ago

So-Cool commented 7 years ago

@greninja I had a look at your pull request.
First of all, in commit f12cc8d you've added a print statement to lib/cuckoo/common/constants.py and an empty line to lib/cuckoo/common/config.py. Both of these files mustn't be changed.

Moreover, on lines 803 and 1146 of cuckooml.py there are more than 80 characters; please split these lines in two.

Finally, could you please squash all the commits into one. Much appreciated!

Thanks for the contribution BTW.

So-Cool commented 7 years ago

Rather than piling up new commits, could you please revert these two files to their original form.
Also please squash the rest of the commits into a single one; this is fairly minor issue hence, one commit should suffice.

greninja commented 7 years ago

Is it fine if I just close this PR and make a new one as this is getting a bit messy?

So-Cool commented 7 years ago

Please fix this one. It should be fairly simple: check out these two files to their original commit and squash all the other.

greninja commented 7 years ago

commit e214147 I believe does the squashing part. I have made some pretty bad goof ups. I thoroughly apologise for that. I am still a learner.

So-Cool commented 7 years ago

No problem @greninja. We're heading in the right direction. e214147 looks good except that it removes lines 803 and 1146 rather than introducing lines 810--813 and 1156--1158 directly.

Moreover, this pull request is still 10 commits, please try to make it only 1, i.e. e214147 with the changes I've mentioned above.

greninja commented 7 years ago

Closing this PR. Had some issues with local repo. Will open it again.

greninja commented 7 years ago

Have defaulted the global variable "imported" to true and it will be set to False when the imports fail. Is it fine? Also will look into the .swp file issue.

So-Cool commented 7 years ago

The .swp is probably artefact of your text editor and it shouldn't be there.
The rest sounds good, but please use some readable variable name, like PLOTTING_LIBRARIES_AVAILABLE.
Also think about what happens if plotting is disabled in config: based on greninja/cuckooml@f7dc849 this global variable will still be True even though it shouldn't.
Also have a look at Google Python Style Guide and try to follow it as closely as possible. E.g. global variables are usually all capitals.

Finally, please keep indentation clean and tidy.

greninja commented 7 years ago

Referring to your third line, @So-Cool :

Firstly, even if the global variable is set to True (although it shoudn't) it won't lead to crashing of the program because Line 817 will come to the rescue.

Secondly, I found another loophole in this approach. The global variable asserts whether the plotting libraries are available or not. I can add an else block, to the corresponding if block on Line 21, which will set the global variable to False ( this will happen just because the config plotting variable is disabled ) but that wont necesarily be true because the machine might have the libraries available.

So in conclusion, in the case where config variable is disabled , the global variable will mislead the user into believing that the libraries are not available, which may not be true always.

Will clean the indentation and thanks for the link!

So-Cool commented 7 years ago

The variable is not telling whether the libraries are available but whether they are imported. That's why:

PLOTTING_LIBRARIES_IMPORTED = False

if Config("cuckooml").cuckooml.plotting:
    try:
        import matplotlib.pyplot as plt
        import seaborn as sns
        PLOTTING_LIBRARIES_IMPORTED = True
    except ImportError, e:
        print >> sys.stderr, "Plotting libraries are not available."
        print >> sys.stderr, e
        PLOTTING_LIBRARIES_IMPORTED = False

makes more sense. With you approach if plotting is disabled in config this variable will still be True and even though this is programatically correct it's logically inconsistent.

Then, since PLOTTING_LIBRARIES_IMPORTED is conditioned on plotting config you don't need to invoke Config("cuckooml").cuckooml.plotting again in lines 817 and 1164. It then becomes as simple as:

    # Safety check for plotting 
    if not PLOTTING_LIBRARIES_IMPORTED and figures:
            print >> sys.stderr, "Warning: plotting libraries were not imported.\n", \
                          "Plots won't be produced."
            if not Config("cuckooml").cuckooml.plotting:
                print >> sys.stderr, "Plotting is disabled in cuckooml config."
            else:
                print >> sys.stderr, "Plotting libraries are missing."
            figures = False

therefore it's easy to understand.

Please let me know if you have any other questions.