@henningw commented on this pull request.
Thank you for the update! I had a look to the usual topics, and added some
remarks about smaller things I noticed.
> + strcpy(elem->host, sentinel);
+ elem->port = port;
+ elem->next = NULL;
+
+ if(sc.sentinel_list == NULL) {
+ sc.sentinel_list = elem;
+ sc.sentinel_list_tail = elem;
+ } else {
+ sc.sentinel_list_tail->next = elem;
+ sc.sentinel_list_tail = elem;
+ }
+
+ sentinel = strtok(NULL, "|");
+ }
+
+ LM_INFO("Adding usrloc dburl sentinels:\n");
This log seems a bit odd, not sure what the relationship to usrloc is. The
actual log output is also LM_DBG, maybe it make sense to also make this one
LM_DBG?
> @@ -141,6 +170,30 @@ static int mod_init(void)
db_redis_hash_expires_str.len = snprintf(
db_redis_hash_expires_str.s, 20, "%d",
db_redis_hash_expires);
}
+
+ shared_time = shm_malloc(sizeof(time_t));
The malloc can fail, consider adding a check for NULL and SHM_MEM_ERROR or
similar. Its also not freed in error cases, but arguably it don't matter much
as the server will not start anyway. But it would be cleaner.
Actually, do we need a shm_malloc for this? Its an integer in the end.
> int mod_register(char *path, int *dlflags, void *p1, void *p2)
{
if(db_api_init() < 0)
return -1;
return 0;
}
+static void db_redis_timer(unsigned int ticks, void *param)
+{
+ if(shared_time) {
I am wondering if we need to protect the access to this shared variable with a
Kamailio lock. The timer will run indepently and then the individual process
might access when its changed.
> @@ -537,11 +604,22 @@ void *db_redis_command_argv(km_redis_con_t *con,
> redis_key_t *query)
}
LM_DBG("query has %d args\n", argc);
- redisReply *reply =
- redisCommandArgv(con->con, argc, (const char **)argv,
NULL);
- if(con->con->err != REDIS_OK) {
- LM_DBG("redis connection is gone, try reconnect. (%d:%s)\n",
- con->con->err, con->con->errstr);
+ current_time = *shared_time;
See previous comment about the locking
>
int db_redis_hash_expires = 0;
str db_redis_hash_expires_str = str_init("");
char db_redis_hash_expires_buf[20] = {0};
+time_t *shared_time = NULL;
I think we should add a db_redis prefix to this variables, as there are pretty
generic and there is a risk that they overlapping with another module.
> + * Kamailio is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301
USA
+ */
+
+#include <stdlib.h>
+
+#include "redis_sentinels.h"
+#include "db_redis_mod.h"
+
+sentinel_config_t sc;
db_redis prefix - as above
>
if(!con || !con->con) {
LM_ERR("Internal error passing null connection\n");
return -1;
}
- *reply = NULL;
- ret = redisGetReply(con->con, reply);
- if(con->con->err != REDIS_OK) {
- LM_DBG("redis connection is gone, try reconnect. (%d:%s)\n",
- con->con->err, con->con->errstr);
+ current_time = *shared_time;
locking as above
> +
+// Redis role enum
+typedef enum
+{
+ REDIS_ROLE_MASTER,
+ REDIS_ROLE_REPLICA
+} redis_role_t;
+
+typedef struct redis_sentinel
+{
+ char *host;
+ unsigned int port;
+ struct redis_sentinel *next;
+} redis_sentinel_t;
+
+typedef struct
Maybe I missed it, but I failed to find the actual usage of the "user" and
"attr" (besides helping with the parsing) fields? It is of course ok to keep it
for future use, if this was the intention.
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/4373#pullrequestreview-3349693621
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/4373/review/[email protected]>_______________________________________________
Kamailio - Development Mailing List -- [email protected]
To unsubscribe send an email to [email protected]
Important: keep the mailing list in the recipients, do not reply only to the
sender!
