Attention is currently required from: osmith, Hoernchen, neels, laforge, pespin, fixeria.
33 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"
Ack
Patch Set #3, Line 55: unsigned int osmo_iofd_get_priv(struct osmo_io_fd *iofd);
const iofd
Done
Patch Set #3, Line 56: int osmo_iofd_get_fd(struct osmo_io_fd *iofd);
const iofd
Done
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?
Ack
Patch Set #3, Line 47: static enum osmo_io_backend g_backend;
g_io_backend?
Ack
Patch Set #3, Line 85: * \returns the newly allocated msghdr or NULL in case of error */
I suggest printing an error message and exit(1) here, since this may be the result of an unexpected […]
Ack
Patch Set #3, Line 121: iofd->msgb_alloc.size + headroom, headroom, iofd->name);
the ownserhip of msghdr->msg is not really clear here. […]
For io_uring the msghdr will also be used for read requests and then the msg might continue to live in the read_cb after the msghdr has already been freed.
always
Done
Ack
It can be static
Patch Set #3, Line 224: superfluous
superflous IMHO means that it's not needed. […]
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.
This path is chosen if we receive more than one message. The original msgb is passed on to the recv callback (where it will be freed) and the pending data is stored in a new msgb.
Patch Set #3, Line 230: /* Trim the original msgb to size */
iofd->pending may already be set here? in that case may leak the previous iofd->pending?
pending should always be NULL here because either iofd_msgb_pending() of iofd_msgb_pending_or_alloc() has been called before.
I'll see if I can restructure the code to make it clearer, like passing msgbs in and out here and setting iofd->pending in the functions that call this one.
coding style
Done
coding style
Done
coding style
Done
coding style
Done
Patch Set #3, Line 360: t osmo_io_fd);
given that there's only one known legacy user of the priv_nr (libosmo-abis), maybe we remove this fr […]
Ack
smo_io_fd);
if (!iofd
we have three integer argiuments after each other, and lots of arguments, which makes it easy to hav […]
I guess we could even have defaults for msgb size/headroom and have osmo_iofd_set_alloc_info() available to set it manually?
Ack
Done
c++ style comment
Done
no need for this if, must probably (free function)
Ack
you still want to free the memory if close somehow fails, otherwise you'll leak it?
The close callback return value indicates whether the free() should be deferred or not, it does not indicate failure. E.g. if a user calls osmo_iofd_close() in the write callback the close callback will only set a flag and the iofd will be freed at the end.
Thinking about this osmo_iofd_close() should probably return the value from close()
Patch Set #3, Line 432: >priv_nr;
Ack
I also added set_data() in case some user needs to set it late or change it
File src/osmo_io_internal.h:
Patch Set #3, Line 87: // TODO: SCTP_*
I'd say this needs to be added before merging the patch? or not needed?
I wanted to keep this patch as-is and add sctp support in a separate one. It's not finished yet, though.
File src/osmo_io_poll.c:
might be just me, but I find "flags" much more descriptive than "what"
In socket.c it's called what in the callback, when in the ofd and flags in the internal code.
I'd keep the name to match what osmo_fd users expect.
Patch Set #3, Line 63: if (iofd->closing) {
Remove {}
Done
Patch Set #3, Line 75: return;
this should be send() in order to pass iofd->flags to it. […]
There is no iofd->flags, did you mean msghdr->flags?
I fact I could simply use sendmsg for everything and have wrappers around that. (write vs sendto) That might reduce duplication and we're already using struct msghdr anyway
if write returns error (rc<0), you'll be doing pretty weird stuff here.
Ack
Patch Set #3, Line 108: if (rc > 0)
This is wrong afaict. It should be struct sockaddr_strorage. […]
I'd use sizeof(saddr) since that most accurately describes the size recvfrom is allowed to write to.
extraneous empty line
Done
coding style
Done
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). […]
Ack
should be static? same as all of the iofd_poll_ functions which are exposed as callback via iofd_pol […]
Done
To view, visit change 30934. To unsubscribe, or for help writing mail filters, visit settings.