Bug #6213
closed
Use osmo_io in gsmtap_util.c
Added by daniel 8 months ago.
Updated 6 months ago.
Description
gsmtap logging/L1/L2 can generate quite a number of messages. It mostly sends on the socket and is pretty self-contained so it's a good candidate to port to osmo_io.
Porting is pretty straight forward, just needs adjusting gsmtap_send(), gsmtap_source_init2(), gsmtap_source_free() to use osmo_io instead of osmo_wqueue. One small issue is that right the user can opt in to using wqueues in gsmtap_source_init2() through the ofd_wq_mode parameter. Grepping through the osmo* checkouts I can see that osmo-remsim bankd and osmocom-bb host/gprsdecode seem to use the direct mode (setting ofd_wq_mode to 0). This sends the messages directly and I'm not sure if we actually need to keep supporting this (laforge ?).
Even if we do we can simply make a union of int fd and struct osmo_io_fd and decide based on (ofd_)wq_mode just like before.
The non-wqueue mode is primarily intended for applications that don't use osmo_select_main and hence require a direct write to the socket. It's fine to keep this mode only in non-osmo-io.
- Status changed from New to Feedback
- % Done changed from 0 to 10
Hi,
I'm wondering what would be the best way to replace usage of osmo write queues with osmo_io.
I'm seeing that libosmocore.git:include/osmocom/core/gsmtap_util.h contains struct gsmtap_inst not just as a complete declaration (vs. forward declaration), so the struct members are part of the API and hence may not be removed afaiu.
It appears that current (Osmocom) projects aren't directly accessing any field members of struct gsmtap_inst (feel free to correct me on that).
The current struct decl. looks like this:
libosmocore.git:include/osmocom/core/gsmtap_util.h:
struct gsmtap_inst {
int ofd_wq_mode; /*!< wait queue mode? */
struct osmo_wqueue wq; /*!< the wait queue */
struct osmo_fd sink_ofd;/*!< file descriptor */
};
Options I have thought of are:
- Submit a first patch to introduce `gsmtap_util_internal.h` with above declaration, turn the existing declaration into a forward declaration. Then change the backend(ed) declaration.
Advantages: We probably don't want such a struct definition exposed, anyways, and we get a proper API/backend separation. No legacy backend code has to be maintained.
Disadvantages: Additional header file for just that struct decl.. Compilation breaks if we have automatically allocated structs (haven't seen any so far, though, it seems to always be 'struct `gsmtap_inst *foo;`)
- Change the existing declaration to something like
struct gsmtap_inst {
int ofd_wq_mode; /*!< wait queue mode? */
union {
struct osmo_wqueue wq; /*!< the wait queue */
struct osmo_io_fd *io_fd; /*!< Osmo IO fd for wait queue mode */
}
union {
struct osmo_fd sink_ofd; /*!< file descriptor */
int sink_fd; /*!< file descriptor */
}
};
Advantages: Avoids breaking any compilations.
Disadvantages: Readability. And we're keeping unused struct members which we probably don't have to keep anyways, because directly accessing them isn't motivated anywhere in the API.
- Like the previous point, but just redefine the struct members completely (if the API is used as previously, afaiu, things won't break anyways).
- Add a union
gsmtap_inst2, as an anonymous union of struct gsmtap_inst and struct gsmtap_inst_osmo_io that is adapted as necessary.
Advantages: The union pointer is 'strict aliasing'-compatible with the previous datatype. So we can pass it just the same and do whatever we want on the backend side.
Disadvantages: We're again keeping unused struct members which we probably don't have to keep anyways, because directly accessing them isn't motivated anywhere in the API. We also have to distinguish somehow what member of the union to use (do we even want that...).
First thing to have into account:
The struct gsmtap_inst has its own library-provided API to allocate the struct, aka gsmtap_source_init() and gsmtap_source_init2(), and gsmtap_source_free(). Hence, adding new fields to the end of the struct should be fine.
Given the existing APIs and the expected way to use the struct, I'd foresee indeed no app is using the struct fields directly. Quick check is to build libosmocore moving the struct to gsmtap_util.c and rebuild all osmocom apps.
Then, if no app is using it, I'd be OK with making the struct definition private, and change it to work with osmo_io only, something like:
struct gsmtap_inst {
int ofd_wq_mode; /*!< wait queue mode? */
struct osmo_io_fd *io_fd; /*!< Osmo IO fd for wait queue mode */
int sink_fd; /*!< file descriptor */
};
Making the struct private would indeed be the nicest solution. Just checking if laforge has any objections since it is technically a breaking change?
On Tue, Oct 10, 2023 at 12:41:56PM +0000, daniel wrote:
Making the struct private would indeed be the nicest solution. Just checking if laforge has any objections since it is technically a breaking change?
No objections if we have no known osmocom code that accesses the struct internals.
One could first merge a patch with making the struct private and see if there's any fall-out
- % Done changed from 10 to 70
- % Done changed from 70 to 80
- Related to Bug #6260: GSMTAP sending fails with ENOSPC added
- Status changed from Feedback to Resolved
- % Done changed from 80 to 100
Also available in: Atom
PDF
