osmocom
/
libosmocore
mirror of https://gerrit.osmocom.org/libosmocore
11
0
Fork 1
Code Issues Releases Wiki Activity
libosmocore/1e977e90302919fccbacace5cf1...

198 lines
8.7 KiB
Plaintext
Raw Blame History

{
"comments": [
{
"unresolved": true,
"key": {
"uuid": "4642b068_2e53601d",
"filename": "/COMMIT_MSG",
"patchSetId": 4
},
"lineNbr": 11,
"author": {
"id": 1000005
},
"writtenOn": "2023-12-01T02:04:12Z",
"side": 1,
"message": "AFAIU, iofd-\u003emode should distinguish which union members are in use, so it should be possible to handle things properly .. where exactly is the bug this is fixing? is the mode switched without NULLing the cb pointers or something in that way?",
"revId": "1e977e90302919fccbacace5cf155f9c75e06c00",
"serverId": "035e6965-6537-41bd-912c-053f3cf69326"
},
{
"unresolved": true,
"key": {
"uuid": "572dad2d_287fe761",
"filename": "/COMMIT_MSG",
"patchSetId": 4
},
"lineNbr": 11,
"author": {
"id": 1000004
},
"writtenOn": "2024-03-04T19:14:57Z",
"side": 1,
"message": "it is not possible to do what you are asking. the various structs in th union all alias to the exact same address. So whether I look at read_cb or recvfrom_cb or recvmsg_cb, they all are at the exact same address in memory. Hence the reason for this patch 😊",
"parentUuid": "4642b068_2e53601d",
"revId": "1e977e90302919fccbacace5cf155f9c75e06c00",
"serverId": "035e6965-6537-41bd-912c-053f3cf69326"
},
{
"unresolved": false,
"key": {
"uuid": "6d7c66be_ce3b62bc",
"filename": "/PATCHSET_LEVEL",
"patchSetId": 4
},
"lineNbr": 0,
"author": {
"id": 1000074
},
"writtenOn": "2023-11-29T18:31:40Z",
"side": 1,
"message": "I\u0027d actually keep the union, imho there\u0027s nothing wrong with it if used, plus:\n- Doesn\u0027t break ABI with previous libosmocore release\n- It saves some bytes on each osmo_io (there can be a big number of them in an app).\n\nI agree though that the comment you mentioned should be fixed by checking the correct io_mode is in used before checking the pointer, since it\u0027s shared with other io_modes through the union.\n\nTL;DR: Fix the code bugs, leave the union.\n\nIf still others prefer merging this, I won\u0027t oppose, just saying.",
"revId": "1e977e90302919fccbacace5cf155f9c75e06c00",
"serverId": "035e6965-6537-41bd-912c-053f3cf69326"
},
{
"unresolved": false,
"key": {
"uuid": "7cd2e0c8_6cd59ec3",
"filename": "/PATCHSET_LEVEL",
"patchSetId": 4
},
"lineNbr": 0,
"author": {
"id": 1000008
},
"writtenOn": "2023-11-30T13:35:04Z",
"side": 1,
"message": "Discussion has happened in the issue: https://projects.osmocom.org/issues/6263\n\nThe issue with the union is that ioops.write_cb and ioops.sendto_cb are at the same address so setting one and then checking the other does not work. In practice the code has no way to know and will assume it\u0027s calling sendto_cb() but is instead calling write_cb().",
"parentUuid": "6d7c66be_ce3b62bc",
"revId": "1e977e90302919fccbacace5cf155f9c75e06c00",
"serverId": "035e6965-6537-41bd-912c-053f3cf69326"
},
{
"unresolved": true,
"key": {
"uuid": "08864048_4580c081",
"filename": "/PATCHSET_LEVEL",
"patchSetId": 4
},
"lineNbr": 0,
"author": {
"id": 1000074
},
"writtenOn": "2023-11-30T14:51:49Z",
"side": 1,
"message": "It can know based on the OSMO_IO_FD_MODE configured during osmo_iofd_setup(). So before using one callback, the implementation needs to check ofc also the io mode is the one used. It doesn\u0027t make sense for a user of the API to set one io_mode and then set callbacks for the other mode, this is a bug in the user, wrong use of the API.\n\n\u003e it\u0027s possible to create an osmo_io_fd using osmo_iofd_setup in one mode (e.g. OSMO_IO_FD_MODE_READ_WRITE) but the register an osmo_io_ops with wrong call-backs (e.g. recvfrom/sendto).\n\nIMHO that\u0027s a bug / wrong use of the API at the user and nothing to be fixed in the osmo_io implementation.",
"parentUuid": "7cd2e0c8_6cd59ec3",
"revId": "1e977e90302919fccbacace5cf155f9c75e06c00",
"serverId": "035e6965-6537-41bd-912c-053f3cf69326"
},
{
"unresolved": true,
"key": {
"uuid": "b83c7537_3ed646cf",
"filename": "/PATCHSET_LEVEL",
"patchSetId": 4
},
"lineNbr": 0,
"author": {
"id": 1000008
},
"writtenOn": "2023-11-30T15:22:26Z",
"side": 1,
"message": "I disagree that we should put the burden of this onto the user. It\u0027s an easy enough mistake to make.\n\nThis is complicated by the fact that the only way to set (either initially or later) the callback function is by passing in a struct osmo_iofd_ops *. This means that with the union there is *no* way at all to detect this.\n\n```\nosmo_iofd_setup(iofd, ..., mode, \u0026ops); // Checking ops callbacks according to mode doesn\u0027t work\nosmo_iofd_set_ioops(iofd, \u0026ops); // Same as above, we have iofd-\u003emode, but ops is ambiguous\n```\n\nMaybe you\u0027re thinking of osmo_stream_* where we have *_set_read_cb() etc. functions?",
"parentUuid": "08864048_4580c081",
"revId": "1e977e90302919fccbacace5cf155f9c75e06c00",
"serverId": "035e6965-6537-41bd-912c-053f3cf69326"
},
{
"unresolved": true,
"key": {
"uuid": "2fff5d3c_c2432b3b",
"filename": "/PATCHSET_LEVEL",
"patchSetId": 4
},
"lineNbr": 0,
"author": {
"id": 1000074
},
"writtenOn": "2023-11-30T15:38:33Z",
"side": 1,
"message": "So you set mode (which is usually a hardcoded value in the code), and then an ops, which is also a hardcoded struct pointing to known functions, which is a union known by the user of the API.\n\nTo me it\u0027s fine. To me what you say it\u0027s like making a claim that you open an AF_INET socket and then something is wrong becuase you pass an AF_INET6 address to some API...\nI agree it would be nice to have more protection there, but current state is good enough for me, specially since changing it has the 2 drawbacks I mentioned early (ABI break, more memory needed per fd).\n\nThat\u0027s my current opinion, let\u0027s see if other have other opinions here.",
"parentUuid": "b83c7537_3ed646cf",
"revId": "1e977e90302919fccbacace5cf155f9c75e06c00",
"serverId": "035e6965-6537-41bd-912c-053f3cf69326"
},
{
"unresolved": true,
"key": {
"uuid": "b01ccde3_ad12bf0a",
"filename": "/PATCHSET_LEVEL",
"patchSetId": 4
},
"lineNbr": 0,
"author": {
"id": 1000008
},
"writtenOn": "2023-11-30T15:45:42Z",
"side": 1,
"message": "Thanks for the clarification. Maybe @laforge@gnumonks.org can join in since he was has experienced this issue.",
"parentUuid": "2fff5d3c_c2432b3b",
"revId": "1e977e90302919fccbacace5cf155f9c75e06c00",
"serverId": "035e6965-6537-41bd-912c-053f3cf69326"
},
{
"unresolved": false,
"key": {
"uuid": "18b647ff_15abd19b",
"filename": "/PATCHSET_LEVEL",
"patchSetId": 4
},
"lineNbr": 0,
"author": {
"id": 1000074
},
"writtenOn": "2023-12-01T15:12:08Z",
"side": 1,
"message": "my opinion here seems to be matching neels thought afaict. I see no need to remove the union. Simply check the mode always.",
"revId": "1e977e90302919fccbacace5cf155f9c75e06c00",
"serverId": "035e6965-6537-41bd-912c-053f3cf69326"
},
{
"unresolved": true,
"key": {
"uuid": "2d9f5f19_5048d303",
"filename": "src/core/osmo_io.c",
"patchSetId": 4
},
"lineNbr": 391,
"author": {
"id": 1000005
},
"writtenOn": "2023-12-01T02:04:12Z",
"side": 1,
"message": "this code should also work with the union present?",
"revId": "1e977e90302919fccbacace5cf155f9c75e06c00",
"serverId": "035e6965-6537-41bd-912c-053f3cf69326"
},
{
"unresolved": true,
"key": {
"uuid": "813d11cc_3e337a84",
"filename": "src/core/osmo_io.c",
"patchSetId": 4
},
"lineNbr": 391,
"author": {
"id": 1000008
},
"writtenOn": "2023-12-01T10:05:01Z",
"side": 1,
"message": "It \"works\" in the sense that it even returns true if mode \u003d\u003d *_READ_WRITE and .recvfrom_cb (or the other way around) is set since both function pointers are sharing the same memory location.",
"parentUuid": "2d9f5f19_5048d303",
"revId": "1e977e90302919fccbacace5cf155f9c75e06c00",
"serverId": "035e6965-6537-41bd-912c-053f3cf69326"
}
]
}
View Git Blame Copy Permalink