Attention is currently required from: pespin.

View Change

5 comments:

• File src/core/socket.c:

  • Patch Set #1, Line 1968: return -EBADF;

    i understand, but for the snprintf() like function signature, if at all possible you should always return sb.chars_needed. Otherwise, if anyone calls OSMO_STRBUF_APPEND(sb, osmo_sock_multiaddr_get_name_buf, ...), they don't get "<error-bad-fd>" returned, in the expected place; instead, it looks like OSMO_STRBUF_APPEND() might even crash? [the place handling < 0 there looks a bit weird; same as insufficient buffer, which should be a different case.]

    the function's purpose is to print a string, not to validate an fd, so better just do this here:

      OSMO_STRBUF_PRINTF(sb, "<error-bad-fd>");
      return sb.chars_needed;

    because then the caller will get the error output in the right place, like, contrived example:

      Stats:
      reads: 0
      writes: 0
      address: <error-bad-fd>

    When you return an error, the string composition may abort and omit the "Stats:..." part too in the resulting output.

    (as i read on, i see that osmo_sock_get_name_buf() makes the same mistake, i guess i need to fix that)

  • Patch Set #1, Line 1993: v6 = !!s

    assigning a value to is_v6 in an 'else' clause and using is_v6 below. It looks like the implication is that if num_hostbuf <= 1, then it cannot be a v6 address, which sounds odd to me

  • Patch Set #1, Line 2005: OSMO_STRBUF_PRINTF(sb, "<need-more-bufs!>");

    (could print this above instead of adding the need_more_bufs local var, just my opinion)

    (also i find it weird to print <need-more-bufs!> in user output, it makes no sense to the reader, because it is reporting an error in internal buffer sizes, and not about the user's config or current status of resources; a user may think that a RAM upgrade is necessary to run osmocom, wildly wrong)

  • Patch Set #1, Line 2019: bool is_v6 = false;

    (can this code dup be collapsed with the above, maybe with a static func?)

  • Patch Set #1, Line 2052: int osmo_sock_get_name_buf(char *str, size_t str_len, int fd)

    Something which we could do is internally integrating the multiaddr support into the existing genera […]

    my first thought is "yes, sounds good".

    Do you think though there may be situations where we don't want all of the addresses in, say, all of the logging or vty output all the time, where instead we want to just continue printing the first address?
    I guess that is hard to answer, so unfortunately the best backwards compatible way is to only have the separate new API returning multiple addrs. Callers can switch to the new function at their own time.

    But, hey, how about we call the new function osmo_sock_get_name_buf2(), as in, v2 supports multiple addresses, and everyone should just use that everywhere from now on?

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