[pisa-src] Re: r2955 - in trunk: Makefile.am openwrt/package/pisa/Makefile pairing/util pairing/util/binary.c pairing/util/call-hipconf.c pairing/util/get-neighbours.c pairing/util/output.c pairing/util/parse.c p...
- From: Diego Biurrun <diego@xxxxxxxxxx>
- To: pisa-src@xxxxxxxxxxxxx
- Date: Tue, 27 Mar 2012 15:46:55 +0200
On Fri, Mar 23, 2012 at 07:15:05PM +0100, Christoph Viethen wrote:
>
> Log:
> Contribute utility which will find Pairing candidates, to be used by
> the shell scripts in the webif GUI. Ensure it gets included in the
> pisa-pairing OpenWrt package, and adapt the Makefile.am appropriately.
>
> --- trunk/Makefile.am Fri Mar 23 17:11:31 2012 (r2954)
> +++ trunk/Makefile.am Fri Mar 23 19:15:04 2012 (r2955)
> @@ -150,6 +152,13 @@
>
> +pairing_util_pisa_pairing_candidates_SOURCES = pairing/util/binary.c
> \
> + pairing/util/call-hipconf.c
> \
> + pairing/util/get-neighbours.c
> \
> + pairing/util/output.c
> \
> +
> pairing/util/pisa-pairing-candidates.c \
> + pairing/util/parse.c
alphabetical order please
> --- /dev/null 00:00:00 1970 (empty, because file is newly added)
> +++ trunk/pairing/util/binary.c Fri Mar 23 19:15:04 2012 (r2955)
> @@ -0,0 +1,189 @@
> +
> +/** @file
> + *
> + * Routines for calling a binary and collecting its output for further
> + * processing.
> + *
> + * @author Christoph Viethen <christoph.viethen@xxxxxxxxxxxxxx>
> + *
> + */
Please leave out the pointless trailing empty line, same in all other places.
> +/**
> + * Clean-up function - will be called whenever we leave
> call_binary_get_pipe()
> + * for any reason, and will clean up different things depending on how far
> + * we got back there.
> + *
> + * Currently just closes file descriptors of a pipe.
> + *
> + * @param readdes
> + * @param writedes
> + */
> +static void call_binary_get_pipe_cleanup(int readdes, int writedes)
Ahem - some parameters look undocumented to me :)
> +/**
> + * This function allocates a pipe and then calls up the named binary, making
> + * sure its output ends up in that pipe. Caller must specify binary's path
> + * name as well as a completely filled-in array of pointers to argv values
> + * (suitable as parameter for execve()).
> + *
> + * Finally, a pointer to the pipe (which now hopefully holds the output from
> + * the named binary) is returned in output_pipe.
> + *
> + * @param output_pipe must point to an address suitable for storing a
> + * pointer to a pipe
> + * @param binary_pathname pathname of the binary to execute
> + * @param binary_argv array of points to argv values, as passed to
> execve()
> + * @param read_stderr whether to catch the output to stderr as well, or
> + * only that which the called binary sends to stdout
> + * @return 0 for "everything okay", or an error code < 0 in
> + * case of failure
> + */
> +int call_binary_get_pipe(FILE **output_pipe, const char *binary_pathname,
> char *const *binary_argv, bool read_stderr)
That line can easily be broken, same for others in this commit.
> + /* make OS execute the binary, fork()ing another process which will call
> + * execve(); contrary to what system() would do internally, we don't
> + * undertake any special kind of signal handling, such as ignoring or
> + * catching anything: if a signal reaches us (being a cgi-bin started by
> + * a web server) or our child process (being rather non-interactive),
our child process .. what?
> + case 0: /* child */
> + /* redirect stdout and stderr to the "write side" of our pipe, so
> + * all output from the binary is collected */
> + if (close(STDIN_FILENO) == 0) {
> + if (dup2(writedes, STDOUT_FILENO) != -1) {
> + if (read_stderr == false ? (close(STDERR_FILENO) == 0) :
> (dup2(writedes, STDERR_FILENO) != -1)) {
> + if (close(writedes) == 0) {
> + if (close(readdes) == 0) {
> + execve(binary_pathname, binary_argv, NULL);
> + /* if execve() works, it never returns */
I think nesting ifs here is less readable than just connecting the
conditions with &&.
> + /* if we get here, something must have gone wrong with the execve()
> or
> + * with the preparatory steps leading up to it */
> + exit(127);
Are you sure you want to do a hard exit here instead of returning and
possibly catching errors at the caller?
> + if (WEXITSTATUS(exec_returncode) != EXIT_SUCCESS) {
> + return CALL_BINARY_GET_PIPE_ERR_BINARY;
> + } else {
> + return CALL_BINARY_GET_PIPE_ERR_NONE;
> + }
nit: Dropping the else and returning outside the if-block looks more
natural to me.
> --- /dev/null 00:00:00 1970 (empty, because file is newly added)
> +++ trunk/pairing/util/call-hipconf.c Fri Mar 23 19:15:04 2012 (r2955)
> @@ -0,0 +1,136 @@
> +
> +/* the name and complete path to the hipconf executable (do not use relative
> + * paths here for security reasons - we'll execute as root) */
> +#define HIPCONF_NAME "hipconf"
> +#define HIPCONF_PATHNAME "/usr/sbin/" HIPCONF_NAME
Are you sure you want to hardcode this? It will not work with locally
installed hipconf versions...
> +/**
> + * This function will perform a number of simple checks of the params that
> + * are supposed to be handed to hipconf.
> + *
> + * @param params
> + * @param num_params
Ahem, see above...
> + * @return 0 for "everything okay", or -1 in case of error
> + */
> +static int call_hipconf_check_params(char **params, const int num_params)
> +{
> +/* size_t issuer_len;
> + * size_t subject_len;
> + * size_t notbefore_len;
> + * size_t notafter_len;
> + * const char allowed_in_v6addresses[] = "0123456789ABCDEFabcdef:";
> + * const char allowed_in_unsigned_int[] = "0123456789";
> + */
Why this commented-out cruft?
> + if (num_params == 4 && !strcmp("daemon", params[1]) && !strcmp("get",
> params[2]) && !strcmp("hi", params[3]) && !strcmp("default", params[4])) {
> + return 0;
> + }
> +
> + if (num_params == 4 && !strcmp("daemon", params[1]) && !strcmp("get",
> params[2]) && !strcmp("ha", params[3]) && !strcmp("all", params[4])) {
> + return 0;
> + }
> +
> + if ((num_params == 4 || num_params == 5) && !strcmp("daemon", params[1])
> && !strcmp("acquire", params[2]) && !strcmp("certificate", params[3])) {
> + /* FIXME: definitely would like more checking code here */
> + return 0;
> + }
more easily breakable long lines
> +/**
> + * This function makes sure the hipconf binary gets called, its output ending
> + * up in a pipe. The variadic parameters get passed on as command-line
> + * arguments to hipconf, after some simple argument-specific plausibility
> + * checks.
> + *
> + * @param output_pipe must point to an address where to store the pointer
> + * to the pipe to
pointer to a pointer to the output pipe?
> +int call_hipconf(FILE **output_pipe, ...)
> +{
> + va_list ap, ap2;
> + int param_count = 0;
> + int i, result;
> +
> + va_start(ap, output_pipe);
> +
> + /* determine number of parameters this function got called with */
> + va_copy(ap2, ap);
> + while (va_arg(ap2, char *) != NULL) {
> + param_count++;
> + }
> + va_end(ap2);
> +
> + /* put variadic parameters into an argv array for execve()ing hipconf */
> + char **const hipconf_argv = malloc((param_count + 2) * sizeof(char *));
> +
> + if (hipconf_argv == NULL) {
> + return CALL_HIPCONF_ERR_ALLOC;
> + }
> +
> + /* FIXME: ugly const-correctness kludge */
> + const char *tempname = HIPCONF_NAME;
> + char *tempname2;
> + memcpy(&tempname2, &tempname, sizeof(char *));
> + hipconf_argv[0] = tempname2;
sizeof(tempname2)
Also, this is extremely ugly. Why don't you memcpy directly into
hipconf_argv?
> --- /dev/null 00:00:00 1970 (empty, because file is newly added)
> +++ trunk/pairing/util/get-neighbours.c Fri Mar 23 19:15:04 2012
> (r2955)
> @@ -0,0 +1,100 @@
> +/*
> + * Copyright (c) 2012, Distributed Systems Group, RWTH Aachen
> + * All rights reserved.
> + */
> +
> +/** @file
> + *
> + * @author Christoph Viethen <christoph.viethen@xxxxxxxxxxxxxx>
> + *
> + */
see above
> +#include "config.h"
> +
> +#include <netinet/in.h>
> +#include <stdbool.h>
> +#include <stdio.h>
> +#include <string.h>
> +
> +#include "protos.h"
system headers before local headers, not a weird mix
> +/* for (int j = 0; ip_argv[j] != NULL; j++) {
> + * printf("ip_argv[%d]=%s\n", j, ip_argv[j]);
> + * }
> + */
Come on, no commented-out cruft please, much less with wrong indentation.
> + /* copy over the contents of the one to the other pointer array */
> + /* FIXME(?): const-correctness kludge */
> + memcpy(ip_argv_dest, ip_argv, 10 * (sizeof(char *)));
see above
> --- /dev/null 00:00:00 1970 (empty, because file is newly added)
> +++ trunk/pairing/util/output.c Fri Mar 23 19:15:04 2012 (r2955)
> @@ -0,0 +1,78 @@
> +/*
> + * Copyright (c) 2012, Distributed Systems Group, RWTH Aachen
> + * All rights reserved.
> + */
> +
> +/** @file
> + *
> + * @author Christoph Viethen <christoph.viethen@xxxxxxxxxxxxxx>
> + *
> + */
> +#include <stdbool.h>
> +#include <stdio.h>
> +
> +#include "pisapair.h"
> +#include "protos.h"
empty line between header #includes and comments
We dropped the @author comments in HIPL, possibly we should do that in
PiSA as well.
> +void output_candidates(struct pairing_candidate *pairing_candidates)
> +{
> + struct pairing_candidate *cur_candidate = pairing_candidates;
> +
> + if (pairing_candidates != NULL) {
Just
if (pairing_candidates) {
is preferred I think, it's more concise, same everywhere.
> --- /dev/null 00:00:00 1970 (empty, because file is newly added)
> +++ trunk/pairing/util/parse.c Fri Mar 23 19:15:04 2012 (r2955)
> @@ -0,0 +1,209 @@
> +
> +struct pairing_candidate *get_pairing_candidates(FILE *hipconf_pipe)
> +{
> +
> + /* read out the pipe line by line */
> + while (fgets(linebuf, sizeof(linebuf), hipconf_pipe) != NULL) {
> + /* only look for entry about an established HA */
> + if (state == 0) {
> + if (prev_linebuf[0] != 0) { /* care first for previous
> line, in case
> + * it hasn't been
> processed yet */
> + if (!strncmp(prev_linebuf, "HA is ESTABLISHED", 17)) {
> + state = WITHIN_HA_ENTRY;
> + cur_candidate = calloc(1, sizeof(struct
> pairing_candidate));
> +
> + if (cur_candidate == NULL) {
> + break;
> + }
> + }
> + prev_linebuf[0] = 0;
> + } else {
> + if (!strncmp(linebuf, "HA is ESTABLISHED", 17)) {
> + state = WITHIN_HA_ENTRY;
> + cur_candidate = calloc(1, sizeof(struct
> pairing_candidate));
> +
> + if (cur_candidate == NULL) {
> + break;
> + }
> + }
> + continue;
> + }
> + }
code duplication
> + if (cur_candidate->flags == (CANDFLAG_HIT_FOUND |
> CANDFLAG_IPV4_FOUND) ||
> + cur_candidate->flags == (CANDFLAG_HIT_FOUND |
> CANDFLAG_IPV6_FOUND)) {
> + if (pairing_candidates == NULL) {
> + pairing_candidates = cur_candidate;
> + } else {
> + struct pairing_candidate **temp;
> +
> + temp = &(pairing_candidates->next);
pointless ()
> + while (*temp != NULL) {
> + temp = &((*temp)->next);
> + }
> +
> + *temp = cur_candidate;
You mostly use temp dereferenced, so you could declare it as a normal
pointer and simplify this block.
> + if (cur_candidate != NULL) {
> + if (cur_candidate->flags == (CANDFLAG_HIT_FOUND |
> CANDFLAG_IPV4_FOUND) ||
> + cur_candidate->flags == (CANDFLAG_HIT_FOUND |
> CANDFLAG_IPV6_FOUND)) {
> + if (pairing_candidates == NULL) {
> + pairing_candidates = cur_candidate;
> + } else {
> + struct pairing_candidate **temp;
> +
> + temp = &(pairing_candidates->next);
> + while (*temp != NULL) {
> + temp = &((*temp)->next);
> + }
> +
> + *temp = cur_candidate;
> + }
> +
> + cur_candidate = NULL;
> + } else {
> + free(cur_candidate);
> + cur_candidate = NULL;
> + }
> + }
code duplication
> + if (family == AF_INET) {
> + do {
> + if ((temp_candidate->flags & CANDFLAG_IPV4_FOUND) &&
> (temp_candidate->ipv4.s_addr == ipv4_addr.s_addr)) {
> + memcpy(temp_candidate->hwaddr, l2addr, 6);
> + temp_candidate->flags |= CANDFLAG_IS_LOCAL;
> + }
> +
> + temp_candidate = temp_candidate->next;
> + } while (temp_candidate != NULL);
> + }
> + if (family == AF_INET6) {
> + do {
> + if ((temp_candidate->flags & CANDFLAG_IPV6_FOUND) &&
> (memcmp(temp_candidate->ipv6.s6_addr, ipv6_addr.s6_addr, 16) == 0)) {
> + memcpy(temp_candidate->hwaddr, l2addr, 6);
> + temp_candidate->flags |= CANDFLAG_IS_LOCAL;
> + }
> +
> + temp_candidate = temp_candidate->next;
> + } while (temp_candidate != NULL);
> + }
> + }
> +}
code duplication
> --- /dev/null 00:00:00 1970 (empty, because file is newly added)
> +++ trunk/pairing/util/pisa-pairing-candidates.c Fri Mar 23 19:15:04
> 2012 (r2955)
> @@ -0,0 +1,68 @@
> +/*
> + * Copyright (c) 2012, Distributed Systems Group, RWTH Aachen
> + * All rights reserved.
> + */
> +
> +/** @file
> + *
> + * @author Christoph Viethen <christoph.viethen@xxxxxxxxxxxxxx>
> + *
> + */
> +
> --- /dev/null 00:00:00 1970 (empty, because file is newly added)
> +++ trunk/pairing/util/pisapair.h Fri Mar 23 19:15:04 2012 (r2955)
> @@ -0,0 +1,36 @@
> +
> +#ifndef _PISAPAIR_H
> +#define _PISAPAIR_H 1
> +
> +#include <netinet/in.h>
> +#include <stdio.h>
see below
> +/*
> + * struct neighbour_entry {
> + * struct in_addr l3addr;
> + * char *device;
> + * uint8_t hwaddr[6];
> + * struct neighbour_entry *next;
> + * };
> + */
more commented-out cruft...
> +#endif
See below, I'm reviewing out-of-order :)
> --- /dev/null 00:00:00 1970 (empty, because file is newly added)
> +++ trunk/pairing/util/protos.h Fri Mar 23 19:15:04 2012 (r2955)
> @@ -0,0 +1,24 @@
> +
> +#ifndef _PROTOS_H
> +#define _PROTOS_H 1
This is not our standard multiple inclusion guard. You are missing the
prefix and we don't define to 1. Also note that identifiers starting
with an underscore and capital letters are reserved for the system, so
you are invading namespaces here.
> +int call_binary_get_pipe(FILE **output_pipe, const char *binary_pathname,
> char *const *binary_argv, _Bool read_stderr);
> +int call_hipconf(FILE **output_pipe, ...);
> +int get_v4neighbours(FILE **, char *);
> +int get_v6neighbours(FILE **, char *);
> +struct pairing_candidate *get_pairing_candidates(FILE *);
> +void process_v4_neighbours(struct pairing_candidate *pairing_candidates,
> FILE *v4neighbours_pipe);
> +void process_v6_neighbours(struct pairing_candidate *pairing_candidates,
> FILE *v6neighbours_pipe);
> +void output_candidates(struct pairing_candidate *pairing_candidates);
These lines are long and could be easily broken.
The header is missing its #include dependencies to compile standalone.
Please add the parameter names for all functions, leaving them out works
of course, but it is bad style as it decreases the usefulness of the
header considerably.
> +#endif
Please comment the #endif.
Diego
--
This is the pisa developer mailing list. Please also subscribe to the main pisa
list at:
//www.freelists.org/list/pisa
Other related posts:
