Code review comment for lp:~stefan.goetz-deactivatedaccount/hipl/hidb-db

 review needs-fixing

On Fri, May 20, 2011 at 09:26:05PM +0000, Stefan Götz wrote:
> Stefan Götz has proposed merging lp:~stefan.goetz/hipl/hidb-db into lp:hipl.
>
> --- firewall/user_ipsec_sadb.c 2011-04-05 16:44:22 +0000
> +++ firewall/user_ipsec_sadb.c 2011-05-20 21:25:59 +0000
> @@ -824,29 +823,38 @@
>
> +/**
> * flushes all entries in the sadb
> *
> - * @return -1, if error occurred, else 0
> + * @return 0
> */
> int hip_sadb_flush(void)
> {
> - int err = 0, i = 0;
> - LHASH_NODE *item = NULL, *tmp = NULL;
> - struct hip_sa_entry *entry = NULL;
> -
> - // iterating over all elements
> - list_for_each_safe(item, tmp, sadb, i)
> - {
> - HIP_IFEL(!(entry = list_entry(item)), -1,
> - "failed to get list entry\n");
> - HIP_IFEL(hip_sa_entry_delete(&entry->inner_src_addr, &entry->inner_dst_addr), -1,
> - "failed to delete sa entry\n");
> - }
> + hip_ht_doall(sadb, delete_sa_entry);
>
> HIP_DEBUG("sadb flushed\n");

This debug line is rather pointless IMO, I'd remove it.

> -out_err:
> - return err;
> + return 0;
> }

Why not make the function void instead of always returning the same value?

> --- hipd/cookie.c 2011-05-10 22:14:13 +0000
> +++ hipd/cookie.c 2011-05-20 21:25:59 +0000
> @@ -372,24 +367,10 @@
> /**
> * precreate all R1 packets
> *
> - * @return zero on success or negative on error
> + * @return 0
> */
> int hip_recreate_all_precreated_r1_packets(void)
> {
> - HIP_HASHTABLE *ht = hip_ht_init(hip_hidb_hash, hip_hidb_match);
> - LHASH_NODE *curr, *iter;
> - struct hip_host_id *tmp;
> - int c;
> -
> - hip_for_each_hi(hip_recreate_r1s_for_entry_move, ht);
> -
> - list_for_each_safe(curr, iter, ht, c)
> - {
> - tmp = list_entry(curr);
> - hip_ht_add(HIP_DB_LOCAL_HID, tmp);
> - list_del(tmp, ht);
> - }
> -
> - hip_ht_uninit(ht);
> + hip_for_each_hi(hip_recreate_r1s_for_entry_move, NULL);
> return 0;
> }

ditto

> --- hipd/init.c 2011-05-04 16:20:00 +0000
> +++ hipd/init.c 2011-05-20 21:25:59 +0000
> @@ -1,5 +1,5 @@
> /*
> - * Copyright (c) 2010 Aalto University and RWTH Aachen University.
> + * Copyright (c) 2010-2011 Aalto University and RWTH Aachen University.
> *
> * Permission is hereby granted, free of charge, to any person
> * obtaining a copy of this software and associated documentation
> @@ -28,6 +28,7 @@
> * This file defines initialization functions for the HIP daemon.
> *
> * @note HIPU: BSD platform needs to be autodetected in hip_set_lowcapability
> + * @author Stefan G??tz <email address hidden>
> */

I'm sceptical that removing a function constitutes a real contribution.

There are more similar cases in this merge request, we need to come to
a conclusion what to do with these cases.

Diego