[hipl-dev] Re: [Merge] lp:~hipl-core/hipl/libhip into lp:hipl

On Sun, Feb 12, 2012 at 10:33:00PM +0200, Xin Gu wrote:
> On 12/02/12 15:25, Diego Biurrun wrote:
> >
> >On Fri, Feb 10, 2012 at 02:24:28PM +0000, Xin wrote:
> >>Xin has proposed merging lp:~hipl-core/hipl/libhip into lp:hipl.
> >>
> >>@@ -244,21 +255,24 @@
> >>
> >>  ### static library dependencies ###
> >>
> >>-hipd_hipd_LDADD                          = lib/core/libhipcore.la
> >>-hipfw_hipfw_LDADD                        = lib/core/libhipcore.la
> >>+hipd_hipd_LDADD                          = lib/hipdaemon/libhipdaemon.la
> >>+hipfw_hipfw_LDADD                        = lib/hipdaemon/libhipdaemon.la
> >Why does the firewall depend on libhipdaemon?
> 
> Because modules/midauth/hipd/midauth.c is used both for building fw
> and libdaemon. Well, I am not an autotool expert, perhaps it is not
> the best solution.

This is not related to autotools at all.  Suddenly you need to link
the firewall against all of hipd.  What symbols did you move and why
did you move it to the libhipdaemon code and not to the common code.

> >>--- hipd/esp_prot_hipd_msg.c        2011-12-16 13:37:33 +0000
> >>+++ lib/hipdaemon/esp_prot_hipd_msg.c       2012-02-10 14:23:36 +0000
> >>@@ -57,6 +57,10 @@
> >>
> >>+int  esp_prot_active               = 0;
> >>+int  esp_prot_num_transforms       = 0;
> >>+long esp_prot_num_parallel_hchains = 0;
> >This looks suspicious.  Why are you moving these global variables, but
> >not the extern declarations?
> >
> >Why is this part of this merge request and split off and done separately?
> >The same applies to all the other similar changes.  I'm not convinced you
> >need to do them (here).
> 
> Because HIPL daemon and libhip both link to libhipdaemon.la, but
> hipd.c is not part of libhipdaemon, so those global variables have
> been moved to other files otherwise libhip can not access them.

Why did you move them to these files in the first place?

> >>--- hipd/hadb.c     2012-01-25 20:45:27 +0000
> >>+++ lib/hipdaemon/hadb.c    2012-02-10 14:23:36 +0000
> >>@@ -105,6 +105,23 @@
> >>
> >>+/* Flag to show if hipl is running in libhip mode (=1) or normal mode (=0).
> >>+ * This variable should NOT be accessed directly. Always use the accessor
> >>+ * functions instead.
> >>+ */
> >>+static int hipd_library_mode = 0;
> >>+
> >>+int is_libhip_mode()
> >>+{
> >>+    return hipd_library_mode;
> >>+}
> >>+
> >>+int set_libhip_mode()
> >>+{
> >>+    hipd_library_mode = 1;
> >>+    return 0;
> >>+}
> >Why in this file?
> 
> I moved them to init.c, also rename them with a "hip_" prefix since
> they are global.

Slightly unrelated, but hip_ is not a good prefix, it should be hipl_
if at all.

> >>--- hipd/init.c     2012-01-18 21:21:26 +0000
> >>+++ lib/hipdaemon/init.c    2012-02-10 14:23:36 +0000
> >>@@ -74,12 +75,14 @@
> >>  #include "accessor.h"
> >>  #include "close.h"
> >>  #include "dh.h"
> >>+#include "esp_prot_hipd_msg.h"
> >>  #include "esp_prot_light_update.h"
> >>  #include "hadb.h"
> >>  #include "hidb.h"
> >>  #include "hip_socket.h"
> >>  #include "hipd.h"
> >>  #include "hiprelay.h"
> >>+#include "init.h"
> >>  #include "input.h"
> >>  #include "maintenance.h"
> >>  #include "nat.h"
> >>@@ -88,9 +91,8 @@
> >>  #include "output.h"
> >>  #include "pkt_handling.h"
> >>  #include "registration.h"
> >>+#include "socket_wrapper.h"
> >>  #include "user.h"
> >>-#include "init.h"
> >>-#include "hipd/esp_prot_hipd_msg.h"
> >We always have the .h file that corresponds to a given .c file last.
> 
> Hmm, I realize that there are several rules for the order of the
> #include. Would be nice if they get documented in HACKING for
> newcomer like me :)

