On Sat, May 12, 2012 at 10:13:17AM +0300, Xin Gu wrote:
> On 11/05/12 00:55, Diego Biurrun wrote:
> >>@@ -202,6 +166,47 @@
> >>
> >>+lib_hipl_libhipl_la_SOURCES = lib/hipl/accessor.c                         \
> >>+                              lib/hipl/lhipl.c                            \
> >>+                              lib/hipl/lhipl_sock.c                       \
> >>+                              lib/hipl/lhipl_operations.c                 \
> >Why this lhipl prefix?
> 
> It is a short form for libhipl. This prefix can help to identify src
> code files related to libhipl.

There is no need to identify files in the libhipl directory as belonging
to libhipl.  What else would they belong to?  We don't have hipfw_ and
hipd_ prefixes in the hipd/ and hipfw/ directories.

> >>--- lib/core/hostid.c	2012-03-01 14:26:43 +0000
> >>+++ lib/core/hostid.c	2012-04-17 16:09:27 +0000
> >>@@ -743,11 +745,16 @@
> >>                  goto out_err;
> >>              }
> >>          } else if (!use_default) {
> >>+            hi_file_dup = strdup(hi_file);
> >>+            if ((err = check_and_create_dir(dirname(hi_file_dup), HIP_DIR_MODE))) {
> >>+                HIP_ERROR("Could not create directory for path: %s\n", hi_file);
> >>+                goto out_err;
> >>+            }
> >>@@ -1093,6 +1100,7 @@
> >>
> >>+    free(hi_file_dup);
> >>      free(dsa_host_id);
> >>      free(dsa_pub_host_id);
> >>      free(rsa_host_id);
> >This string duplication is not necessary AFAICT.
> 
> strdup() is necessary before using dirname(), or you are suggesting
> other ways? can you clarify?

Constructing the path on the fly does not seem more complicated than
strdup() and free(), but just do whatever you want.

> >>+int hipl_sendto(const hipl_sock_id hsock_id, const void *const msg,
> >>+                const size_t len, const int flags,
> >>+                const char *const peername, uint16_t port)
> >>+{
> >>+
> >>+    if (hipl_lib_bex_feedback()) {
> >>+        err = hipl_sendmsg_internal(hsock,&params, flags);
> >>+    } else {
> >>+        fd_set fdset;
> >>+        if (hipl_hsock_ha_state(hsock) == HIP_STATE_UNASSOCIATED) {
> >>+            HIP_DEBUG("Sending via hsock %d, Triggering BEX.\n", hsock->sid);
> >>+            err = hipl_sendmsg_internal(hsock,&params, flags);
> >>+            HIP_IFEL(err != -EWAITBEX, -1, "hipl_sendmsg_internal() failed\n");
> >>+        }
> >>+        if (hipl_hsock_ha_state(hsock) == HIP_STATE_ESTABLISHED) {
> >>+            HIP_DEBUG("Sending via hsock %d, HA established.\n", hsock->sid);
> >>+            err = hipl_sendmsg_internal(hsock,&params, flags);
> >>+        } else {
> >>+            while (hipl_hsock_ha_state(hsock) != HIP_STATE_ESTABLISHED) {
> >>+                FD_ZERO(&fdset);
> >>+                FD_SET(hsock->sock_fd,&fdset);
> >>+                HIP_DEBUG("Sending via hsock %d, Waiting BEX.\n", hsock->sid);
> >>+                err = select(hsock->sock_fd + 1,&fdset, NULL, NULL, NULL);
> >>+                HIP_IFEL(err<  0, -1, "select(): %s\n", strerror(errno));
> >>+                err = hipl_sendmsg_internal(hsock,&params, flags);
> >>+                HIP_IFEL(err<  0&&  err != -EWAITBEX&&  err != -EBEXESTABLISHED,
> >>+                         -1, "hipl_sendmsg_internal() failed\n");
> >>+            }
> >>+            err = hipl_sendmsg_internal(hsock,&params, flags);
> >>+        }
> >>+    }
> >>+
> >>+out_err:
> >>+    free(buf);
> >>+    return err;
> >>+}
> >HIP_IFEL ugliness
> >
> 
> I think I followed the guideline of HIP_IFEL, I use it here because
> I need to free memory at the end.

I maintain that there is never any good reason to use HIP_IFEL in new code.

Diego