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

  • From: Xin Gu <eric.nevup@xxxxxxxxx>
  • To: hipl-dev@xxxxxxxxxxxxx
  • Date: Mon, 13 Feb 2012 10:39:38 +0200

On 13/02/12 00:26, Diego Biurrun wrote:
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.

In trunk, the modules/midauth/hipd/midauth.c is used to build both hipd and hipfw. They are both executable, so it is fine. Now since this file goes to libhipdeamon, a share library, autotools will complain conflict because it is used both for share lib and executable.

--- 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?

Those moves come from previous work in libhip branch. I think it is reasonable, if you consider to share code between hipd and libhip.


--- 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.

Ok, I will change all of those prefix staff related to this merge then.


@@ -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

Sorry I didn't say this at the first place. I will push it separately to trunk soon.

@@ -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.

Hmm, I didn't get it. In my environment, the CONFIG_HIP_FIREWALL is defined, and I have this -Werror option on, but I never get compilation error on unused variable. It is a global variable, how can compiler give this error?

BTW, if the CONFIG_HIP_FIREWALL is not defined, the compilation does break, but not break here. It is not related to this merge but will it be a problem?


--- 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 = fork()) > 0) {
        serv_pid = pid;
        printf("server pid: %d\n", serv_pid);
    }

    if (pid == 0) {
        if (execv("test/hipnetcat", serv_argv)) {
            perror("execv");
            return;
        }
    }

you mean && this two if statements? I am confused, they are 2 processes here I don't know how can you do that...


+    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.

For most of cases 1 sec is far more than enough. If the machine is loaded, probably other tests will also fail, because the check framework have a default timeout also, if I remember correctly.


+        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.


Hmm, that's a good question, I just tried it on my machine, it runs well :)


Xin

Other related posts: