[hipl-dev] Re: [Merge] lp:~toxedvirus/hipl/hipfw-modules into lp:hipl
- From: Diego Biurrun <diego@xxxxxxxxxx>
- To: mp+40891@xxxxxxxxxxxxxxxxxx
- Date: Mon, 15 Nov 2010 19:26:41 +0100
On Mon, Nov 15, 2010 at 05:12:04PM +0000, Andrius Bentkus wrote:
> Andrius Bentkus has proposed merging lp:~toxedvirus/hipl/hipfw-modules into
> lp:hipl.
>
> Requested reviews:
> Diego Biurrun (diego-biurrun)
>
>
> --- firewall/firewall.c 2010-10-24 21:32:10 +0000
> +++ firewall/firewall.c 2010-11-15 17:11:47 +0000
> @@ -170,40 +178,122 @@
> */
> int hip_fw_async_sock = 0;
>
> +
> /**
unrelated whitespace change
> +static int hipfw_option_handler(const char option, char *argument)
> +{
> + switch (option) {
> + case 'A':
> + accept_hip_esp_traffic_by_default = 1;
> + restore_accept_hip_esp_traffic = 1;
> + break;
> + case 'b':
> + foreground = 0;
> + break;
> + case 'd':
> + log_level = LOGDEBUG_ALL;
> + break;
> + case 'e':
> + hip_userspace_ipsec = 1;
> + hip_esp_protection = 1;
Please align the =, more below.
> + hip_cmd_init(&option_list, firewall_modules, firewall_num_modules);
> +
> + if (hip_cmd_options_parse(argc, argv, &option_list))
> + {
K&R places { on the same line as keywords.
> --- lib/core/modularization.c 2010-11-15 13:37:13 +0000
> +++ lib/core/modularization.c 2010-11-15 17:11:47 +0000
> @@ -504,3 +507,48 @@
> + */
> +int hip_lmod_load_modules(const struct module_info *const modules, const
> unsigned int size)
Please break overly long lines where easily possible, like here for
example.
Many more such instances exist below.
> --- lib/core/options.c 1970-01-01 00:00:00 +0000
> +++ lib/core/options.c 2010-11-15 17:11:47 +0000
> @@ -0,0 +1,379 @@
> +
> +/**
> + *
> + * @brief The option extensions for libmodularization (lmod).
libmodularization no longer exists.
> +#include "common.h"
> +#include "options.h"
> +#include "modularization.h"
> +#include "modules/modules.h"
> +#include "lib/core/linkedlist.h"
nit:
#include "lib/core/linkedlist.h"
#include "modules/modules.h"
#include "common.h"
#include "modularization.h"
#include "options.h"
> +/**
> + * Print a well formated help guide for a given list of options.
well-formatted
> + * @param appname Name of the application name to be used in the help.
application name to be used in the help
> +void hip_cmd_print_usage(const char *appname, const struct
> hip_cmd_option_list *list)
> +{
> + struct hip_cmd_option_block *block;
> + unsigned int i;
> + hip_ll_node_t *iter = NULL;
> +
> + printf("Usage: %s", appname);
> +
> + while ((iter = hip_ll_iterate(list->linkedlist, iter))) {
> + block = (struct hip_cmd_option_block *)iter->ptr;
useless cast
> + for (i = 0; i < block->size; i++) {
> + char *pch = strchr(block->options[i].description, '=');
> + int pos = (int)((unsigned long)pch - (unsigned
> long)&(block->options[i].description));
At least the double cast could be avoided by using a different type
for pos or casting to the right type directly.
Better would be no casts at all.
> + if (pos == 0) {
> + printf(" [-%c]", block->options[i].short_name);
> + } else {
> + // TODO: remove this super hack ... but it works so nicely
> + block->options[i].description[pos] = '\0';
> + printf(" [-%c %s]", block->options[i].short_name,
> block->options[i].description);
> + block->options[i].description[pos] = '=';
> + }
> + }
> + }
> + printf("\n");
> + iter = NULL;
> + while ((iter = hip_ll_iterate(list->linkedlist, iter))) {
> + block = (struct hip_cmd_option_block *)iter->ptr;
useless cast
> +/**
> + * Get an option from a list
.
> +void hip_cmd_options_init(struct hip_cmd_option_list *list)
> +{
> + list->linkedlist = malloc(sizeof(hip_ll_t));
> + hip_ll_init(list->linkedlist);
> + list->missing_argument = missing_argument;
> + list->missing_option = missing_option;
Align the =.
> +static unsigned int hip_cmd_options_string_size(const struct
> hip_cmd_option_list *list)
> +{
> + unsigned int i, size = 0;
> + struct hip_cmd_option_block *block;
> + hip_ll_node_t *iter = NULL;
> + while ((iter = hip_ll_iterate(list->linkedlist, iter))) {
> + block = (struct hip_cmd_option_block *)iter->ptr;
> + for (i = 0; i < block->size; i++) {
> + size += block->options[i].type + 1;
Indentation is off.
> +int hip_cmd_options_parse(const int argc,
> + char **argv,
> + const struct hip_cmd_option_list *list)
> +{
> + int ch;
> + struct hip_cmd_option *option;
> + char *opts = hip_cmd_options_create_string(list);
> +
> + while ((ch = getopt(argc, (char **)argv, opts)) != -1) {
The cast is unnecessary, argv is already char**.
> + char cch = (char)ch;
I'm very suspicious of this way of converting int to char.
> + switch (cch)
> + {
> + case '?':
> + // handle not existent argument
> + if (list->missing_option != NULL) {
All these '!= NULL' in if conditions are pointless.
> +int hip_cmd_init(struct hip_cmd_option_list *option_list,
> + const struct module_info *const modules,
> + const unsigned int size)
Indentation is off.
> + // check for bad paramters
paramEters
I would like to again politely suggest the use of a spellchecker.
> --- lib/core/options.h 1970-01-01 00:00:00 +0000
> +++ lib/core/options.h 2010-11-15 17:11:47 +0000
> @@ -0,0 +1,96 @@
> +
> +/**
> + *Use this definition in order to add auto variables in a
> + * much more uncomplicated way */
"less complicated"; "uncomplicated" is not an English word.
> +// change the code accordingly in hip_cmd_options_create_string@xxxxxxxxx if
> you touch this
> +// hip_cmd_opt_string_size@xxxxxxxxx
What needs to be changed and why? This should be refactored instead.
> +enum hip_cmd_option_types
> +{
> + OPTION_TYPE_STANDARD = 0,
> + OPTION_TYPE_REQUIRED = 1,
> + OPTION_TYPE_OPTIONAL = 2
Indentation should be 4 spaces, more below.
> --- modules/heartbeat/hipd/heartbeat.h 2010-09-25 18:30:26 +0000
> +++ modules/heartbeat/hipd/heartbeat.h 2010-11-15 17:11:47 +0000
> @@ -25,9 +25,9 @@
> * OTHER DEALINGS IN THE SOFTWARE.
> */
>
> -#ifndef HIP_HIPD_HEARTBEAT_H
> -#define HIP_HIPD_HEARTBEAT_H
> +#ifndef HIP_MODULES_HEARTBEAT_HIPD_HEARTBEAT_H
> +#define HIP_MODULES_HEARTBEAT_HIPD_HEARTBEAT_H
>
> int hip_heartbeat_init(void);
>
> -#endif /* HIP_HIPD_HEARTBEAT_H */
> +#endif /* HIP_MODULES_HEARTBEAT_HIPD_HEARTBEAT_H */
These (and the others) are unrelated. They can be committed to trunk
separately anytime.
Diego
Other related posts:
