Attention is currently required from: msuraev.

View Change

9 comments:

• Patchset:

  • Patch Set #8:

    I'm sorry that I'm late for this patch review.
    This patch has a number of rough edges,
    I would welcome if we could revert it and get some more review cycles.

• File src/sockaddr_str.c:

  • Patch Set #8, Line 547: * buf_len >= 2: (hostA|hostB|...|...)

    buf_len is the buffer size of the output string buffer *buf.
    A buf_len == 0 cannot return any "()" string, it has zero size.
    buf_len == 1 cannot contain "hostA".

    Did you mean sa_str_count?

  • Patch Set #8, Line 555: int osmo_sockaddr_strs_to_str(char *buf, size_t buf_len, const struct osmo_sockaddr_str **sa_str, size_t sa_str_count)

    i find it odd that you want to pass an array of osmo_sockaddr_str POINTERS,
    instead of an array of osmo_sockaddr_str instances.
    i.e. i would expect an arg like 'const struct osmo_sockaddr_str *sa_strs',
    not '**'.

  • Patch Set #8, Line 562: return -ENOMEM;

    this check should not be here. The idea is to always return the nr of characters needed to output the full string, regardless of the underlying buffer length. If you return -ENOMEM on some specific buffer lengths, you destroy the ability of finding a sufficient buffer length from the return value. The OSMO_STRBUF macros deal with all cases of insufficient buffer length, just let it play through to the end, so that the proper sb.chars_needed is returned.

  • Patch Set #8, Line 567: for (i = 0; i < sa_str_count; i++) {

    for (...) {
         if (i)
           OSMO_STRBUF_PRINTF(sb, "|");
         if (sa_str[i])
           OSMO_STRBUF_PRINTF(sb, OSMO_SOCKADDR_STR_NO_PORT_FMT,
                              OSMO_SOCKADDR_STR_NO_PORT_FMT_ARGS(sa_str[i]));
      }
      if (sa_str_count != 1)
         OSMO_STRBUF_PRINTF(sb, ")");

• File tests/sockaddr_str/sockaddr_str_test.c:

  • Patch Set #8, Line 28: #include <osmocom/core/logging.h>

    why add application and logging?
    there should be no select loop.
    nothing about osmo_sockaddr_str should ever cause osmocom logging.

  • Patch Set #8, Line 268: struct osmo_sockaddr_str *sa_strs[OSMO_SOCK_MAX_ADDRS];

    (...and here just a struct osmo_sockaddr_str sa_strs[N], no need to talloc instances below)

  • Patch Set #8, Line 271: for (i = 0, j = 0; j < count && i < OSMO_SOCK_MAX_ADDRS; j++) {//ARRAY_SIZE(oip_data)

    can we drop the // comments?

  • Patch Set #8, Line 290: };

    why add logging infrastructure??

To view, visit change 28992. To unsubscribe, or for help writing mail filters, visit settings.