Add cli options for all config options (except for deprecated ones) #841
Conversation
|
I appreciate the desire for this change. It's a pretty big one, though. This change would also require:
Before you put more work into this -- let me talk to the team about it to decide if we want to support this. I'll get back to you. |
There was a problem hiding this comment.
Thank you for your patience. After speaking with the team about it and considering the effect, we decided we actually really like this change. But I do have some requests.
At first we considered asking you to add new options to make it so we can have all command line options mirror the config options, and then deprecate the old ones. However, I think that is perhaps too much to ask.
Another thing we considered was if it made sense to add a feature like -C ConfigFileOption=Value to make it so any config file option could be passed in as a command line option, and so we wouldn't clutter the --help strings and manpages with a lot more options. However, this too seemed like a big ask and clunky considering we already support many if not most features in both command-line option and config option form.
In the end, we thought it best to accept your approach, but with some minor corrections. I think I found all of the issues with consistency with existing options and have added comments for each.
But in addition to the individual corrections, we will need to add all of the new command line options to the --help messages and theand manpage documentation files for the affected programs. Can you please add these?
|
|
||
| {"AllowAllMatchScan", NULL, 0, CLOPT_TYPE_BOOL, MATCH_BOOL, 1, NULL, 0, OPT_CLAMD, "Permit use of the ALLMATCHSCAN command.", "yes"}, | ||
| {"AllowAllMatchScan", "allow-all-match-scan", 0, CLOPT_TYPE_BOOL, MATCH_BOOL, 1, NULL, 0, OPT_CLAMD, "Permit use of the ALLMATCHSCAN command.", "yes"}, |
There was a problem hiding this comment.
Please change it to --allow-allmatch-scan (allmatch as one word) for consistency clamdscan and clamscan's --allmatch option.
I hate to ask this because I like it as two words better ... but it's better to have consistency with the other command line options.
|
|
||
| {"LeaveTemporaryFiles", NULL, 0, CLOPT_TYPE_BOOL, MATCH_BOOL, 0, NULL, 0, OPT_CLAMD, "Don't remove temporary files (for debugging purposes).", "no"}, | ||
| {"LeaveTemporaryFiles", "leave-temporary-files", 0, CLOPT_TYPE_BOOL, MATCH_BOOL, 0, NULL, 0, OPT_CLAMD, "Don't remove temporary files (for debugging purposes).", "no"}, |
There was a problem hiding this comment.
Please change this to leave-temps to match clamscan's option for the same feature.
|
|
||
| {"GenerateMetadataJson", NULL, 0, CLOPT_TYPE_BOOL, MATCH_BOOL, 0, NULL, 0, OPT_CLAMD, "Record metadata about the file being scanned.\nScan metadata is useful for file analysis purposes and for debugging scan behavior.\nThe JSON metadata will be printed after the scan is complete if Debug is enabled.\nA metadata.json file will be written to the scan temp directory if LeaveTemporaryFiles is enabled.", "no"}, | ||
| {"GenerateMetadataJson", "generate-metadata-json", 0, CLOPT_TYPE_BOOL, MATCH_BOOL, 0, NULL, 0, OPT_CLAMD, "Record metadata about the file being scanned.\nScan metadata is useful for file analysis purposes and for debugging scan behavior.\nThe JSON metadata will be printed after the scan is complete if Debug is enabled.\nA metadata.json file will be written to the scan temp directory if LeaveTemporaryFiles is enabled.", "no"}, |
There was a problem hiding this comment.
Please change this to gen-json to match clamscan's option for the same feature.
Note: it also pains me to make this request, because I like the more verbose option more. but I think consistency is more important -- and --gen-json is also a lot easier to type.
|
|
||
| {"LogVerbose", NULL, 0, CLOPT_TYPE_BOOL, MATCH_BOOL, 0, NULL, 0, OPT_CLAMD | OPT_FRESHCLAM | OPT_MILTER, "Enable verbose logging.", "yes"}, | ||
| {"LogVerbose", "log-verbose", 0, CLOPT_TYPE_BOOL, MATCH_BOOL, 0, NULL, 0, OPT_CLAMD | OPT_FRESHCLAM | OPT_MILTER, "Enable verbose logging.", "yes"}, |
There was a problem hiding this comment.
freshclam, clamscan, clamdscan, sigtool, and clamonacc all use --verbose (-v) (separate option on line 118).
That --verbose option and LogVerbose presently do the same thing (enabling verbose mode for both messages and log file).
I think it would be best to combine these lines (118 and 269) into one option for all of the programs in question, using the --verbose option name for consistency freshclam and clamscan's existing --verbose option.
|
|
||
| {"Foreground", "foreground", 'F', CLOPT_TYPE_BOOL, MATCH_BOOL, 0, NULL, 0, OPT_CLAMD | OPT_FRESHCLAM | OPT_MILTER | OPT_CLAMONACC, "Don't fork into background.", "no"}, | ||
|
|
||
| {"Debug", NULL, 0, CLOPT_TYPE_BOOL, MATCH_BOOL, 0, NULL, 0, OPT_CLAMD | OPT_FRESHCLAM, "Enable debug messages in libclamav.", "no"}, | ||
| {"Debug", "debug", 0, CLOPT_TYPE_BOOL, MATCH_BOOL, 0, NULL, 0, OPT_CLAMD | OPT_FRESHCLAM, "Enable debug messages in libclamav.", "no"}, |
There was a problem hiding this comment.
There is already a --debug option (line 116). I think the correct thing would be to combine this with that one.
I think that will work correctly though I'm not entirely sure. If not, we can leave as-is, since the --debug option already works for clamd and freshclam, plus other programs.
| // The Whitelist option should remain functional for a version or two, but has been deprecated in the documentation in favor of "AllowList". | ||
| {"Whitelist", NULL, 0, CLOPT_TYPE_STRING, NULL, -1, NULL, 0, OPT_MILTER, "This option specifies a file which contains a list of basic POSIX regular\nexpressions. Addresses (sent to or from - see below) matching these regexes\nwill not be scanned. Optionally each line can start with the string \"From:\"\nor \"To:\" (note: no whitespace after the colon) indicating if it is,\nrespectively, the sender or recipient that is to be allowed.\nIf the field is missing, \"To:\" is assumed.\nLines starting with #, : or ! are ignored.", "/etc/allowed_addresses"}, | ||
| {"Whitelist", "whitelist", 0, CLOPT_TYPE_STRING, NULL, -1, NULL, 0, OPT_MILTER, "This option specifies a file which contains a list of basic POSIX regular\nexpressions. Addresses (sent to or from - see below) matching these regexes\nwill not be scanned. Optionally each line can start with the string \"From:\"\nor \"To:\" (note: no whitespace after the colon) indicating if it is,\nrespectively, the sender or recipient that is to be allowed.\nIf the field is missing, \"To:\" is assumed.\nLines starting with #, : or ! are ignored.", "/etc/allowed_addresses"}, |
There was a problem hiding this comment.
I think we can skip this one. It is supposed to be deprecated.
|
@abautu are you ready for re-review? I see you merged some more changes into your PR. Although since then a merge conflict has come up. |
|
Also I did not see any answer to the comments I left on the PR. |
I added long-named CLI options for all config options that did not have one and are not deprecated. This would allow clamd to be started without a config file and configured from command line only.