True, but you should also not rearrange things w/o need to do so.
I'll try to update HACKING this week, there's a bunch of stuff I
wanted to add in there.

> >>@@ -866,7 +1074,7 @@
> >>   */
> >>-static void hip_close(int signum)
> >>+static void hipd_close(int signum)
> >>  {
> >>      static int terminate = 0;
> >>
> >>@@ -928,8 +1136,8 @@
> >>
> >>      /* Register signal handlers */
> >>-    signal(SIGINT, hip_close);
> >>-    signal(SIGTERM, hip_close);
> >>+    signal(SIGINT, hipd_close);
> >>+    signal(SIGTERM, hipd_close);
> >>      signal(SIGCHLD, sig_chld);
> >I repeat: Please push such changes separately.
> >
> >If you do not understand a review comment or disagree, ask or speak up.
> >Just ignoring review comments can be construed as rude behavior.
> 
> I think this change is related to this merge, because we introduce a
> new method "hip_close()" as HIP socket API, the previous "hip_close"
> method in init.c is changed to hipd_close() to avoid conflict.

Then why didn't you say this previously when I first mentioned you
could/should separate this?

And I maintain that you should separate this.  The change makes sense
outside of this merge request and getting this in will reduce the size
of the merge request.

smaller merge request --> quicker review cycle --> sooner merge

> >>@@ -1090,6 +1298,68 @@
> >>
> >>+int libhipd_init(void)
> >>+{
> >>+    set_libhip_mode();
> >>+    hip_nat_status = 1;
> >>+#ifdef CONFIG_HIP_FIREWALL
> >>+    hipfw_status = 0;
> >>+#endif
> >You never tested in a setup with the firewall enabled.
> >
> >I'm getting more and more sceptical of this whole merge proposal.
> >It has obviously seen little to no testing, at least outside of
> >the very basic standard configuration that it worked in.
> >
> >It seems that the previous iteration was never compiled.  What sort of
> >testing did you do?  Does this pass 'make alltests'?  Does it pass the
> >autobuilder?
> 
> This part of code is unchanged since I started to work on the libhip
> branch. The reason of this #ifdef is that hipfw cannot handle the
> traffic from libhip, so this hipfw_status is always set to 0.

You misunderstand - the variable is unused, so you never compiled
with the firewall enabled, since -Werror would have killed the
build due to the unused variable.

> I didn't know there are other testing methods like 'make alltests'
> and autobuilder, but before every commit I make sure the code pass
> the 'make check'. from now on I will also check 'make alltests'.
> Could you give some hints about how to use the autobuilder you
> mentioned?

Who is mentoring you over in Finland and why are they not giving you
appropriate hints? :)

The autobuilder is the script tools/hipl_autobuild.sh, it should work
if you run it directly.

You could also just read Makefile.am to find out about available targets.
You could also read the output of configure to see which options are
enabled or not.  Also look at 'configure --help' for available options.

> >>--- lib/hipdaemon/socket_wrapper.c  1970-01-01 00:00:00 +0000
> >>+++ lib/hipdaemon/socket_wrapper.c  2012-02-10 14:23:36 +0000
> >>@@ -0,0 +1,812 @@
> >>+
> >>+/**
> >>+ * @file
> >>+ * This file contains implementation of libhip API.
> >>+ *
> >>+ */
> >pointless empty line
> >
> >libhip makes no sense, it should be libhip_l_.
> >
> >You are also missing some articles in that sentence, i.e.
> >
> >   This file contains the implementation of the libhipl API.
> 
> So do you suggest me to change all the libhip to libhipl? Is it a
> final decision? I guess quite a lot of places need to changed.

I already said so before, yes.  Why did you not speak up earlier?

> >>+static void build_sockaddr(struct in6_addr *const addr, const uint16_t 
> >>port,
> >>+                           struct sockaddr_storage *const ss)
> >>+{
> >>+
> >>+    if (IN6_IS_ADDR_V4MAPPED(addr)) {
> >>+        struct sockaddr_in *const in = (struct sockaddr_in *) ss;
> >>+        in->sin_family = AF_INET;
> >>+        IPV6_TO_IPV4_MAP(addr,&in->sin_addr);
> >>+        in->sin_port = port;
> >>+    } else {
> >>+        struct sockaddr_in6 *const in6 = (struct sockaddr_in6 *) ss;
> >>+        in6->sin6_family = AF_INET6;
> >>+        ipv6_addr_copy(&in6->sin6_addr, addr);
> >>+        in6->sin6_port = port;
> >>+    }
> >Why do you use variable indirection?
> 
> Hmm, do you mean the declaration inside the if and else block? I
> assume that define variable at the beginning of a code block is ok,
> because that's the place they get initialized. Well, it depends on
> the code style.

No, I'm wondering why you need to work with sockaddr_in instead of
sockaddr_storage directly.  However, I keep confusing those two
structs and I'm getting tired tonight.  I'll look it up tomorrow.

> >>+/**
> >>+ * Initialization function for socket wrapper functionality
> >>+ */
> >>+void hip_init_socket_wrapper(void)
> >>+{
> >>+    hip_ll_init(&socket_list);
> >>+    memset(&default_hit, 0, sizeof(default_hit));
> >Global variables are initialized to 0, so this seems pointless.
> >
> >>+/**
> >>+ * Add peer hit-addr mapping to hadb.
> >>+ * @param peer_hit peer's hit
> >>+ * @param peer_addr peer's addr, v4 addr should be mapped.
> >Vertical alignment would make this more readable.
> >
> >>+ * @return 0 if success, -1 otherwise.
> >s/if/on/
> >
> >>+ */
> >>+int add_peer_info(const hip_hit_t *peer_hit,
> >>+                  const struct in6_addr *peer_addr)
> >nit: Unbreak this line.
> >
> 
> All above fixed.

It's nice that you keep mentioning all the stuff you fixed, but to be
honest I would really prefer if you snip whatever you implement and just
reply to the parts that need clarification.  Much less lines to scroll
through :)

> >>+{
> >>+int add_peer_info(const hip_hit_t *peer_hit, const struct in6_addr 
> >>*peer_addr)
> >>+    return hip_hadb_add_peer_info(peer_hit, peer_addr, NULL, NULL);
> >>+}
> >Does this even compile?
> >
> >I'm stopping my review here.  Sorry, but I feel like I'm wasting precious
> >time on things that haven't seen even minimal testing.
> 
> Where do you get those line of diff? I cannot find it in my merge
> request mail. If the code cannot be compiled, I won't ask for a
> merge...

Sorry, I fucked up somewhere, dunno how, my apologies...

> >>--- test/check_hipnetcat.c  1970-01-01 00:00:00 +0000
> >>+++ test/check_hipnetcat.c  2012-02-10 14:23:36 +0000
> >>@@ -0,0 +1,208 @@
> >>+    hipnc_test_start(serv_argv, client_argv);
> >>+}
> >>+END_TEST
> >>+
> >>+static void hipnc_test_start(char *serv_argv[], char *client_argv[])
> >>+{
> >>+
> >>+    if (pid == 0) {
> >>+        if (execv("test/hipnetcat", serv_argv)) {
> >The conditions can be combined, same below.
> 
> How? I think the code now is quite clear though.

&&

> >>+    if (pid == 0) {
> >>+        sleep(1);
> >What do you sleep()?
> 
> The hipnetcat client and server are both started, client waits one
> second to make sure server is ready.

Hmmm, ugly - are you sure one second is enough?  What if the machine
is loaded and startup is delayed?  The solution seems brittle to me.

> >>+        if (execv("test/hipnetcat", client_argv)) {
> >Does this work when building out-of-tree?
> 
> What do you mean building out-of-tree?

Building from a directory outside of the directory that contains the
program sources.  Fire up Google for further details.

> This link might be easier for you to view my changes this round:
> http://bazaar.launchpad.net/~hipl-core/hipl/libhip/revision/4870?remember=4870&compare_revid=4868

Somehow I prefer to work through mail.  I guess I should try some of
that newfangled stuff as well.  I'll do the next review round through
launchpad, let's see ...

Diego

Other related posts: