From adbd66699191cdf59f91e2acef1b2fa93dde9af0 Mon Sep 17 00:00:00 2001 From: Markus Makela Date: Wed, 30 Nov 2016 19:32:12 +0200 Subject: [PATCH] Change module command parameter types This commit introduces safe session references that can be handled without holding locks. This allows the safe searching of sessions with the unique ID of the session. Remove the use of raw pointers passed as strings. Change the comments of the argument types and add more details to the parsing function documentation. --- include/maxscale/modulecmd.h | 28 +++++++++++++++++------- include/maxscale/session.h | 20 +++++++++++++++++ server/core/modulecmd.c | 30 +++++++------------------- server/core/session.c | 37 ++++++++++++++++++++++++++++++++ server/core/test/testmodulecmd.c | 22 +++++-------------- 5 files changed, 90 insertions(+), 47 deletions(-) diff --git a/include/maxscale/modulecmd.h b/include/maxscale/modulecmd.h index 10f772bb8..8d9a11aef 100644 --- a/include/maxscale/modulecmd.h +++ b/include/maxscale/modulecmd.h @@ -55,14 +55,12 @@ typedef struct #define MODULECMD_ARG_NONE 0 /**< Empty argument */ #define MODULECMD_ARG_STRING 1 /**< String */ #define MODULECMD_ARG_BOOLEAN 2 /**< Boolean value */ -#define MODULECMD_ARG_SERVICE 3 /**< Service name */ -#define MODULECMD_ARG_SERVER 4 /**< Server name */ -#define MODULECMD_ARG_SESSION 5 /**< SESSION pointer in string format */ -#define MODULECMD_ARG_SESSION_PTR 6 /**< Raw SESSION pointer */ -#define MODULECMD_ARG_DCB 7 /**< DCB pointer in string format*/ -#define MODULECMD_ARG_DCB_PTR 8 /**< Raw DCB pointer*/ -#define MODULECMD_ARG_MONITOR 9 /**< Monitor name */ -#define MODULECMD_ARG_FILTER 10 /**< Filter name */ +#define MODULECMD_ARG_SERVICE 3 /**< Service */ +#define MODULECMD_ARG_SERVER 4 /**< Server */ +#define MODULECMD_ARG_SESSION 6 /**< Session */ +#define MODULECMD_ARG_DCB 8 /**< DCB */ +#define MODULECMD_ARG_MONITOR 9 /**< Monitor */ +#define MODULECMD_ARG_FILTER 10 /**< Filter */ #define MODULECMD_ARG_OUTPUT 11 /**< DCB suitable for writing results to. This should always be the first argument if the function requires an output DCB. */ @@ -157,6 +155,20 @@ const MODULECMD* modulecmd_find_command(const char *domain, const char *identifi /** * @brief Parse arguments for a command * + * The argument types expect different forms of input. + * + * | Argument type | Expected input | + * |-----------------------|-------------------| + * | MODULECMD_ARG_SERVICE | Service name | + * | MODULECMD_ARG_SERVER | Server name | + * | MODULECMD_ARG_SESSION | Session unique ID | + * | MODULECMD_ARG_MONITOR | Monitor name   | + * | MODULECMD_ARG_FILTER | Filter name | + * | MODULECMD_ARG_STRING | String | + * | MODULECMD_ARG_BOOLEAN | Boolean value | + * | MODULECMD_ARG_DCB | Raw DCB pointer | + * | MODULECMD_ARG_OUTPUT | DCB for output | + * * @param cmd Command for which the parameters are parsed * @param argc Number of arguments * @param argv Argument list in string format of size @c argc diff --git a/include/maxscale/session.h b/include/maxscale/session.h index 9dda114cf..6b5f401c7 100644 --- a/include/maxscale/session.h +++ b/include/maxscale/session.h @@ -339,4 +339,24 @@ static inline bool session_set_autocommit(SESSION* ses, bool autocommit) return prev_autocommit; } +/** + * @brief Get a session reference by ID + * + * This creates an additional reference to a session whose unique ID matches @c id. + * + * @param id Unique session ID + * @return Reference to a SESSION or NULL if the session was not found + * + * @note The caller must free the session reference by passing it to the + * @c session_put_ref function + */ +SESSION* session_get_ref(int id); + +/** + * @brief Release a session reference + * + * @param session Session reference to release + */ +void session_put_ref(SESSION *session); + MXS_END_DECLS diff --git a/server/core/modulecmd.c b/server/core/modulecmd.c index 018f7bb97..d9146563a 100644 --- a/server/core/modulecmd.c +++ b/server/core/modulecmd.c @@ -277,25 +277,15 @@ static bool process_argument(modulecmd_arg_type_t *type, const void* value, break; case MODULECMD_ARG_SESSION: - arg->type.type = MODULECMD_ARG_SESSION; - arg->value.session = (SESSION*)strtol((char*)value, NULL, 0); - rval = true; - break; - - case MODULECMD_ARG_SESSION_PTR: - arg->type.type = MODULECMD_ARG_SESSION_PTR; - arg->value.session = (SESSION*)value; + if ((arg->value.session = session_get_ref(atoi(value)))) + { + arg->type.type = MODULECMD_ARG_SESSION; + } rval = true; break; case MODULECMD_ARG_DCB: arg->type.type = MODULECMD_ARG_DCB; - arg->value.dcb = (DCB*)strtol((char*)value, NULL, 0); - rval = true; - break; - - case MODULECMD_ARG_DCB_PTR: - arg->type.type = MODULECMD_ARG_DCB_PTR; arg->value.dcb = (DCB*)value; rval = true; break; @@ -373,6 +363,10 @@ static void free_argument(struct arg_node *arg) MXS_FREE(arg->value.string); break; + case MODULECMD_ARG_SESSION: + session_put_ref(arg->value.session); + break; + default: break; } @@ -625,18 +619,10 @@ char* modulecmd_argtype_to_str(modulecmd_arg_type_t *type) strtype = "SESSION"; break; - case MODULECMD_ARG_SESSION_PTR: - strtype = "SESSION_PTR"; - break; - case MODULECMD_ARG_DCB: strtype = "DCB"; break; - case MODULECMD_ARG_DCB_PTR: - strtype = "DCB_PTR"; - break; - case MODULECMD_ARG_MONITOR: strtype = "MONITOR"; break; diff --git a/server/core/session.c b/server/core/session.c index 9b155c982..15fc6207b 100644 --- a/server/core/session.c +++ b/server/core/session.c @@ -915,3 +915,40 @@ const char* session_trx_state_to_string(session_trx_state_t state) MXS_ERROR("Unknown session_trx_state_t value: %d", (int)state); return "UNKNOWN"; } + +static bool ses_find_id(DCB *dcb, void *data) +{ + void **params = (void**)data; + SESSION **ses = (SESSION**)params[0]; + int *id = (int*)params[1]; + bool rval = true; + + if (dcb->session->ses_id == *id) + { + /** We need to increment the reference count of the session to prevent it + * from being freed while it's in use. */ + atomic_add(&dcb->session->refcount, 1); + *ses = dcb->session; + rval = false; + } + + return rval; +} + +SESSION* session_get_ref(int id) +{ + SESSION *session = NULL; + void *params[] = {&session, &id}; + + dcb_foreach(ses_find_id, params); + + return session; +} + +void session_put_ref(SESSION *session) +{ + if (session) + { + session_free(session); + } +} diff --git a/server/core/test/testmodulecmd.c b/server/core/test/testmodulecmd.c index 6b6628e2f..5b341faa8 100644 --- a/server/core/test/testmodulecmd.c +++ b/server/core/test/testmodulecmd.c @@ -305,16 +305,12 @@ int test_map() } static DCB my_dcb; -static SESSION my_session; bool ptrfn(const MODULECMD_ARG *argv) { bool rval = false; - if (argv->argc == 4 && argv->argv[0].value.dcb == &my_dcb && - argv->argv[1].value.dcb == &my_dcb && - argv->argv[2].value.session == &my_session && - argv->argv[3].value.session == &my_session) + if (argv->argc == 1 && argv->argv[0].value.dcb == &my_dcb) { rval = true; } @@ -329,26 +325,18 @@ int test_pointers() modulecmd_arg_type_t args[] = { - {MODULECMD_ARG_DCB, ""}, - {MODULECMD_ARG_DCB_PTR, ""}, - {MODULECMD_ARG_SESSION, ""}, - {MODULECMD_ARG_SESSION_PTR, ""} + {MODULECMD_ARG_DCB, ""} }; - TEST(modulecmd_register_command(ns, id, ptrfn, 4, args), "Registering a command should succeed"); + TEST(modulecmd_register_command(ns, id, ptrfn, 1, args), "Registering a command should succeed"); TEST(strlen(modulecmd_get_error()) == 0, "Error message should be empty"); const MODULECMD *cmd = modulecmd_find_command(ns, id); TEST(cmd, "The registered command should be found"); - char dcb_str[200]; - char session_str[200]; - sprintf(dcb_str, "%p", &my_dcb); - sprintf(session_str, "%p", &my_session); + const void* params[] = {&my_dcb}; - const void* params[] = {dcb_str, &my_dcb, session_str, &my_session}; - - MODULECMD_ARG *arg = modulecmd_arg_parse(cmd, 4, params); + MODULECMD_ARG *arg = modulecmd_arg_parse(cmd, 1, params); TEST(arg, "Parsing arguments should succeed"); TEST(strlen(modulecmd_get_error()) == 0, "Error message should be empty");