[hipl-dev] Re: [Merge] lp:~hipl-core/hipl/libhip into lp:hipl
- From: Diego Biurrun <diego@xxxxxxxxxx>
- To: hipl-dev@xxxxxxxxxxxxx
- Date: Mon, 13 Feb 2012 10:25:22 +0100
On Mon, Feb 13, 2012 at 10:39:38AM +0200, Xin Gu wrote:
> 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.
No.
> >>>>--- 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.
If those moves make sense at all, then they make sense outside of this
merge request and should be submitted separately.
> >>>>@@ -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?
More stupidity from my side, ignore ...
> 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?
Yes of course - no configuration must be broken by your merge.
Diego
Other related posts:
