Some fixes #1941

sjaeckel · 2023-12-09T14:11:10Z

Check commit messages for details.

Signed-off-by: Steffen Jaeckel <jaeckel-floss@eyet-services.de>

jubalh

All looks fine, the only thing I don't understand is Fix libstrophe verbosity level always being set to 0 on start . What exactly is being fixed here?
Do you remove the line because we already require a later libstrophe version?

H3rnand3zzz

Thank you for the PR, generally nice, though I would like to see more readability and consistency in changes. I tried to read the code without commit message and it was a bit hard, which can make further maintenance a bit harder.

H3rnand3zzz · 2023-12-11T08:09:52Z

src/xmpp/iq.c

+};
+
+static void
+_win_find(char* key,


it appears that the name mismatches the purpose, maybe rather something like _mam_win_match_add?

src/xmpp/iq.c

H3rnand3zzz · 2023-12-11T08:16:28Z

src/xmpp/iq.c

+                                                        cur);
+        cur = next;
+    }
+    struct iq_win_finder st = { 0 };


undescriptive variable name, something like matcher_data would be good enough.

Also another suggestion: make a blank space between } and struct, since these are 2 different logical parts (1 part to remove window, other to remove iq handlers, maybe these should even be separated on 2 functions).

Generally, it's a bit strange approach to make your own dynamic array and structure just for this, why not something like this for readability? (let's call it pseudocode since it's not tested)

