Attention is currently required from: osmith, Hoernchen, neels, laforge, fixeria, daniel.

View Change

26 comments:

• File include/osmocom/core/osmo_io.h:

  • Patch Set #3, Line 49: void osmo_iofd_read_enable(struct osmo_io_fd *iofd);

    duplicated declaration? one probably should be "disable"

  • Patch Set #3, Line 55: unsigned int osmo_iofd_get_priv(struct osmo_io_fd *iofd);

    const iofd

  • Patch Set #3, Line 56: int osmo_iofd_get_fd(struct osmo_io_fd *iofd);

    const iofd

• File src/osmo_io.c:

  • Patch Set #3, Line 44: extern struct iofd_backend_ops iofd_poll_ops;

    we have an internal header, maybe move this there?

  • Patch Set #3, Line 47: static enum osmo_io_backend g_backend;

    g_io_backend?

  • Patch Set #3, Line 50: struct iofd_backend_ops g_iofd_ops;

    then better name it with an osmo_ prefix.

  • Patch Set #3, Line 111: talloc_free(msghdr);

    the ownserhip of msghdr->msg is not really clear here. If a msg ptr is passed in iofd_msghdr_alloc, is it expected to pass ownsership of msg to msghdr? If so, either:

    • call talloc_steal in iofd_msghdr_alloc
    • explicitly talloc_free msghdr->msg here.

  • Patch Set #3, Line 203: e

    no doxygen docs, but function is exported

    Ack

  • Patch Set #3, Line 225: msg_pending = iofd_msgb_alloc(iofd);

    I wonder why do we need to allocate + copy to a new msgb here.

  • Patch Set #3, Line 228: iofd->pending = msg_pending;

    iofd->pending may already be set here? in that case may leak the previous iofd->pending?

  • Patch Set #3, Line 299: int osmo_iofd_sendto_msgb(struct osmo_io_fd *iofd, struct msgb *msg, const struct osmo_sockaddr *dest)

    we may be missing "int flags" here?

  • Patch Set #3, Line 367: iofd->name = talloc_strdup(iofd, name);

    This can be split into a separate func osmo_iofd_set_name(), simplifying the current osmo_iofd_setup param list. Some apps may not even care nor want to spend cycles allocating memory and setting names.

  • Patch Set #3, Line 389: g_backend == OSMO_IO_BACKEND_POLL ? "poll" : "uring"

    there might be more in the future, it might be worth having a separate value_string for that rather […]

    Ack

  • Patch Set #3, Line 408: if (iofd->pending)

    no need for this if, must probably (free function)

  • Patch Set #3, Line 414: talloc_free(iofd);

    you still want to free the memory if close somehow fails, otherwise you'll leak it?

  • Patch Set #3, Line 430: osmo_iofd_get_priv

    please rename to osmo_iofd_get_priv_nr(). […]

    Ack

• File src/osmo_io_internal.h:

  • Patch Set #3, Line 72:

    struct {
    			bool read_enabled;
    			bool read_pending;
    			bool write_pending;
    			bool write_enabled;
    			/* TODO: index into array of registered fd's? */
    		} uring;
    

    should this maybe introduced only in the actual io_oring patch?

    Ack

  • Patch Set #3, Line 87: // TODO: SCTP_*

    I'd say this needs to be added before merging the patch? or not needed?

• File src/osmo_io_poll.c:

  • Patch Set #3, Line 63: if (iofd->closing) {

    Remove {}

  • Patch Set #3, Line 71: rc = write(ofd->fd, msgb_data(msg), msgb_length(msg));

    this should be send() in order to pass iofd->flags to it.
    Actually, you should have aseparate read_flags and write_flags fields most probably.

  • Patch Set #3, Line 72: if (rc < msgb_length(msg)) {

    if write returns error (rc<0), you'll be doing pretty weird stuff here.

  • Patch Set #3, Line 104: socklen_t addrlen = sizeof(struct sockaddr);

    This is wrong afaict. It should be struct sockaddr_strorage.

    No need for sa, You can simply do:
     socklen_t addrlen = sizeof(saddr.u.ss);
     rc = recvfrom(ofd->fd, msgb_data(msg), msgb_tailroom(msg), 0,
    			      &saddr.u.sa, &addrlen);

  • Patch Set #3, Line 114:

    extraneous empty line

    Ack

  • Patch Set #3, Line 117: {

    coding style

    Ack

  • Patch Set #3, Line 124: struct osmo_sockaddr *dest = &msghdr->sa;

    Usually we use "osa" to name an osmo_sockaddr, to differentiate it from "sa" (sockaddr). Please update the field name in msghdr.

  • Patch Set #3, Line 128: &dest->u.sa, sizeof(*dest));

    not sure if sizeof(sockaddr_storage) is good here, you may have to pass dest.u.sin or dest.u.sin6 depending on the dest.u.sa.sa_family?

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