[dm-devel] [PATCH 4/4] libmultipath: avoid cleanup __attribute__ with cancellation points
Benjamin Marzinski
bmarzins at redhat.com
Tue Oct 11 21:53:03 UTC 2022
the cleanup __attribute__ doesn't get run when a thread is cancelled, so
it is only safe in cases where there aren't pthreads or no cancellation
points happen in the code block after the variable needs cleaning up.
Signed-off-by: Benjamin Marzinski <bmarzins at redhat.com>
---
libmultipath/configure.c | 6 +--
libmultipath/dmparser.c | 2 +-
libmultipath/generic.c | 4 +-
libmultipath/prioritizers/weightedpath.c | 6 ++-
multipathd/cli_handlers.c | 11 ++++--
multipathd/main.c | 49 ++++++++++++++----------
6 files changed, 48 insertions(+), 30 deletions(-)
diff --git a/libmultipath/configure.c b/libmultipath/configure.c
index e5249fc1..25708257 100644
--- a/libmultipath/configure.c
+++ b/libmultipath/configure.c
@@ -1075,7 +1075,7 @@ int coalesce_paths (struct vectors *vecs, vector mpvec, char *refwwid,
int ret = CP_FAIL;
int k, i, r;
int is_daemon = (cmd == CMD_NONE) ? 1 : 0;
- char *params __attribute__((cleanup(cleanup_charp))) = NULL;
+ char *params = NULL;
struct multipath * mpp;
struct path * pp1 = NULL;
struct path * pp2;
@@ -1208,6 +1208,7 @@ int coalesce_paths (struct vectors *vecs, vector mpvec, char *refwwid,
remove_map(mpp, vecs->pathvec, NULL);
continue;
}
+ pthread_cleanup_push(cleanup_free_ptr, ¶ms);
if (cmd == CMD_DRY_RUN)
mpp->action = ACT_DRY_RUN;
@@ -1216,8 +1217,7 @@ int coalesce_paths (struct vectors *vecs, vector mpvec, char *refwwid,
force_reload == FORCE_RELOAD_YES ? 1 : 0);
r = domap(mpp, params, is_daemon);
- free(params);
- params = NULL;
+ pthread_cleanup_pop(1);
if (r == DOMAP_FAIL || r == DOMAP_RETRY) {
condlog(3, "%s: domap (%u) failure "
diff --git a/libmultipath/dmparser.c b/libmultipath/dmparser.c
index 44650aaa..066a33af 100644
--- a/libmultipath/dmparser.c
+++ b/libmultipath/dmparser.c
@@ -94,8 +94,8 @@ int assemble_map(struct multipath *mp, char **params)
}
}
+ condlog(4, "%s: assembled map [%s]", mp->alias, get_strbuf_str(&buff));
*params = steal_strbuf_str(&buff);
- condlog(4, "%s: assembled map [%s]", mp->alias, *params);
r = 0;
out:
pthread_cleanup_pop(1);
diff --git a/libmultipath/generic.c b/libmultipath/generic.c
index 3e2268e6..cdee21d9 100644
--- a/libmultipath/generic.c
+++ b/libmultipath/generic.c
@@ -24,11 +24,12 @@ int generic_style(const struct gen_multipath* gm, struct strbuf *buf,
__attribute__((unused)) int verbosity)
{
struct strbuf tmp = STRBUF_INIT;
- char *alias_buf __attribute__((cleanup(cleanup_charp)));
+ char *alias_buf = NULL;
const char *wwid_buf;
int ret;
pthread_cleanup_push_cast(reset_strbuf, &tmp);
+ pthread_cleanup_push(cleanup_free_ptr, &alias_buf);
gm->ops->snprint(gm, &tmp, 'n');
alias_buf = steal_strbuf_str(&tmp);
gm->ops->snprint(gm, &tmp, 'w');
@@ -37,5 +38,6 @@ int generic_style(const struct gen_multipath* gm, struct strbuf *buf,
ret = print_strbuf(buf, "%%n %s[%%G]:%%d %%s",
strcmp(alias_buf, wwid_buf) ? "(%w) " : "");
pthread_cleanup_pop(1);
+ pthread_cleanup_pop(1);
return ret;
}
diff --git a/libmultipath/prioritizers/weightedpath.c b/libmultipath/prioritizers/weightedpath.c
index df700bf3..02d40a3d 100644
--- a/libmultipath/prioritizers/weightedpath.c
+++ b/libmultipath/prioritizers/weightedpath.c
@@ -64,7 +64,7 @@ build_wwn_path(struct path *pp, struct strbuf *buf)
int prio_path_weight(struct path *pp, char *prio_args)
{
struct strbuf path = STRBUF_INIT;
- char *arg __attribute__((cleanup(cleanup_charp))) = NULL;
+ char *arg = NULL;
char *temp, *regex, *prio;
char split_char[] = " \t";
int priority = DEFAULT_PRIORITY, path_found = 0;
@@ -77,11 +77,12 @@ int prio_path_weight(struct path *pp, char *prio_args)
arg = strdup(prio_args);
temp = arg;
+ pthread_cleanup_push(cleanup_free_ptr, &arg);
regex = get_next_string(&temp, split_char);
/* Return default priority if the argument is not parseable */
if (!regex) {
- return priority;
+ goto out;
}
pthread_cleanup_push_cast(reset_strbuf, &path);
@@ -123,6 +124,7 @@ int prio_path_weight(struct path *pp, char *prio_args)
}
}
out:
+ pthread_cleanup_pop(1);
pthread_cleanup_pop(1);
return priority;
}
diff --git a/multipathd/cli_handlers.c b/multipathd/cli_handlers.c
index ddc807a1..2d1c5cfe 100644
--- a/multipathd/cli_handlers.c
+++ b/multipathd/cli_handlers.c
@@ -812,8 +812,9 @@ cli_reload(void *v, struct strbuf *reply, void *data)
static int resize_map(struct multipath *mpp, unsigned long long size,
struct vectors * vecs)
{
- char *params __attribute__((cleanup(cleanup_charp))) = NULL;
+ char *params = NULL;
unsigned long long orig_size = mpp->size;
+ int r = 1;
mpp->size = size;
update_mpp_paths(mpp, vecs->pathvec);
@@ -823,15 +824,19 @@ static int resize_map(struct multipath *mpp, unsigned long long size,
mpp->size = orig_size;
return 1;
}
+ pthread_cleanup_push(cleanup_free_ptr, ¶ms);
mpp->action = ACT_RESIZE;
mpp->force_udev_reload = 1;
if (domap(mpp, params, 1) == DOMAP_FAIL) {
condlog(0, "%s: failed to resize map : %s", mpp->alias,
strerror(errno));
mpp->size = orig_size;
- return 1;
+ goto out;
}
- return 0;
+ r = 0;
+out:
+ pthread_cleanup_pop(1);
+ return r;
}
static int
diff --git a/multipathd/main.c b/multipathd/main.c
index ba52d393..2517b541 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -589,9 +589,9 @@ static int
update_map (struct multipath *mpp, struct vectors *vecs, int new_map)
{
int retries = 3;
- char *params __attribute__((cleanup(cleanup_charp))) = NULL;
+ char *params = NULL;
struct path *pp;
- int i;
+ int i, r;
retry:
condlog(4, "%s: updating new map", mpp->alias);
@@ -622,10 +622,11 @@ retry:
retries = -1;
goto fail;
}
- if (domap(mpp, params, 1) == DOMAP_FAIL && retries-- > 0) {
+ pthread_cleanup_push(cleanup_free_ptr, ¶ms);
+ r = domap(mpp, params, 1);
+ pthread_cleanup_pop(1);
+ if (r == DOMAP_FAIL && retries-- > 0) {
condlog(0, "%s: map_udate sleep", mpp->alias);
- free(params);
- params = NULL;
sleep(1);
goto retry;
}
@@ -1185,7 +1186,7 @@ int
ev_add_path (struct path * pp, struct vectors * vecs, int need_do_map)
{
struct multipath * mpp;
- char *params __attribute((cleanup(cleanup_charp))) = NULL;
+ char *params = NULL;
int retries = 3;
int start_waiter = 0;
int ret;
@@ -1277,6 +1278,7 @@ rescan:
/*
* reload the map for the multipath mapped device
*/
+ pthread_cleanup_push(cleanup_free_ptr, ¶ms);
ret = domap(mpp, params, 1);
while (ret == DOMAP_RETRY && retries-- > 0) {
condlog(0, "%s: retry domap for addition of new "
@@ -1284,6 +1286,7 @@ rescan:
sleep(1);
ret = domap(mpp, params, 1);
}
+ pthread_cleanup_pop(1);
if (ret == DOMAP_FAIL || ret == DOMAP_RETRY) {
condlog(0, "%s: failed in domap for addition of new "
"path %s", mpp->alias, pp->dev);
@@ -1294,8 +1297,6 @@ rescan:
condlog(0, "%s: ev_add_path sleep", mpp->alias);
sleep(1);
update_mpp_paths(mpp, vecs->pathvec);
- free(params);
- params = NULL;
goto rescan;
}
else if (mpp->action == ACT_RELOAD)
@@ -1356,8 +1357,9 @@ ev_remove_path (struct path *pp, struct vectors * vecs, int need_do_map)
{
struct multipath * mpp;
int i, retval = REMOVE_PATH_SUCCESS;
- char *params __attribute__((cleanup(cleanup_charp))) = NULL;
+ char *params = NULL;
+ pthread_cleanup_push(cleanup_free_ptr, ¶ms);
/*
* avoid referring to the map of an orphaned path
*/
@@ -1376,7 +1378,8 @@ ev_remove_path (struct path *pp, struct vectors * vecs, int need_do_map)
if (update_mpp_paths(mpp, vecs->pathvec)) {
condlog(0, "%s: failed to update paths",
mpp->alias);
- goto fail;
+ retval = REMOVE_PATH_MAP_ERROR;
+ goto out;
}
/*
@@ -1398,7 +1401,8 @@ ev_remove_path (struct path *pp, struct vectors * vecs, int need_do_map)
if (setup_map(mpp, ¶ms, vecs)) {
condlog(0, "%s: failed to setup map for"
" removal of path %s", mpp->alias, pp->dev);
- goto fail;
+ retval = REMOVE_PATH_MAP_ERROR;
+ goto out;
}
if (mpp->wait_for_udev) {
@@ -1431,8 +1435,10 @@ ev_remove_path (struct path *pp, struct vectors * vecs, int need_do_map)
/* setup_multipath will free the path
* regardless of whether it succeeds or
* fails */
- if (setup_multipath(vecs, mpp))
- return REMOVE_PATH_MAP_ERROR;
+ if (setup_multipath(vecs, mpp)) {
+ retval = REMOVE_PATH_MAP_ERROR;
+ goto fail;
+ }
sync_map_state(mpp);
condlog(2, "%s: path removed from map %s",
@@ -1445,13 +1451,14 @@ ev_remove_path (struct path *pp, struct vectors * vecs, int need_do_map)
free_path(pp);
}
out:
- return retval;
-
+ if (retval == REMOVE_PATH_MAP_ERROR) {
+ condlog(0, "%s: error removing path. removing map %s", pp->dev,
+ mpp->alias);
+ remove_map_and_stop_waiter(mpp, vecs);
+ }
fail:
- condlog(0, "%s: error removing path. removing map %s", pp->dev,
- mpp->alias);
- remove_map_and_stop_waiter(mpp, vecs);
- return REMOVE_PATH_MAP_ERROR;
+ pthread_cleanup_pop(1);
+ return retval;
}
int
@@ -2067,7 +2074,7 @@ int update_prio(struct path *pp, int refresh_all)
static int reload_map(struct vectors *vecs, struct multipath *mpp, int refresh,
int is_daemon)
{
- char *params __attribute__((cleanup(cleanup_charp))) = NULL;
+ char *params = NULL;
struct path *pp;
int i, r;
@@ -2089,9 +2096,11 @@ static int reload_map(struct vectors *vecs, struct multipath *mpp, int refresh,
condlog(0, "%s: failed to setup map", mpp->alias);
return 1;
}
+ pthread_cleanup_push(cleanup_free_ptr, ¶ms);
select_action(mpp, vecs->mpvec, 1);
r = domap(mpp, params, is_daemon);
+ pthread_cleanup_pop(1);
if (r == DOMAP_FAIL || r == DOMAP_RETRY) {
condlog(3, "%s: domap (%u) failure "
"for reload map", mpp->alias, r);
--
2.17.2
More information about the dm-devel
mailing list