static void _mam_win_match_add(char* key, ProfIqHandler* handler, GArray* to_be_removed) { if (handler->func == _mam_rsm_id_handler) { // Append the element to the array g_array_append_val(to_be_removed, g_strdup(key)); } } GArray* to_be_removed = g_array_new(FALSE, FALSE, sizeof(char*)); // Iterate through id_handlers and call _mam_win_match_add for each entry to identify windows to be removed. g_hash_table_foreach(id_handlers, (GHFunc)_mam_win_match_add, to_be_removed); // Access elements using g_array_index and free each element for (guint n = 0; n < to_be_removed->len; ++n) { char* to_remove = g_array_index(to_be_removed, char*, n); g_hash_table_remove(id_handlers, to_remove); } g_array_free(to_be_removed, TRUE);

H3rnand3zzz · 2023-12-11T08:21:31Z

src/xmpp/message.c

@@ -1261,7 +1261,7 @@ _handle_muc_private_message(xmpp_stanza_t* const stanza)
    ProfMessage* message = message_init();
    message->type = PROF_MSG_TYPE_MUCPM;

-    const gchar* from = xmpp_stanza_get_from(stanza);
+    const char* from = xmpp_stanza_get_from(stanza);


There are few more cases, I suggest to catch all of them, using regex: gchar.+xmpp_

Feel free to open another PR with those changes.

H3rnand3zzz · 2023-12-11T08:31:32Z

src/xmpp/iq.c

@@ -267,8 +268,56 @@ iq_handlers_init(void)
    rooms_cache = g_hash_table_new_full(g_str_hash, g_str_equal, free, (GDestroyNotify)xmpp_stanza_release);
 }

+struct iq_win_finder
+{
+    gsize max, cur;


max can be more descriptive? E.g. max_size

sjaeckel · 2023-12-12T14:44:06Z

All looks fine, the only thing I don't understand is Fix libstrophe verbosity level always being set to 0 on start . What exactly is being fixed here? Do you remove the line because we already require a later libstrophe version?

The reason for that line was the missing init of the verbosity variable in an earlier release of libstrophe. 1. This has been fixed since and 2. we set the verbosity level now as well in connection_init() from the profanity preference that was added later. This line was simply forgotten to be removed when the preference was introduced.

jubalh · 2023-12-12T15:23:18Z

The reason for that line was the missing init of the verbosity variable in an earlier release of libstrophe. 1. This has been fixed since and 2. we set the verbosity level now as well in connection_init() from the profanity preference that was added later. This line was simply forgotten to be removed when the preference was introduced.

Can you add this to the commit message? Would make it easier to understand later :)

Fixup/revert of e55f6d7 This line had been added because an earlier release of libstrophe missed the initialisation of that variable, which has since been fixed. Signed-off-by: Steffen Jaeckel <jaeckel-floss@eyet-services.de>

Signed-off-by: Steffen Jaeckel <jaeckel-floss@eyet-services.de>

Before this patch the following scenario lead to a segfault: 1. open a window that sends a MAM request 2. fast enough close that window again before the MAM response was processed Once the MAM response is received we'd call `_mam_rsm_id_handler()` from the `_iq_handler()` and `window` would point to a non-existant window which leads to a segfault. Signed-off-by: Steffen Jaeckel <jaeckel-floss@eyet-services.de>

Reset the `received_disco_items` flag when initializing the iq module. This has caused the console error message "Server doesn't support MAM" sometimes on reconnect. Fixes #1940 Signed-off-by: Steffen Jaeckel <jaeckel-floss@eyet-services.de>

1. close logfile as last action 2. Fix `plugins_shutdown()` accessing `((ProfPlugin*)curr->data)->lang` after `curr->data` had already potentially been free'd. Signed-off-by: Steffen Jaeckel <jaeckel-floss@eyet-services.de>

sjaeckel

Thanks for your suggestions.

As they're mere opinions and don't really fix anything, I suggest to merge as is.

src/xmpp/iq.c

sjaeckel · 2023-12-12T10:39:49Z

src/xmpp/message.c

@@ -1261,7 +1261,7 @@ _handle_muc_private_message(xmpp_stanza_t* const stanza)
    ProfMessage* message = message_init();
    message->type = PROF_MSG_TYPE_MUCPM;

-    const gchar* from = xmpp_stanza_get_from(stanza);
+    const char* from = xmpp_stanza_get_from(stanza);


Feel free to open another PR with those changes.

Fix some things regarding char* vs. gchar*

64046bc

Signed-off-by: Steffen Jaeckel <jaeckel-floss@eyet-services.de>

sjaeckel force-pushed the some-fixes branch from 446f6e6 to e4d224d Compare December 9, 2023 14:13

sjaeckel linked an issue Dec 9, 2023 that may be closed by this pull request

Wrong error about server not supporting MAM. #1940

Closed

jubalh reviewed Dec 10, 2023

View reviewed changes

jubalh assigned sjaeckel Dec 10, 2023

jubalh added improvement cleanup labels Dec 10, 2023

jubalh added this to the next milestone Dec 10, 2023

H3rnand3zzz suggested changes Dec 11, 2023

View reviewed changes

sjaeckel added 5 commits December 12, 2023 16:30

Save SM queue on /reconnect now

2b51675

Signed-off-by: Steffen Jaeckel <jaeckel-floss@eyet-services.de>

Reset internal flag

629cd33

Reset the `received_disco_items` flag when initializing the iq module. This has caused the console error message "Server doesn't support MAM" sometimes on reconnect. Fixes #1940 Signed-off-by: Steffen Jaeckel <jaeckel-floss@eyet-services.de>

Improve shutdown

31c6d5f

1. close logfile as last action 2. Fix `plugins_shutdown()` accessing `((ProfPlugin*)curr->data)->lang` after `curr->data` had already potentially been free'd. Signed-off-by: Steffen Jaeckel <jaeckel-floss@eyet-services.de>

sjaeckel force-pushed the some-fixes branch from e4d224d to 31c6d5f Compare December 12, 2023 15:31

sjaeckel commented Dec 12, 2023

View reviewed changes

jubalh approved these changes Dec 12, 2023

View reviewed changes

jubalh merged commit b18b6cb into master Dec 12, 2023
6 checks passed

jubalh deleted the some-fixes branch December 12, 2023 15:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Some fixes #1941

Some fixes #1941

sjaeckel commented Dec 9, 2023

jubalh left a comment

H3rnand3zzz left a comment

H3rnand3zzz Dec 11, 2023

H3rnand3zzz Dec 11, 2023

H3rnand3zzz Dec 11, 2023

H3rnand3zzz Dec 11, 2023

H3rnand3zzz Dec 11, 2023

sjaeckel Dec 12, 2023

H3rnand3zzz Dec 11, 2023

sjaeckel commented Dec 12, 2023

jubalh commented Dec 12, 2023

sjaeckel left a comment

sjaeckel Dec 12, 2023

Some fixes #1941

Some fixes #1941

Conversation

sjaeckel commented Dec 9, 2023

jubalh left a comment

Choose a reason for hiding this comment

H3rnand3zzz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sjaeckel commented Dec 12, 2023

jubalh commented Dec 12, 2023

sjaeckel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment