Attention is currently required from: osmith, Hoernchen, neels, laforge, fixeria, daniel. pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmocore/+/30934 )
Change subject: Add osmo_io with initial poll backend ......................................................................
Patch Set 3:
(26 comments)
File include/osmocom/core/osmo_io.h:
https://gerrit.osmocom.org/c/libosmocore/+/30934/comment/8d04803d_26ccd6a7 PS3, Line 49: void osmo_iofd_read_enable(struct osmo_io_fd *iofd); duplicated declaration? one probably should be "disable"
https://gerrit.osmocom.org/c/libosmocore/+/30934/comment/a893cd75_7d281ec1 PS3, Line 55: unsigned int osmo_iofd_get_priv(struct osmo_io_fd *iofd); const iofd
https://gerrit.osmocom.org/c/libosmocore/+/30934/comment/9779c049_4723f660 PS3, Line 56: int osmo_iofd_get_fd(struct osmo_io_fd *iofd); const iofd
File src/osmo_io.c:
https://gerrit.osmocom.org/c/libosmocore/+/30934/comment/4eb23142_d4708c38 PS3, Line 44: extern struct iofd_backend_ops iofd_poll_ops; we have an internal header, maybe move this there?
https://gerrit.osmocom.org/c/libosmocore/+/30934/comment/b45183be_4c0b39cd PS3, Line 47: static enum osmo_io_backend g_backend; g_io_backend?
https://gerrit.osmocom.org/c/libosmocore/+/30934/comment/325f6a28_d6e94902 PS3, Line 50: struct iofd_backend_ops g_iofd_ops; then better name it with an osmo_ prefix.
https://gerrit.osmocom.org/c/libosmocore/+/30934/comment/5f694a6e_7c527ff6 PS3, 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.
https://gerrit.osmocom.org/c/libosmocore/+/30934/comment/c5f10141_edbf357f PS3, Line 203: e
no doxygen docs, but function is exported
Ack
https://gerrit.osmocom.org/c/libosmocore/+/30934/comment/3989be04_0f4f8bb4 PS3, Line 225: msg_pending = iofd_msgb_alloc(iofd); I wonder why do we need to allocate + copy to a new msgb here.
https://gerrit.osmocom.org/c/libosmocore/+/30934/comment/d9474b17_ea77ad96 PS3, Line 228: iofd->pending = msg_pending; iofd->pending may already be set here? in that case may leak the previous iofd->pending?
https://gerrit.osmocom.org/c/libosmocore/+/30934/comment/9cf86edf_f55914c5 PS3, 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?
https://gerrit.osmocom.org/c/libosmocore/+/30934/comment/8d0ed2a8_3583faa5 PS3, 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.
https://gerrit.osmocom.org/c/libosmocore/+/30934/comment/9dd628e7_0023c4fa PS3, 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
https://gerrit.osmocom.org/c/libosmocore/+/30934/comment/f517d93a_a320919a PS3, Line 408: if (iofd->pending) no need for this if, must probably (free function)
https://gerrit.osmocom.org/c/libosmocore/+/30934/comment/5e7e43dd_5e60bd63 PS3, Line 414: talloc_free(iofd); you still want to free the memory if close somehow fails, otherwise you'll leak it?
https://gerrit.osmocom.org/c/libosmocore/+/30934/comment/7decfc04_2f211349 PS3, Line 430: osmo_iofd_get_priv
please rename to osmo_iofd_get_priv_nr(). […]
Ack
File src/osmo_io_internal.h:
https://gerrit.osmocom.org/c/libosmocore/+/30934/comment/f501f63f_e7dff15c PS3, 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
https://gerrit.osmocom.org/c/libosmocore/+/30934/comment/7ff40994_73fcb0ba PS3, 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:
https://gerrit.osmocom.org/c/libosmocore/+/30934/comment/c4b0bb76_56537732 PS3, Line 63: if (iofd->closing) { Remove {}
https://gerrit.osmocom.org/c/libosmocore/+/30934/comment/4f2e3acc_e61c1809 PS3, 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.
https://gerrit.osmocom.org/c/libosmocore/+/30934/comment/ef75ef5d_767d6bba PS3, Line 72: if (rc < msgb_length(msg)) { if write returns error (rc<0), you'll be doing pretty weird stuff here.
https://gerrit.osmocom.org/c/libosmocore/+/30934/comment/2c843641_2b92acf7 PS3, 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);
https://gerrit.osmocom.org/c/libosmocore/+/30934/comment/0b2e6e3a_49d7b8f0 PS3, Line 114:
extraneous empty line
Ack
https://gerrit.osmocom.org/c/libosmocore/+/30934/comment/4b1467a4_feb2bfb2 PS3, Line 117: {
coding style
Ack
https://gerrit.osmocom.org/c/libosmocore/+/30934/comment/f6938d24_856ee183 PS3, 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.
https://gerrit.osmocom.org/c/libosmocore/+/30934/comment/fa350aec_7561a9b7 PS3, 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?