* [dm-devel] [PATCH 0/4] remove dangerous cleanup __attribute__ uses
@ 2022-10-11 21:52 Benjamin Marzinski
2022-10-11 21:53 ` [dm-devel] [PATCH 1/4] libmultipath: don't print garbage keywords Benjamin Marzinski
` (4 more replies)
0 siblings, 5 replies; 11+ messages in thread
From: Benjamin Marzinski @ 2022-10-11 21:52 UTC (permalink / raw)
To: Christophe Varoqui; +Cc: device-mapper development, Martin Wilck
the cleanup __attribute__ is only run when a variable goes out of scope
normally. It is not run on pthread cancellation. This means that
multipathd could leak whatever resources were supposed to be cleaned up
if the thread was cancelled in a function using variables with the
cleanup __attribute__. This patchset removes all these uses in cases
where the code is run by multipathd and includes a cancellation point
in the variables scope (usually condlog(), which calls fprintf(), a
cancellation point, the way multipathd is usually run).
Benjamin Marzinski (4):
libmultipath: don't print garbage keywords
libmultipath: avoid STRBUF_ON_STACK with cancellation points
libmultipath: use regular array for field widths
libmultipath: avoid cleanup __attribute__ with cancellation points
libmpathutil/parser.c | 13 ++--
libmpathutil/strbuf.h | 4 +-
libmultipath/alias.c | 59 ++++++++++-------
libmultipath/blacklist.c | 4 +-
libmultipath/configure.c | 6 +-
libmultipath/discovery.c | 34 ++++++----
libmultipath/dmparser.c | 23 +++----
libmultipath/foreign.c | 9 +--
libmultipath/generic.c | 14 ++--
libmultipath/libmultipath.version | 4 +-
libmultipath/print.c | 82 ++++++++++++++----------
libmultipath/print.h | 4 +-
libmultipath/prioritizers/weightedpath.c | 22 ++++---
libmultipath/propsel.c | 76 ++++++++++++++++------
libmultipath/sysfs.h | 11 +---
libmultipath/uevent.c | 6 +-
multipath/main.c | 5 +-
multipathd/cli_handlers.c | 52 +++++++--------
multipathd/main.c | 49 ++++++++------
19 files changed, 286 insertions(+), 191 deletions(-)
--
2.17.2
--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel
^ permalink raw reply [flat|nested] 11+ messages in thread
* [dm-devel] [PATCH 1/4] libmultipath: don't print garbage keywords
2022-10-11 21:52 [dm-devel] [PATCH 0/4] remove dangerous cleanup __attribute__ uses Benjamin Marzinski
@ 2022-10-11 21:53 ` Benjamin Marzinski
2022-10-21 7:05 ` Martin Wilck
2022-10-11 21:53 ` [dm-devel] [PATCH 2/4] libmultipath: avoid STRBUF_ON_STACK with cancellation points Benjamin Marzinski
` (3 subsequent siblings)
4 siblings, 1 reply; 11+ messages in thread
From: Benjamin Marzinski @ 2022-10-11 21:53 UTC (permalink / raw)
To: Christophe Varoqui; +Cc: device-mapper development, Martin Wilck
If snprint_keyword() failed to correctly set up sbuf, don't print it.
Instead, return an error.
Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
libmpathutil/parser.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/libmpathutil/parser.c b/libmpathutil/parser.c
index 014d9b83..8d3ac53a 100644
--- a/libmpathutil/parser.c
+++ b/libmpathutil/parser.c
@@ -152,7 +152,7 @@ int
snprint_keyword(struct strbuf *buff, const char *fmt, struct keyword *kw,
const void *data)
{
- int r;
+ int r = 0;
char *f;
struct config *conf;
STRBUF_ON_STACK(sbuf);
@@ -190,8 +190,10 @@ snprint_keyword(struct strbuf *buff, const char *fmt, struct keyword *kw,
}
} while (*fmt++);
out:
- return __append_strbuf_str(buff, get_strbuf_str(&sbuf),
- get_strbuf_len(&sbuf));
+ if (r >= 0)
+ r = __append_strbuf_str(buff, get_strbuf_str(&sbuf),
+ get_strbuf_len(&sbuf));
+ return r;
}
static const char quote_marker[] = { '\0', '"', '\0' };
--
2.17.2
--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [dm-devel] [PATCH 2/4] libmultipath: avoid STRBUF_ON_STACK with cancellation points
2022-10-11 21:52 [dm-devel] [PATCH 0/4] remove dangerous cleanup __attribute__ uses Benjamin Marzinski
2022-10-11 21:53 ` [dm-devel] [PATCH 1/4] libmultipath: don't print garbage keywords Benjamin Marzinski
@ 2022-10-11 21:53 ` Benjamin Marzinski
2022-10-11 21:53 ` [dm-devel] [PATCH 3/4] libmultipath: use regular array for field widths Benjamin Marzinski
` (2 subsequent siblings)
4 siblings, 0 replies; 11+ messages in thread
From: Benjamin Marzinski @ 2022-10-11 21:53 UTC (permalink / raw)
To: Christophe Varoqui; +Cc: device-mapper development, Martin Wilck
STRBUF_ON_STACK() uses the cleanup __attribute__, which doesn't get run
if a thread is cancelled. condlog() will call fprintf() when run under
systemd, which is a cancellation point. The snprint function for the
generic mutipath and generic path operations both call cancellation
points. Also, the keyword print functions can call cancellation points.
Because of all this, I did not see any safe uses of STRBUF_ON_STACK()
outside of the unit tests. Replace them all with pthread cleanup
handlers.
Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
libmpathutil/parser.c | 5 +-
libmpathutil/strbuf.h | 4 +-
libmultipath/alias.c | 59 +++++++++++-------
libmultipath/blacklist.c | 4 +-
libmultipath/discovery.c | 34 +++++++----
libmultipath/dmparser.c | 21 +++----
libmultipath/foreign.c | 4 +-
libmultipath/generic.c | 10 +++-
libmultipath/print.c | 50 ++++++++++------
libmultipath/prioritizers/weightedpath.c | 16 ++---
libmultipath/propsel.c | 76 ++++++++++++++++++------
libmultipath/sysfs.h | 11 +---
libmultipath/uevent.c | 6 +-
13 files changed, 195 insertions(+), 105 deletions(-)
diff --git a/libmpathutil/parser.c b/libmpathutil/parser.c
index 8d3ac53a..04c65b51 100644
--- a/libmpathutil/parser.c
+++ b/libmpathutil/parser.c
@@ -25,6 +25,7 @@
#include "parser.h"
#include "debug.h"
#include "strbuf.h"
+#include "util.h"
/* local vars */
static int sublevel = 0;
@@ -155,11 +156,12 @@ snprint_keyword(struct strbuf *buff, const char *fmt, struct keyword *kw,
int r = 0;
char *f;
struct config *conf;
- STRBUF_ON_STACK(sbuf);
+ struct strbuf sbuf = STRBUF_INIT;
if (!kw || !kw->print)
return 0;
+ pthread_cleanup_push_cast(reset_strbuf, &sbuf);
do {
f = strchr(fmt, '%');
if (f == NULL) {
@@ -193,6 +195,7 @@ out:
if (r >= 0)
r = __append_strbuf_str(buff, get_strbuf_str(&sbuf),
get_strbuf_len(&sbuf));
+ pthread_cleanup_pop(1);
return r;
}
diff --git a/libmpathutil/strbuf.h b/libmpathutil/strbuf.h
index ae863417..5aa54677 100644
--- a/libmpathutil/strbuf.h
+++ b/libmpathutil/strbuf.h
@@ -42,7 +42,9 @@ void free_strbuf(struct strbuf *buf);
* macro: STRBUF_ON_STACK
*
* Define and initialize a local struct @strbuf to be cleaned up when
- * the current scope is left
+ * the current scope is left. This can only be used in non-threaded
+ * programs, or if there are no pthread cancellation points in the
+ * current scope, after the buffer could first be used.
*/
#define STRBUF_ON_STACK(__x) \
struct strbuf __attribute__((cleanup(reset_strbuf))) (__x) = STRBUF_INIT;
diff --git a/libmultipath/alias.c b/libmultipath/alias.c
index 05201224..3a81a7ad 100644
--- a/libmultipath/alias.c
+++ b/libmultipath/alias.c
@@ -112,12 +112,14 @@ scan_devname(const char *alias, const char *prefix)
static int
id_already_taken(int id, const char *prefix, const char *map_wwid)
{
- STRBUF_ON_STACK(buf);
+ struct strbuf buf = STRBUF_INIT;
const char *alias;
+ int r = 0;
+ pthread_cleanup_push_cast(reset_strbuf, &buf);
if (append_strbuf_str(&buf, prefix) < 0 ||
format_devname(&buf, id) < 0)
- return 0;
+ goto out;
alias = get_strbuf_str(&buf);
if (dm_map_present(alias)) {
@@ -126,11 +128,13 @@ id_already_taken(int id, const char *prefix, const char *map_wwid)
/* If both the name and the wwid match, then it's fine.*/
if (dm_get_uuid(alias, wwid, sizeof(wwid)) == 0 &&
strncmp(map_wwid, wwid, sizeof(wwid)) == 0)
- return 0;
+ goto out;
condlog(3, "%s: alias '%s' already taken, but not in bindings file. reselecting alias", map_wwid, alias);
- return 1;
+ r = 1;
}
- return 0;
+out:
+ pthread_cleanup_pop(1);
+ return r;
}
@@ -277,10 +281,12 @@ rlookup_binding(FILE *f, char *buff, const char *map_alias)
static char *
allocate_binding(int fd, const char *wwid, int id, const char *prefix)
{
- STRBUF_ON_STACK(buf);
+ struct strbuf buf = STRBUF_INIT;
off_t offset;
ssize_t len;
- char *alias, *c;
+ char *alias = NULL;
+ const char *str;
+ int end;
if (id <= 0) {
condlog(0, "%s: cannot allocate new binding for id %d",
@@ -288,38 +294,41 @@ allocate_binding(int fd, const char *wwid, int id, const char *prefix)
return NULL;
}
+ pthread_cleanup_push_cast(reset_strbuf, &buf);
if (append_strbuf_str(&buf, prefix) < 0 ||
format_devname(&buf, id) == -1)
- return NULL;
+ goto out;
if (print_strbuf(&buf, " %s\n", wwid) < 0)
- return NULL;
+ goto out;
offset = lseek(fd, 0, SEEK_END);
if (offset < 0){
condlog(0, "Cannot seek to end of bindings file : %s",
strerror(errno));
- return NULL;
+ goto out;
}
len = get_strbuf_len(&buf);
- alias = steal_strbuf_str(&buf);
+ str = get_strbuf_str(&buf);
- if (write(fd, alias, len) != len) {
+ if (write(fd, str, len) != len) {
condlog(0, "Cannot write binding to bindings file : %s",
strerror(errno));
/* clear partial write */
if (ftruncate(fd, offset))
condlog(0, "Cannot truncate the header : %s",
strerror(errno));
- free(alias);
- return NULL;
+ goto out;
}
- c = strchr(alias, ' ');
- if (c)
- *c = '\0';
+ end = strcspn(str, " ");
- condlog(3, "Created new binding [%s] for WWID [%s]", alias, wwid);
+ condlog(3, "Created new binding [%.*s] for WWID [%s]", end, str, wwid);
+ alias = steal_strbuf_str(&buf);
+
+ alias[len] = '\0';
+out:
+ pthread_cleanup_pop(1);
return alias;
}
@@ -549,24 +558,28 @@ static int add_binding(Bindings *bindings, const char *alias, const char *wwid)
static int write_bindings_file(const Bindings *bindings, int fd)
{
struct binding *bnd;
- STRBUF_ON_STACK(line);
- int i;
+ struct strbuf line = STRBUF_INIT;
+ int i, r = -1;
if (write(fd, BINDINGS_FILE_HEADER, sizeof(BINDINGS_FILE_HEADER) - 1)
!= sizeof(BINDINGS_FILE_HEADER) - 1)
return -1;
+ pthread_cleanup_push_cast(reset_strbuf, &line);
vector_foreach_slot(bindings, bnd, i) {
int len;
if ((len = print_strbuf(&line, "%s %s\n",
bnd->alias, bnd->wwid)) < 0)
- return -1;
+ goto out;
if (write(fd, get_strbuf_str(&line), len) != len)
- return -1;
+ goto out;
truncate_strbuf(&line, 0);
}
- return 0;
+ r = 0;
+out:
+ pthread_cleanup_pop(1);
+ return r;
}
static int fix_bindings_file(const struct config *conf,
diff --git a/libmultipath/blacklist.c b/libmultipath/blacklist.c
index 8d15d2ea..7e0b036a 100644
--- a/libmultipath/blacklist.c
+++ b/libmultipath/blacklist.c
@@ -341,11 +341,12 @@ int
filter_protocol(const struct _vector *blist, const struct _vector *elist,
const struct path *pp)
{
- STRBUF_ON_STACK(buf);
+ struct strbuf buf = STRBUF_INIT;
const char *prot;
int r = MATCH_NOTHING;
if (pp) {
+ pthread_cleanup_push_cast(reset_strbuf, &buf);
snprint_path_protocol(&buf, pp);
prot = get_strbuf_str(&buf);
@@ -354,6 +355,7 @@ filter_protocol(const struct _vector *blist, const struct _vector *elist,
else if (match_reglist(blist, prot))
r = MATCH_PROTOCOL_BLIST;
log_filter(pp->dev, NULL, NULL, NULL, NULL, prot, r, 3);
+ pthread_cleanup_pop(1);
}
return r;
diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
index f3fccedd..32181259 100644
--- a/libmultipath/discovery.c
+++ b/libmultipath/discovery.c
@@ -838,13 +838,15 @@ static void
scsi_tmo_error_msg(struct path *pp)
{
STATIC_BITFIELD(bf, LAST_BUS_PROTOCOL_ID + 1);
- STRBUF_ON_STACK(proto_buf);
+ struct strbuf proto_buf = STRBUF_INIT;
unsigned int proto_id = bus_protocol_id(pp);
+ pthread_cleanup_push_cast(reset_strbuf, &proto_buf);
snprint_path_protocol(&proto_buf, pp);
condlog(2, "%s: setting scsi timeouts is unsupported for protocol %s",
pp->dev, get_strbuf_str(&proto_buf));
set_bit_in_bitfield(proto_id, bf);
+ pthread_cleanup_pop(1);
}
int
@@ -1142,12 +1144,13 @@ parse_vpd_pg83(const unsigned char *in, size_t in_len,
size_t len, vpd_len, i;
int vpd_type, prio = -1;
int err = -ENODATA;
- STRBUF_ON_STACK(buf);
+ struct strbuf buf = STRBUF_INIT;
/* Need space at least for one digit */
if (out_len <= 1)
return 0;
+ pthread_cleanup_push_cast(reset_strbuf, &buf);
d = in + 4;
while (d <= in + in_len - 4) {
bool invalid = false;
@@ -1241,8 +1244,9 @@ parse_vpd_pg83(const unsigned char *in, size_t in_len,
}
if (prio <= 0)
- return err;
+ goto out;
+ err = 0;
if (d != in + in_len)
/* Should this be fatal? (overflow covered above) */
condlog(2, "%s: warning: last descriptor end %zd != VPD length %zu",
@@ -1262,10 +1266,10 @@ parse_vpd_pg83(const unsigned char *in, size_t in_len,
size_t i;
if ((err = print_strbuf(&buf, "%d", vpd_type)) < 0)
- return err;
+ goto out;
for (i = 0; i < vpd_len; i++)
if ((err = print_strbuf(&buf, "%02x", vpd[i])) < 0)
- return err;
+ goto out;
} else if (vpd_type == 0x8) {
char type;
@@ -1276,12 +1280,12 @@ parse_vpd_pg83(const unsigned char *in, size_t in_len,
else
type = '8';
if ((err = fill_strbuf(&buf, type, 1)) < 0)
- return err;
+ goto out;
vpd += 4;
len = vpd_len - 4;
if ((err = __append_strbuf_str(&buf, (const char *)vpd, len)) < 0)
- return err;
+ goto out;
/* The input is 0-padded, make sure the length is correct */
truncate_strbuf(&buf, strlen(get_strbuf_str(&buf)));
@@ -1298,12 +1302,12 @@ parse_vpd_pg83(const unsigned char *in, size_t in_len,
size_t p_len;
if ((err = fill_strbuf(&buf, '1', 1)) < 0)
- return err;
+ goto out;
while (vpd && (p = memchr(vpd, ' ', vpd_len))) {
p_len = p - vpd;
if ((err = __append_strbuf_str(&buf, (const char *)vpd,
p_len)) < 0)
- return err;
+ goto out;
vpd = p;
vpd_len -= p_len;
while (vpd && vpd_len > 0 && *vpd == ' ') {
@@ -1311,12 +1315,12 @@ parse_vpd_pg83(const unsigned char *in, size_t in_len,
vpd_len --;
}
if (vpd_len > 0 && (err = fill_strbuf(&buf, '_', 1)) < 0)
- return err;
+ goto out;
}
if (vpd_len > 0) {
if ((err = __append_strbuf_str(&buf, (const char *)vpd,
vpd_len)) < 0)
- return err;
+ goto out;
}
}
@@ -1331,6 +1335,10 @@ parse_vpd_pg83(const unsigned char *in, size_t in_len,
len = out_len - 1;
}
strlcpy(out, get_strbuf_str(&buf), len + 1);
+out:
+ pthread_cleanup_pop(1);
+ if (err)
+ return err;
return len;
}
@@ -2435,11 +2443,13 @@ int pathinfo(struct path *pp, struct config *conf, int mask)
* It's likely that this path is not fit for
* multipath use.
*/
- STRBUF_ON_STACK(buf);
+ struct strbuf buf = STRBUF_INIT;
+ pthread_cleanup_push_cast(reset_strbuf, &buf);
snprint_path(&buf, "%T", pp, 0);
condlog(1, "%s: no WWID in state \"%s\", giving up",
pp->dev, get_strbuf_str(&buf));
+ pthread_cleanup_pop(1);
return PATHINFO_SKIPPED;
}
return PATHINFO_OK;
diff --git a/libmultipath/dmparser.c b/libmultipath/dmparser.c
index 3b37a926..44650aaa 100644
--- a/libmultipath/dmparser.c
+++ b/libmultipath/dmparser.c
@@ -48,10 +48,10 @@ int assemble_map(struct multipath *mp, char **params)
{
static const char no_path_retry[] = "queue_if_no_path";
static const char retain_hwhandler[] = "retain_attached_hw_handler";
- int i, j;
+ int i, j, r = 1;
int minio;
int nr_priority_groups, initial_pg_nr;
- STRBUF_ON_STACK(buff);
+ struct strbuf buff = STRBUF_INIT;
struct pathgroup * pgp;
struct path * pp;
@@ -68,15 +68,16 @@ int assemble_map(struct multipath *mp, char **params)
get_linux_version_code() < KERNEL_VERSION(4, 3, 0))
add_feature(&mp->features, retain_hwhandler);
+ pthread_cleanup_push_cast(reset_strbuf, &buff);
if (print_strbuf(&buff, "%s %s %i %i", mp->features, mp->hwhandler,
nr_priority_groups, initial_pg_nr) < 0)
- goto err;
+ goto out;
vector_foreach_slot (mp->pg, pgp, i) {
pgp = VECTOR_SLOT(mp->pg, i);
if (print_strbuf(&buff, " %s %i 1", mp->selector,
VECTOR_SIZE(pgp->paths)) < 0)
- goto err;
+ goto out;
vector_foreach_slot (pgp->paths, pp, j) {
int tmp_minio = minio;
@@ -86,19 +87,19 @@ int assemble_map(struct multipath *mp, char **params)
tmp_minio = minio * pp->priority;
if (!strlen(pp->dev_t) ) {
condlog(0, "dev_t not set for '%s'", pp->dev);
- goto err;
+ goto out;
}
if (print_strbuf(&buff, " %s %d", pp->dev_t, tmp_minio) < 0)
- goto err;
+ goto out;
}
}
*params = steal_strbuf_str(&buff);
condlog(4, "%s: assembled map [%s]", mp->alias, *params);
- return 0;
-
-err:
- return 1;
+ r = 0;
+out:
+ pthread_cleanup_pop(1);
+ return r;
}
/*
diff --git a/libmultipath/foreign.c b/libmultipath/foreign.c
index d01a5ef0..8981ff58 100644
--- a/libmultipath/foreign.c
+++ b/libmultipath/foreign.c
@@ -547,7 +547,7 @@ int snprint_foreign_topology(struct strbuf *buf, int verbosity,
void print_foreign_topology(int verbosity)
{
- STRBUF_ON_STACK(buf);
+ struct strbuf buf = STRBUF_INIT;
struct foreign *fgn;
int i;
fieldwidth_t *width __attribute__((cleanup(cleanup_ucharp))) = NULL;
@@ -559,6 +559,7 @@ void print_foreign_topology(int verbosity)
unlock_foreigns(NULL);
return;
}
+ pthread_cleanup_push_cast(reset_strbuf, &buf);
pthread_cleanup_push(unlock_foreigns, NULL);
vector_foreach_slot(foreigns, fgn, i) {
const struct _vector *vec;
@@ -573,6 +574,7 @@ void print_foreign_topology(int verbosity)
__snprint_foreign_topology(&buf, verbosity, width);
pthread_cleanup_pop(1);
printf("%s", get_strbuf_str(&buf));
+ pthread_cleanup_pop(1);
}
int snprint_foreign_paths(struct strbuf *buf, const char *style,
diff --git a/libmultipath/generic.c b/libmultipath/generic.c
index e7cf2975..3e2268e6 100644
--- a/libmultipath/generic.c
+++ b/libmultipath/generic.c
@@ -23,15 +23,19 @@
int generic_style(const struct gen_multipath* gm, struct strbuf *buf,
__attribute__((unused)) int verbosity)
{
- STRBUF_ON_STACK(tmp);
+ struct strbuf tmp = STRBUF_INIT;
char *alias_buf __attribute__((cleanup(cleanup_charp)));
const char *wwid_buf;
+ int ret;
+ pthread_cleanup_push_cast(reset_strbuf, &tmp);
gm->ops->snprint(gm, &tmp, 'n');
alias_buf = steal_strbuf_str(&tmp);
gm->ops->snprint(gm, &tmp, 'w');
wwid_buf = get_strbuf_str(&tmp);
- return print_strbuf(buf, "%%n %s[%%G]:%%d %%s",
- strcmp(alias_buf, wwid_buf) ? "(%w) " : "");
+ ret = print_strbuf(buf, "%%n %s[%%G]:%%d %%s",
+ strcmp(alias_buf, wwid_buf) ? "(%w) " : "");
+ pthread_cleanup_pop(1);
+ return ret;
}
diff --git a/libmultipath/print.c b/libmultipath/print.c
index d7d522c8..97f9a177 100644
--- a/libmultipath/print.c
+++ b/libmultipath/print.c
@@ -911,19 +911,21 @@ void _get_path_layout (const struct _vector *gpvec, enum layout_reset reset,
return;
for (j = 0; j < ARRAY_SIZE(pd); j++) {
- STRBUF_ON_STACK(buff);
+ struct strbuf buff = STRBUF_INIT;
reset_width(&width[j], reset, pd[j].header);
if (gpvec == NULL)
continue;
+ pthread_cleanup_push_cast(reset_strbuf, &buff);
vector_foreach_slot (gpvec, gp, i) {
gp->ops->snprint(gp, &buff, pd[j].wildcard);
width[j] = MAX(width[j],
MIN(get_strbuf_len(&buff), MAX_FIELD_WIDTH));
truncate_strbuf(&buff, 0);
}
+ pthread_cleanup_pop(1);
}
}
@@ -951,19 +953,21 @@ _get_multipath_layout (const struct _vector *gmvec, enum layout_reset reset,
if (width == NULL)
return;
for (j = 0; j < ARRAY_SIZE(mpd); j++) {
- STRBUF_ON_STACK(buff);
+ struct strbuf buff = STRBUF_INIT;
reset_width(&width[j], reset, mpd[j].header);
if (gmvec == NULL)
continue;
+ pthread_cleanup_push_cast(reset_strbuf, &buff);
vector_foreach_slot (gmvec, gm, i) {
gm->ops->snprint(gm, &buff, mpd[j].wildcard);
width[j] = MAX(width[j],
MIN(get_strbuf_len(&buff), MAX_FIELD_WIDTH));
truncate_strbuf(&buff, 0);
}
+ pthread_cleanup_pop(1);
condlog(4, "%s: width %d", mpd[j].header, width[j]);
}
}
@@ -1182,7 +1186,7 @@ int _snprint_pathgroup(const struct gen_pathgroup *ggp, struct strbuf *line,
void _print_multipath_topology(const struct gen_multipath *gmp, int verbosity)
{
- STRBUF_ON_STACK(buff);
+ struct strbuf buff = STRBUF_INIT;
fieldwidth_t *p_width __attribute__((cleanup(cleanup_ucharp))) = NULL;
const struct gen_pathgroup *gpg;
const struct _vector *pgvec, *pathvec;
@@ -1202,8 +1206,10 @@ void _print_multipath_topology(const struct gen_multipath *gmp, int verbosity)
gmp->ops->rel_pathgroups(gmp, pgvec);
}
+ pthread_cleanup_push_cast(reset_strbuf, &buff);
_snprint_multipath_topology(gmp, &buff, verbosity, p_width);
printf("%s", get_strbuf_str(&buff));
+ pthread_cleanup_pop(1);
}
int snprint_multipath_style(const struct gen_multipath *gmp,
@@ -1225,10 +1231,10 @@ int _snprint_multipath_topology(const struct gen_multipath *gmp,
struct strbuf *buff, int verbosity,
const fieldwidth_t *p_width)
{
- int j, i, rc;
+ int j, i, rc = 0;
const struct _vector *pgvec;
const struct gen_pathgroup *gpg;
- STRBUF_ON_STACK(style);
+ struct strbuf style = STRBUF_INIT;
size_t initial_len = get_strbuf_len(buff);
fieldwidth_t *width __attribute__((cleanup(cleanup_ucharp))) = NULL;
@@ -1241,17 +1247,21 @@ int _snprint_multipath_topology(const struct gen_multipath *gmp,
if (verbosity == 1)
return _snprint_multipath(gmp, buff, "%n", width);
+ pthread_cleanup_push_cast(reset_strbuf, &style);
if(isatty(1) &&
(rc = print_strbuf(&style, "%c[%dm", 0x1B, 1)) < 0) /* bold on */
- return rc;
+ goto err;
if ((rc = gmp->ops->style(gmp, &style, verbosity)) < 0)
- return rc;
+ goto err;
if(isatty(1) &&
(rc = print_strbuf(&style, "%c[%dm", 0x1B, 0)) < 0) /* bold off */
- return rc;
+ goto err;
- if ((rc = _snprint_multipath(gmp, buff, get_strbuf_str(&style), width)) < 0
- || (rc = _snprint_multipath(gmp, buff, PRINT_MAP_PROPS, width)) < 0)
+ if ((rc = _snprint_multipath(gmp, buff, get_strbuf_str(&style), width)) >= 0)
+ rc = _snprint_multipath(gmp, buff, PRINT_MAP_PROPS, width);
+err:
+ pthread_cleanup_pop(1);
+ if (rc < 0)
return rc;
pgvec = gmp->ops->get_pathgroups(gmp);
@@ -1868,7 +1878,7 @@ int __snprint_config(const struct config *conf, struct strbuf *buff,
{
int rc;
- if ((rc = snprint_defaults(conf, buff)) < 0 ||
+ if ((rc = snprint_defaults(conf, buff)) < 1 ||
(rc = snprint_blacklist(conf, buff)) < 0 ||
(rc = snprint_blacklist_except(conf, buff)) < 0 ||
(rc = snprint_hwtable(conf, buff,
@@ -1887,17 +1897,21 @@ int __snprint_config(const struct config *conf, struct strbuf *buff,
char *snprint_config(const struct config *conf, int *len,
const struct _vector *hwtable, const struct _vector *mpvec)
{
- STRBUF_ON_STACK(buff);
- char *reply;
- int rc = __snprint_config(conf, &buff, hwtable, mpvec);
+ struct strbuf buff = STRBUF_INIT;
+ char *reply = NULL;
+ int rc;
+
+ pthread_cleanup_push_cast(reset_strbuf, &buff);
+ rc = __snprint_config(conf, &buff, hwtable, mpvec);
if (rc < 0)
- return NULL;
+ goto out;
if (len)
*len = get_strbuf_len(&buff);
reply = steal_strbuf_str(&buff);
-
+out:
+ pthread_cleanup_pop(1);
return reply;
}
@@ -2012,7 +2026,7 @@ static void print_all_paths_custo(vector pathvec, int banner, const char *fmt)
{
int i;
struct path * pp;
- STRBUF_ON_STACK(line);
+ struct strbuf line = STRBUF_INIT;
fieldwidth_t *width __attribute__((cleanup(cleanup_ucharp))) = NULL;
if (!VECTOR_SIZE(pathvec)) {
@@ -2025,6 +2039,7 @@ static void print_all_paths_custo(vector pathvec, int banner, const char *fmt)
return;
get_path_layout(pathvec, 1, width);
+ pthread_cleanup_push_cast(reset_strbuf, &line);
if (banner)
append_strbuf_str(&line, "===== paths list =====\n");
@@ -2034,6 +2049,7 @@ static void print_all_paths_custo(vector pathvec, int banner, const char *fmt)
snprint_path(&line, fmt, pp, width);
printf("%s", get_strbuf_str(&line));
+ pthread_cleanup_pop(1);
}
void print_all_paths(vector pathvec, int banner)
diff --git a/libmultipath/prioritizers/weightedpath.c b/libmultipath/prioritizers/weightedpath.c
index 561ebb48..df700bf3 100644
--- a/libmultipath/prioritizers/weightedpath.c
+++ b/libmultipath/prioritizers/weightedpath.c
@@ -63,7 +63,7 @@ build_wwn_path(struct path *pp, struct strbuf *buf)
/* main priority routine */
int prio_path_weight(struct path *pp, char *prio_args)
{
- STRBUF_ON_STACK(path);
+ struct strbuf path = STRBUF_INIT;
char *arg __attribute__((cleanup(cleanup_charp))) = NULL;
char *temp, *regex, *prio;
char split_char[] = " \t";
@@ -84,24 +84,25 @@ int prio_path_weight(struct path *pp, char *prio_args)
return priority;
}
+ pthread_cleanup_push_cast(reset_strbuf, &path);
if (!strcmp(regex, HBTL)) {
if (print_strbuf(&path, "%d:%d:%d:%" PRIu64, pp->sg_id.host_no,
pp->sg_id.channel, pp->sg_id.scsi_id,
pp->sg_id.lun) < 0)
- return priority;
+ goto out;
} else if (!strcmp(regex, DEV_NAME)) {
if (append_strbuf_str(&path, pp->dev) < 0)
- return priority;
+ goto out;
} else if (!strcmp(regex, SERIAL)) {
if (build_serial_path(pp, &path) != 0)
- return priority;
+ goto out;
} else if (!strcmp(regex, WWN)) {
if (build_wwn_path(pp, &path) != 0)
- return priority;
+ goto out;
} else {
condlog(0, "%s: %s - Invalid arguments", pp->dev,
pp->prio.name);
- return priority;
+ goto out;
}
while (!path_found) {
@@ -121,7 +122,8 @@ int prio_path_weight(struct path *pp, char *prio_args)
regfree(&pathe);
}
}
-
+out:
+ pthread_cleanup_pop(1);
return priority;
}
diff --git a/libmultipath/propsel.c b/libmultipath/propsel.c
index d4f19897..f35acdaa 100644
--- a/libmultipath/propsel.c
+++ b/libmultipath/propsel.c
@@ -217,7 +217,7 @@ out:
int select_rr_weight(struct config *conf, struct multipath * mp)
{
const char *origin;
- STRBUF_ON_STACK(buff);
+ struct strbuf buff = STRBUF_INIT;
mp_set_mpe(rr_weight);
mp_set_ovr(rr_weight);
@@ -225,16 +225,18 @@ int select_rr_weight(struct config *conf, struct multipath * mp)
mp_set_conf(rr_weight);
mp_set_default(rr_weight, DEFAULT_RR_WEIGHT);
out:
+ pthread_cleanup_push_cast(reset_strbuf, &buff);
print_rr_weight(&buff, mp->rr_weight);
condlog(3, "%s: rr_weight = %s %s", mp->alias,
get_strbuf_str(&buff), origin);
+ pthread_cleanup_pop(1);
return 0;
}
int select_pgfailback(struct config *conf, struct multipath * mp)
{
const char *origin;
- STRBUF_ON_STACK(buff);
+ struct strbuf buff = STRBUF_INIT;
mp_set_mpe(pgfailback);
mp_set_ovr(pgfailback);
@@ -242,9 +244,11 @@ int select_pgfailback(struct config *conf, struct multipath * mp)
mp_set_conf(pgfailback);
mp_set_default(pgfailback, DEFAULT_FAILBACK);
out:
+ pthread_cleanup_push_cast(reset_strbuf, &buff);
print_pgfailback(&buff, mp->pgfailback);
condlog(3, "%s: failback = %s %s", mp->alias,
get_strbuf_str(&buff), origin);
+ pthread_cleanup_pop(1);
return 0;
}
@@ -367,7 +371,7 @@ void reconcile_features_with_options(const char *id, char **features, int* no_pa
{
static const char q_i_n_p[] = "queue_if_no_path";
static const char r_a_h_h[] = "retain_attached_hw_handler";
- STRBUF_ON_STACK(buff);
+ struct strbuf buff = STRBUF_INIT;
if (*features == NULL)
return;
@@ -382,6 +386,7 @@ void reconcile_features_with_options(const char *id, char **features, int* no_pa
* For backward compatibility we allow 'features "1 queue_if_no_path"';
* it's translated into "no_path_retry queue" here.
*/
+ pthread_cleanup_push_cast(reset_strbuf, &buff);
if (strstr(*features, q_i_n_p)) {
condlog(0, "%s: option 'features \"1 %s\"' is deprecated, "
"please use 'no_path_retry queue' instead",
@@ -400,6 +405,7 @@ void reconcile_features_with_options(const char *id, char **features, int* no_pa
}
remove_feature(features, q_i_n_p);
}
+ pthread_cleanup_pop(1);
if (strstr(*features, r_a_h_h)) {
condlog(0, "%s: option 'features \"1 %s\"' is deprecated",
id, r_a_h_h);
@@ -779,7 +785,7 @@ out:
int select_no_path_retry(struct config *conf, struct multipath *mp)
{
const char *origin = NULL;
- STRBUF_ON_STACK(buff);
+ struct strbuf buff = STRBUF_INIT;
if (mp->disable_queueing) {
condlog(0, "%s: queueing disabled", mp->alias);
@@ -791,6 +797,7 @@ int select_no_path_retry(struct config *conf, struct multipath *mp)
mp_set_hwe(no_path_retry);
mp_set_conf(no_path_retry);
out:
+ pthread_cleanup_push_cast(reset_strbuf, &buff);
print_no_path_retry(&buff, mp->no_path_retry);
if (origin)
condlog(3, "%s: no_path_retry = %s %s", mp->alias,
@@ -798,6 +805,7 @@ out:
else
condlog(3, "%s: no_path_retry = undef %s",
mp->alias, default_origin);
+ pthread_cleanup_pop(1);
return 0;
}
@@ -845,23 +853,25 @@ int select_minio(struct config *conf, struct multipath *mp)
int select_fast_io_fail(struct config *conf, struct path *pp)
{
const char *origin;
- STRBUF_ON_STACK(buff);
+ struct strbuf buff = STRBUF_INIT;
pp_set_ovr_pce(fast_io_fail);
pp_set_hwe(fast_io_fail);
pp_set_conf(fast_io_fail);
pp_set_default(fast_io_fail, DEFAULT_FAST_IO_FAIL);
out:
+ pthread_cleanup_push_cast(reset_strbuf, &buff);
print_undef_off_zero(&buff, pp->fast_io_fail);
condlog(3, "%s: fast_io_fail_tmo = %s %s", pp->dev,
get_strbuf_str(&buff), origin);
+ pthread_cleanup_pop(1);
return 0;
}
int select_dev_loss(struct config *conf, struct path *pp)
{
const char *origin;
- STRBUF_ON_STACK(buff);
+ struct strbuf buff = STRBUF_INIT;
pp_set_ovr_pce(dev_loss);
pp_set_hwe(dev_loss);
@@ -869,16 +879,18 @@ int select_dev_loss(struct config *conf, struct path *pp)
pp->dev_loss = DEV_LOSS_TMO_UNSET;
return 0;
out:
+ pthread_cleanup_push_cast(reset_strbuf, &buff);
print_dev_loss(&buff, pp->dev_loss);
condlog(3, "%s: dev_loss_tmo = %s %s", pp->dev,
get_strbuf_str(&buff), origin);
+ pthread_cleanup_pop(1);
return 0;
}
int select_eh_deadline(struct config *conf, struct path *pp)
{
const char *origin;
- STRBUF_ON_STACK(buff);
+ struct strbuf buff = STRBUF_INIT;
pp_set_ovr_pce(eh_deadline);
pp_set_hwe(eh_deadline);
@@ -887,9 +899,11 @@ int select_eh_deadline(struct config *conf, struct path *pp)
/* not changing sysfs in default cause, so don't print anything */
return 0;
out:
+ pthread_cleanup_push_cast(reset_strbuf, &buff);
print_undef_off_zero(&buff, pp->eh_deadline);
condlog(3, "%s: eh_deadline = %s %s", pp->dev,
get_strbuf_str(&buff), origin);
+ pthread_cleanup_pop(1);
return 0;
}
@@ -911,7 +925,7 @@ out:
int select_reservation_key(struct config *conf, struct multipath *mp)
{
const char *origin;
- STRBUF_ON_STACK(buff);
+ struct strbuf buff = STRBUF_INIT;
char *from_file = "";
uint64_t prkey = 0;
@@ -929,10 +943,12 @@ out:
else
put_be64(mp->reservation_key, prkey);
}
+ pthread_cleanup_push_cast(reset_strbuf, &buff);
print_reservation_key(&buff, mp->reservation_key,
mp->sa_flags, mp->prkey_source);
condlog(3, "%s: reservation_key = %s %s%s", mp->alias,
get_strbuf_str(&buff), origin, from_file);
+ pthread_cleanup_pop(1);
return 0;
}
@@ -1029,16 +1045,18 @@ use_delay_watch_checks(struct config *conf, struct multipath *mp)
{
int value = NU_UNDEF;
const char *origin = default_origin;
- STRBUF_ON_STACK(buff);
+ struct strbuf buff = STRBUF_INIT;
do_set(delay_watch_checks, mp->mpe, value, multipaths_origin);
do_set(delay_watch_checks, conf->overrides, value, overrides_origin);
do_set_from_hwe(delay_watch_checks, mp, value, hwe_origin);
do_set(delay_watch_checks, conf, value, conf_origin);
out:
+ pthread_cleanup_push_cast(reset_strbuf, &buff);
if (print_off_int_undef(&buff, value) > 0)
condlog(3, "%s: delay_watch_checks = %s %s", mp->alias,
get_strbuf_str(&buff), origin);
+ pthread_cleanup_pop(1);
return value;
}
@@ -1047,23 +1065,25 @@ use_delay_wait_checks(struct config *conf, struct multipath *mp)
{
int value = NU_UNDEF;
const char *origin = default_origin;
- STRBUF_ON_STACK(buff);
+ struct strbuf buff = STRBUF_INIT;
do_set(delay_wait_checks, mp->mpe, value, multipaths_origin);
do_set(delay_wait_checks, conf->overrides, value, overrides_origin);
do_set_from_hwe(delay_wait_checks, mp, value, hwe_origin);
do_set(delay_wait_checks, conf, value, conf_origin);
out:
+ pthread_cleanup_push_cast(reset_strbuf, &buff);
if (print_off_int_undef(&buff, value) > 0)
condlog(3, "%s: delay_wait_checks = %s %s", mp->alias,
get_strbuf_str(&buff), origin);
+ pthread_cleanup_pop(1);
return value;
}
int select_delay_checks(struct config *conf, struct multipath *mp)
{
int watch_checks, wait_checks;
- STRBUF_ON_STACK(buff);
+ struct strbuf buff = STRBUF_INIT;
watch_checks = use_delay_watch_checks(conf, mp);
wait_checks = use_delay_wait_checks(conf, mp);
@@ -1077,6 +1097,7 @@ int select_delay_checks(struct config *conf, struct multipath *mp)
mp->san_path_err_threshold = 1;
condlog(3, "%s: san_path_err_threshold = 1 %s", mp->alias,
(watch_checks > 0)? delay_watch_origin : delay_wait_origin);
+ pthread_cleanup_push_cast(reset_strbuf, &buff);
if (watch_checks > 0) {
mp->san_path_err_forget_rate = watch_checks;
print_off_int_undef(&buff, mp->san_path_err_forget_rate);
@@ -1091,6 +1112,7 @@ int select_delay_checks(struct config *conf, struct multipath *mp)
condlog(3, "%s: san_path_err_recovery_time = %s %s", mp->alias,
get_strbuf_str(&buff), delay_wait_origin);
}
+ pthread_cleanup_pop(1);
return 0;
}
@@ -1108,7 +1130,7 @@ static int san_path_deprecated_warned;
int select_san_path_err_threshold(struct config *conf, struct multipath *mp)
{
const char *origin;
- STRBUF_ON_STACK(buff);
+ struct strbuf buff = STRBUF_INIT;
if (marginal_path_check_enabled(mp) || (conf->marginal_pathgroups == MARGINAL_PATHGROUP_FPIN)) {
mp->san_path_err_threshold = NU_NO;
@@ -1124,17 +1146,19 @@ int select_san_path_err_threshold(struct config *conf, struct multipath *mp)
mp_set_conf(san_path_err_threshold);
mp_set_default(san_path_err_threshold, DEFAULT_ERR_CHECKS);
out:
+ pthread_cleanup_push_cast(reset_strbuf, &buff);
if (print_off_int_undef(&buff, mp->san_path_err_threshold) > 0)
condlog(3, "%s: san_path_err_threshold = %s %s",
mp->alias, get_strbuf_str(&buff), origin);
warn_san_path_deprecated(mp, san_path_err_threshold);
+ pthread_cleanup_pop(1);
return 0;
}
int select_san_path_err_forget_rate(struct config *conf, struct multipath *mp)
{
const char *origin;
- STRBUF_ON_STACK(buff);
+ struct strbuf buff = STRBUF_INIT;
if (marginal_path_check_enabled(mp) || (conf->marginal_pathgroups == MARGINAL_PATHGROUP_FPIN)) {
mp->san_path_err_forget_rate = NU_NO;
@@ -1150,9 +1174,11 @@ int select_san_path_err_forget_rate(struct config *conf, struct multipath *mp)
mp_set_conf(san_path_err_forget_rate);
mp_set_default(san_path_err_forget_rate, DEFAULT_ERR_CHECKS);
out:
+ pthread_cleanup_push_cast(reset_strbuf, &buff);
if (print_off_int_undef(&buff, mp->san_path_err_forget_rate) > 0)
condlog(3, "%s: san_path_err_forget_rate = %s %s",
mp->alias, get_strbuf_str(&buff), origin);
+ pthread_cleanup_pop(1);
warn_san_path_deprecated(mp, san_path_err_forget_rate);
return 0;
@@ -1161,7 +1187,7 @@ out:
int select_san_path_err_recovery_time(struct config *conf, struct multipath *mp)
{
const char *origin;
- STRBUF_ON_STACK(buff);
+ struct strbuf buff = STRBUF_INIT;
if (marginal_path_check_enabled(mp) || (conf->marginal_pathgroups == MARGINAL_PATHGROUP_FPIN)) {
mp->san_path_err_recovery_time = NU_NO;
@@ -1177,9 +1203,11 @@ int select_san_path_err_recovery_time(struct config *conf, struct multipath *mp)
mp_set_conf(san_path_err_recovery_time);
mp_set_default(san_path_err_recovery_time, DEFAULT_ERR_CHECKS);
out:
+ pthread_cleanup_push_cast(reset_strbuf, &buff);
if (print_off_int_undef(&buff, mp->san_path_err_recovery_time) != 0)
condlog(3, "%s: san_path_err_recovery_time = %s %s", mp->alias,
get_strbuf_str(&buff), origin);
+ pthread_cleanup_pop(1);
warn_san_path_deprecated(mp, san_path_err_recovery_time);
return 0;
@@ -1188,7 +1216,7 @@ out:
int select_marginal_path_err_sample_time(struct config *conf, struct multipath *mp)
{
const char *origin;
- STRBUF_ON_STACK(buff);
+ struct strbuf buff = STRBUF_INIT;
if (conf->marginal_pathgroups == MARGINAL_PATHGROUP_FPIN) {
mp->marginal_path_err_sample_time = NU_NO;
@@ -1208,16 +1236,18 @@ out:
mp->alias, 2 * IOTIMEOUT_SEC);
mp->marginal_path_err_sample_time = 2 * IOTIMEOUT_SEC;
}
+ pthread_cleanup_push_cast(reset_strbuf, &buff);
if (print_off_int_undef(&buff, mp->marginal_path_err_sample_time) > 0)
condlog(3, "%s: marginal_path_err_sample_time = %s %s",
mp->alias, get_strbuf_str(&buff), origin);
+ pthread_cleanup_pop(1);
return 0;
}
int select_marginal_path_err_rate_threshold(struct config *conf, struct multipath *mp)
{
const char *origin;
- STRBUF_ON_STACK(buff);
+ struct strbuf buff = STRBUF_INIT;
if (conf->marginal_pathgroups == MARGINAL_PATHGROUP_FPIN) {
mp->marginal_path_err_rate_threshold = NU_NO;
@@ -1231,16 +1261,18 @@ int select_marginal_path_err_rate_threshold(struct config *conf, struct multipat
mp_set_conf(marginal_path_err_rate_threshold);
mp_set_default(marginal_path_err_rate_threshold, DEFAULT_ERR_CHECKS);
out:
+ pthread_cleanup_push_cast(reset_strbuf, &buff);
if (print_off_int_undef(&buff, mp->marginal_path_err_rate_threshold) > 0)
condlog(3, "%s: marginal_path_err_rate_threshold = %s %s",
mp->alias, get_strbuf_str(&buff), origin);
+ pthread_cleanup_pop(1);
return 0;
}
int select_marginal_path_err_recheck_gap_time(struct config *conf, struct multipath *mp)
{
const char *origin;
- STRBUF_ON_STACK(buff);
+ struct strbuf buff = STRBUF_INIT;
if (conf->marginal_pathgroups == MARGINAL_PATHGROUP_FPIN) {
mp->marginal_path_err_recheck_gap_time = NU_NO;
@@ -1254,17 +1286,19 @@ int select_marginal_path_err_recheck_gap_time(struct config *conf, struct multip
mp_set_conf(marginal_path_err_recheck_gap_time);
mp_set_default(marginal_path_err_recheck_gap_time, DEFAULT_ERR_CHECKS);
out:
+ pthread_cleanup_push_cast(reset_strbuf, &buff);
if (print_off_int_undef(&buff,
mp->marginal_path_err_recheck_gap_time) > 0)
condlog(3, "%s: marginal_path_err_recheck_gap_time = %s %s",
mp->alias, get_strbuf_str(&buff), origin);
+ pthread_cleanup_pop(1);
return 0;
}
int select_marginal_path_double_failed_time(struct config *conf, struct multipath *mp)
{
const char *origin;
- STRBUF_ON_STACK(buff);
+ struct strbuf buff = STRBUF_INIT;
if (conf->marginal_pathgroups == MARGINAL_PATHGROUP_FPIN) {
mp->marginal_path_double_failed_time = NU_NO;
@@ -1278,9 +1312,11 @@ int select_marginal_path_double_failed_time(struct config *conf, struct multipat
mp_set_conf(marginal_path_double_failed_time);
mp_set_default(marginal_path_double_failed_time, DEFAULT_ERR_CHECKS);
out:
+ pthread_cleanup_push_cast(reset_strbuf, &buff);
if (print_off_int_undef(&buff, mp->marginal_path_double_failed_time) > 0)
condlog(3, "%s: marginal_path_double_failed_time = %s %s",
mp->alias, get_strbuf_str(&buff), origin);
+ pthread_cleanup_pop(1);
return 0;
}
@@ -1324,7 +1360,7 @@ out:
int select_ghost_delay (struct config *conf, struct multipath * mp)
{
const char *origin;
- STRBUF_ON_STACK(buff);
+ struct strbuf buff = STRBUF_INIT;
mp_set_mpe(ghost_delay);
mp_set_ovr(ghost_delay);
@@ -1332,9 +1368,11 @@ int select_ghost_delay (struct config *conf, struct multipath * mp)
mp_set_conf(ghost_delay);
mp_set_default(ghost_delay, DEFAULT_GHOST_DELAY);
out:
+ pthread_cleanup_push_cast(reset_strbuf, &buff);
if (print_off_int_undef(&buff, mp->ghost_delay) != 0)
condlog(3, "%s: ghost_delay = %s %s", mp->alias,
get_strbuf_str(&buff), origin);
+ pthread_cleanup_pop(1);
return 0;
}
diff --git a/libmultipath/sysfs.h b/libmultipath/sysfs.h
index 799f68e9..7e80b411 100644
--- a/libmultipath/sysfs.h
+++ b/libmultipath/sysfs.h
@@ -26,14 +26,9 @@ ssize_t sysfs_bin_attr_get_value(struct udev_device *dev, const char *attr_name,
sysfs_attr_value_ok(__rc, __l); \
})
-#define log_sysfs_attr_set_value(prio, rc, fmt, __args...) \
-do { \
- STRBUF_ON_STACK(__buf); \
- if (print_strbuf(&__buf, fmt, ##__args) >= 0 && \
- print_strbuf(&__buf, ": %s", rc < 0 ? strerror(-rc) : \
- "write underflow") >= 0) \
- condlog(prio, "%s", get_strbuf_str(&__buf)); \
-} while(0)
+#define log_sysfs_attr_set_value(prio, rc, fmt, __args...) \
+ condlog(prio, fmt ": %s", ##__args , rc < 0 ? strerror(-rc) : \
+ "write underflow")
int sysfs_get_size (struct path *pp, unsigned long long * size);
int sysfs_check_holders(char * check_devt, char * new_devt);
diff --git a/libmultipath/uevent.c b/libmultipath/uevent.c
index 57447ca0..db6da984 100644
--- a/libmultipath/uevent.c
+++ b/libmultipath/uevent.c
@@ -460,11 +460,12 @@ static void print_uevq(const char *msg, struct list_head *uevq)
{
struct uevent *uev;
int i = 0;
- STRBUF_ON_STACK(buf);
+ struct strbuf buf = STRBUF_INIT;
if (4 > MAX_VERBOSITY || 4 > libmp_verbosity)
return;
+ pthread_cleanup_push_cast(reset_strbuf, &buf);
if (list_empty(uevq))
append_strbuf_str(&buf, "*empty*");
else
@@ -473,7 +474,8 @@ static void print_uevq(const char *msg, struct list_head *uevq)
print_uev(&buf, uev);
}
- condlog(4, "uevent queue (%s): %s", msg, steal_strbuf_str(&buf));
+ condlog(4, "uevent queue (%s): %s", msg, get_strbuf_str(&buf));
+ pthread_cleanup_pop(1);
}
static void
--
2.17.2
--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [dm-devel] [PATCH 3/4] libmultipath: use regular array for field widths
2022-10-11 21:52 [dm-devel] [PATCH 0/4] remove dangerous cleanup __attribute__ uses Benjamin Marzinski
2022-10-11 21:53 ` [dm-devel] [PATCH 1/4] libmultipath: don't print garbage keywords Benjamin Marzinski
2022-10-11 21:53 ` [dm-devel] [PATCH 2/4] libmultipath: avoid STRBUF_ON_STACK with cancellation points Benjamin Marzinski
@ 2022-10-11 21:53 ` Benjamin Marzinski
2022-10-21 7:04 ` Martin Wilck
2022-10-11 21:53 ` [dm-devel] [PATCH 4/4] libmultipath: avoid cleanup __attribute__ with cancellation points Benjamin Marzinski
2022-10-20 16:03 ` [dm-devel] [PATCH 0/4] remove dangerous cleanup __attribute__ uses Martin Wilck
4 siblings, 1 reply; 11+ messages in thread
From: Benjamin Marzinski @ 2022-10-11 21:53 UTC (permalink / raw)
To: Christophe Varoqui; +Cc: device-mapper development, Martin Wilck
We know the size of these arrays, so we can just allocate them on the
stack. Also, show_path() doesn't use the width, so don't initialize it
in the first place.
Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
libmultipath/foreign.c | 5 ++--
libmultipath/libmultipath.version | 4 +--
libmultipath/print.c | 32 +++++++++++-------------
libmultipath/print.h | 4 +--
multipath/main.c | 5 ++--
multipathd/cli_handlers.c | 41 ++++++++++++-------------------
6 files changed, 38 insertions(+), 53 deletions(-)
diff --git a/libmultipath/foreign.c b/libmultipath/foreign.c
index 8981ff58..4cc2a8e3 100644
--- a/libmultipath/foreign.c
+++ b/libmultipath/foreign.c
@@ -550,10 +550,9 @@ void print_foreign_topology(int verbosity)
struct strbuf buf = STRBUF_INIT;
struct foreign *fgn;
int i;
- fieldwidth_t *width __attribute__((cleanup(cleanup_ucharp))) = NULL;
+ fieldwidth_t width[path_layout_size()];
- if ((width = alloc_path_layout()) == NULL)
- return;
+ memset(width, 0, sizeof(width));
rdlock_foreigns();
if (foreigns == NULL) {
unlock_foreigns(NULL);
diff --git a/libmultipath/libmultipath.version b/libmultipath/libmultipath.version
index 8a447f7f..af7c5ed2 100644
--- a/libmultipath/libmultipath.version
+++ b/libmultipath/libmultipath.version
@@ -38,9 +38,7 @@ global:
add_map_with_path;
adopt_paths;
alloc_multipath;
- alloc_multipath_layout;
alloc_path;
- alloc_path_layout;
alloc_path_with_pathinfo;
change_foreign;
check_alias_settings;
@@ -126,6 +124,7 @@ global:
libmultipath_exit;
libmultipath_init;
load_config;
+ multipath_layout_size;
need_io_err_check;
orphan_path;
parse_prkey_flags;
@@ -133,6 +132,7 @@ global:
path_discovery;
path_get_tpgs;
pathinfo;
+ path_layout_size;
path_offline;
print_all_paths;
print_foreign_topology;
diff --git a/libmultipath/print.c b/libmultipath/print.c
index 97f9a177..87d6a329 100644
--- a/libmultipath/print.c
+++ b/libmultipath/print.c
@@ -805,6 +805,12 @@ static const struct multipath_data mpd[] = {
{'g', "vpd page data", snprint_multipath_vpd_data},
};
+
+int multipath_layout_size(void)
+{
+ return ARRAY_SIZE(mpd);
+}
+
static const struct path_data pd[] = {
{'w', "uuid", snprint_path_uuid},
{'i', "hcil", snprint_hcil},
@@ -834,6 +840,11 @@ static const struct path_data pd[] = {
{'L', "LUN hex", snprint_path_lunhex},
};
+int path_layout_size(void)
+{
+ return ARRAY_SIZE(pd);
+}
+
static const struct pathgroup_data pgd[] = {
{'s', "selector", snprint_pg_selector},
{'p', "pri", snprint_pg_pri},
@@ -871,10 +882,6 @@ int snprint_wildcards(struct strbuf *buff)
return get_strbuf_len(buff) - initial_len;
}
-fieldwidth_t *alloc_path_layout(void) {
- return calloc(ARRAY_SIZE(pd), sizeof(fieldwidth_t));
-}
-
void get_path_layout(vector pathvec, int header, fieldwidth_t *width)
{
vector gpvec = vector_convert(NULL, pathvec, struct path,
@@ -929,11 +936,6 @@ void _get_path_layout (const struct _vector *gpvec, enum layout_reset reset,
}
}
-fieldwidth_t *alloc_multipath_layout(void) {
-
- return calloc(ARRAY_SIZE(mpd), sizeof(fieldwidth_t));
-}
-
void get_multipath_layout (vector mpvec, int header, fieldwidth_t *width) {
vector gmvec = vector_convert(NULL, mpvec, struct multipath,
dm_multipath_to_gen);
@@ -1187,12 +1189,11 @@ int _snprint_pathgroup(const struct gen_pathgroup *ggp, struct strbuf *line,
void _print_multipath_topology(const struct gen_multipath *gmp, int verbosity)
{
struct strbuf buff = STRBUF_INIT;
- fieldwidth_t *p_width __attribute__((cleanup(cleanup_ucharp))) = NULL;
+ fieldwidth_t p_width[ARRAY_SIZE(pd)] = {0};
const struct gen_pathgroup *gpg;
const struct _vector *pgvec, *pathvec;
int j;
- p_width = alloc_path_layout();
pgvec = gmp->ops->get_pathgroups(gmp);
if (pgvec != NULL) {
@@ -1236,14 +1237,11 @@ int _snprint_multipath_topology(const struct gen_multipath *gmp,
const struct gen_pathgroup *gpg;
struct strbuf style = STRBUF_INIT;
size_t initial_len = get_strbuf_len(buff);
- fieldwidth_t *width __attribute__((cleanup(cleanup_ucharp))) = NULL;
+ fieldwidth_t width[ARRAY_SIZE(mpd)] = {0};
if (verbosity <= 0)
return 0;
- if ((width = alloc_multipath_layout()) == NULL)
- return -ENOMEM;
-
if (verbosity == 1)
return _snprint_multipath(gmp, buff, "%n", width);
@@ -2027,7 +2025,7 @@ static void print_all_paths_custo(vector pathvec, int banner, const char *fmt)
int i;
struct path * pp;
struct strbuf line = STRBUF_INIT;
- fieldwidth_t *width __attribute__((cleanup(cleanup_ucharp))) = NULL;
+ fieldwidth_t width[ARRAY_SIZE(pd)] = {0};
if (!VECTOR_SIZE(pathvec)) {
if (banner)
@@ -2035,8 +2033,6 @@ static void print_all_paths_custo(vector pathvec, int banner, const char *fmt)
return;
}
- if ((width = alloc_path_layout()) == NULL)
- return;
get_path_layout(pathvec, 1, width);
pthread_cleanup_push_cast(reset_strbuf, &line);
diff --git a/libmultipath/print.h b/libmultipath/print.h
index 52f5b256..4e50827d 100644
--- a/libmultipath/print.h
+++ b/libmultipath/print.h
@@ -16,11 +16,11 @@ enum layout_reset {
};
/* fieldwidth_t is defined in generic.h */
-fieldwidth_t *alloc_path_layout(void);
+int multipath_layout_size(void);
+int path_layout_size(void);
void _get_path_layout (const struct _vector *gpvec, enum layout_reset,
fieldwidth_t *width);
void get_path_layout (vector pathvec, int header, fieldwidth_t *width);
-fieldwidth_t *alloc_multipath_layout(void);
void _get_multipath_layout (const struct _vector *gmvec, enum layout_reset,
fieldwidth_t *width);
void get_multipath_layout (vector mpvec, int header, fieldwidth_t *width);
diff --git a/multipath/main.c b/multipath/main.c
index 7b69a3ce..f4c85409 100644
--- a/multipath/main.c
+++ b/multipath/main.c
@@ -457,7 +457,7 @@ configure (struct config *conf, enum mpath_cmds cmd,
int di_flag = 0;
char * refwwid = NULL;
char * dev = NULL;
- fieldwidth_t *width __attribute__((cleanup(cleanup_ucharp))) = NULL;
+ fieldwidth_t width[path_layout_size()];
/*
* allocate core vectors to store paths and multipaths
@@ -544,8 +544,7 @@ configure (struct config *conf, enum mpath_cmds cmd,
if (libmp_verbosity > 2)
print_all_paths(pathvec, 1);
- if ((width = alloc_path_layout()) == NULL)
- goto out;
+ memset(width, 0, sizeof(width));
get_path_layout(pathvec, 0, width);
foreign_path_layout(width);
diff --git a/multipathd/cli_handlers.c b/multipathd/cli_handlers.c
index 5b8f647b..ddc807a1 100644
--- a/multipathd/cli_handlers.c
+++ b/multipathd/cli_handlers.c
@@ -38,11 +38,10 @@ show_paths (struct strbuf *reply, struct vectors *vecs, char *style, int pretty)
int i;
struct path * pp;
int hdr_len = 0;
- fieldwidth_t *width __attribute__((cleanup(cleanup_ucharp))) = NULL;
+ fieldwidth_t width[path_layout_size()];
+ memset(width, 0, sizeof(width));
if (pretty) {
- if ((width = alloc_path_layout()) == NULL)
- return 1;
get_path_layout(vecs->pathvec, 1, width);
foreign_path_layout(width);
}
@@ -50,10 +49,10 @@ show_paths (struct strbuf *reply, struct vectors *vecs, char *style, int pretty)
return 1;
vector_foreach_slot(vecs->pathvec, pp, i) {
- if (snprint_path(reply, style, pp, width) < 0)
+ if (snprint_path(reply, style, pp, pretty? width : NULL) < 0)
return 1;
}
- if (snprint_foreign_paths(reply, style, width) < 0)
+ if (snprint_foreign_paths(reply, style, pretty? width : NULL) < 0)
return 1;
if (pretty && get_strbuf_len(reply) == (size_t)hdr_len)
@@ -67,12 +66,7 @@ static int
show_path (struct strbuf *reply, struct vectors *vecs, struct path *pp,
char *style)
{
- fieldwidth_t *width __attribute__((cleanup(cleanup_ucharp))) = NULL;
-
- if ((width = alloc_path_layout()) == NULL)
- return 1;
- get_path_layout(vecs->pathvec, 1, width);
- if (snprint_path(reply, style, pp, 0) < 0)
+ if (snprint_path(reply, style, pp, NULL) < 0)
return 1;
return 0;
}
@@ -95,10 +89,9 @@ show_maps_topology (struct strbuf *reply, struct vectors * vecs)
{
int i;
struct multipath * mpp;
- fieldwidth_t *p_width __attribute__((cleanup(cleanup_ucharp))) = NULL;
+ fieldwidth_t p_width[path_layout_size()];
- if ((p_width = alloc_path_layout()) == NULL)
- return 1;
+ memset(p_width, 0, sizeof(p_width));
get_path_layout(vecs->pathvec, 0, p_width);
foreign_path_layout(p_width);
@@ -258,10 +251,9 @@ cli_list_map_topology (void *v, struct strbuf *reply, void *data)
struct multipath * mpp;
struct vectors * vecs = (struct vectors *)data;
char * param = get_keyparam(v, MAP);
- fieldwidth_t *p_width __attribute__((cleanup(cleanup_ucharp))) = NULL;
+ fieldwidth_t p_width[path_layout_size()];
- if ((p_width = alloc_path_layout()) == NULL)
- return 1;
+ memset(p_width, 0, sizeof(p_width));
get_path_layout(vecs->pathvec, 0, p_width);
param = convert_dev(param, 0);
mpp = find_mp_by_str(vecs->mpvec, param);
@@ -357,11 +349,10 @@ show_maps (struct strbuf *reply, struct vectors *vecs, char *style,
int i;
struct multipath * mpp;
int hdr_len = 0;
- fieldwidth_t *width __attribute__((cleanup(cleanup_ucharp))) = NULL;
+ fieldwidth_t width[multipath_layout_size()];
+ memset(width, 0, sizeof(width));
if (pretty) {
- if ((width = alloc_multipath_layout()) == NULL)
- return 1;
get_multipath_layout(vecs->mpvec, 1, width);
foreign_multipath_layout(width);
}
@@ -374,10 +365,11 @@ show_maps (struct strbuf *reply, struct vectors *vecs, char *style,
i--;
continue;
}
- if (snprint_multipath(reply, style, mpp, width) < 0)
+ if (snprint_multipath(reply, style, mpp,
+ pretty? width : NULL) < 0)
return 1;
}
- if (snprint_foreign_multipaths(reply, style, width) < 0)
+ if (snprint_foreign_multipaths(reply, style, pretty? width : NULL) < 0)
return 1;
if (pretty && get_strbuf_len(reply) == (size_t)hdr_len)
@@ -416,10 +408,9 @@ cli_list_map_fmt (void *v, struct strbuf *reply, void *data)
struct vectors * vecs = (struct vectors *)data;
char * param = get_keyparam(v, MAP);
char * fmt = get_keyparam(v, FMT);
- fieldwidth_t *width __attribute__((cleanup(cleanup_ucharp))) = NULL;
+ fieldwidth_t width[multipath_layout_size()];
- if ((width = alloc_multipath_layout()) == NULL)
- return 1;
+ memset(width, 0, sizeof(width));
get_multipath_layout(vecs->mpvec, 1, width);
param = convert_dev(param, 0);
mpp = find_mp_by_str(vecs->mpvec, param);
--
2.17.2
--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [dm-devel] [PATCH 4/4] libmultipath: avoid cleanup __attribute__ with cancellation points
2022-10-11 21:52 [dm-devel] [PATCH 0/4] remove dangerous cleanup __attribute__ uses Benjamin Marzinski
` (2 preceding siblings ...)
2022-10-11 21:53 ` [dm-devel] [PATCH 3/4] libmultipath: use regular array for field widths Benjamin Marzinski
@ 2022-10-11 21:53 ` Benjamin Marzinski
2022-10-20 16:03 ` [dm-devel] [PATCH 0/4] remove dangerous cleanup __attribute__ uses Martin Wilck
4 siblings, 0 replies; 11+ messages in thread
From: Benjamin Marzinski @ 2022-10-11 21:53 UTC (permalink / raw)
To: Christophe Varoqui; +Cc: device-mapper development, Martin Wilck
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@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
--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [dm-devel] [PATCH 0/4] remove dangerous cleanup __attribute__ uses
2022-10-11 21:52 [dm-devel] [PATCH 0/4] remove dangerous cleanup __attribute__ uses Benjamin Marzinski
` (3 preceding siblings ...)
2022-10-11 21:53 ` [dm-devel] [PATCH 4/4] libmultipath: avoid cleanup __attribute__ with cancellation points Benjamin Marzinski
@ 2022-10-20 16:03 ` Martin Wilck
2022-10-20 22:57 ` Benjamin Marzinski
4 siblings, 1 reply; 11+ messages in thread
From: Martin Wilck @ 2022-10-20 16:03 UTC (permalink / raw)
To: bmarzins, christophe.varoqui; +Cc: dm-devel
On Tue, 2022-10-11 at 16:52 -0500, Benjamin Marzinski wrote:
> the cleanup __attribute__ is only run when a variable goes out of
> scope
> normally. It is not run on pthread cancellation. This means that
> multipathd could leak whatever resources were supposed to be cleaned
> up
> if the thread was cancelled in a function using variables with the
> cleanup __attribute__. This patchset removes all these uses in cases
> where the code is run by multipathd and includes a cancellation point
> in the variables scope (usually condlog(), which calls fprintf(), a
> cancellation point, the way multipathd is usually run).
I have to say I don't like this.
Cleaning up resources is certainly very important, but in most of
cases it's only about freeing memory on exit: memory which is going to
be freed by the system anyway when multipathd terminates. Resource
cleanup matters much more during runtime than on exit. The only threads
that are cancelled during multipathd runtime are the TUR threads.
It's nice to have valgrind show zero leaks when we kill multipathd out
if the sudden. But we should weigh this against the side effects it
has, which is ugly, hard-to-maintain code.
pthread_cleanup_push()/pop() calls contribute a lot to the bad
readability and maintainability of much of the multipath code base, not
to mention the weird errors some gcc versions throw for
pthread_cleanup() constructs. I'd rather try to get rid of as much of
them as we can. I know it won't be possible everywhere, because some
resources (like file descriptors) really need to be cleaned up, but I
am really unsure whether we need pthread cleanup for every free()
operation.
__attribute__((cleanup())), on the contrary, goes a long way to make
code more readable and easier to review. It actually helps a lot to
ensure resources are properly cleaned up, considering how easily a
"goto cleanup;" statement may be forgotten. Replacing it by
pthread_cleanup() and goto statements is undesirable, IMO.
If your concern is really condlog(), let's discuss if we can find a way
to convert this such that it is no cancellation point any more. I can
imagine converting the locking in log_safe() from a mutex into some
lockless scheme, using atomic variables, and using the log thread not
only for syslog, but also for the fprintf() case.
Regards
Martin
>
> Benjamin Marzinski (4):
> libmultipath: don't print garbage keywords
> libmultipath: avoid STRBUF_ON_STACK with cancellation points
> libmultipath: use regular array for field widths
> libmultipath: avoid cleanup __attribute__ with cancellation points
>
> libmpathutil/parser.c | 13 ++--
> libmpathutil/strbuf.h | 4 +-
> libmultipath/alias.c | 59 ++++++++++-------
> libmultipath/blacklist.c | 4 +-
> libmultipath/configure.c | 6 +-
> libmultipath/discovery.c | 34 ++++++----
> libmultipath/dmparser.c | 23 +++----
> libmultipath/foreign.c | 9 +--
> libmultipath/generic.c | 14 ++--
> libmultipath/libmultipath.version | 4 +-
> libmultipath/print.c | 82 ++++++++++++++--------
> --
> libmultipath/print.h | 4 +-
> libmultipath/prioritizers/weightedpath.c | 22 ++++---
> libmultipath/propsel.c | 76 ++++++++++++++++------
> libmultipath/sysfs.h | 11 +---
> libmultipath/uevent.c | 6 +-
> multipath/main.c | 5 +-
> multipathd/cli_handlers.c | 52 +++++++--------
> multipathd/main.c | 49 ++++++++------
> 19 files changed, 286 insertions(+), 191 deletions(-)
>
--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [dm-devel] [PATCH 0/4] remove dangerous cleanup __attribute__ uses
2022-10-20 16:03 ` [dm-devel] [PATCH 0/4] remove dangerous cleanup __attribute__ uses Martin Wilck
@ 2022-10-20 22:57 ` Benjamin Marzinski
0 siblings, 0 replies; 11+ messages in thread
From: Benjamin Marzinski @ 2022-10-20 22:57 UTC (permalink / raw)
To: Martin Wilck; +Cc: dm-devel
On Thu, Oct 20, 2022 at 04:03:46PM +0000, Martin Wilck wrote:
> On Tue, 2022-10-11 at 16:52 -0500, Benjamin Marzinski wrote:
> > the cleanup __attribute__ is only run when a variable goes out of
> > scope
> > normally. It is not run on pthread cancellation. This means that
> > multipathd could leak whatever resources were supposed to be cleaned
> > up
> > if the thread was cancelled in a function using variables with the
> > cleanup __attribute__. This patchset removes all these uses in cases
> > where the code is run by multipathd and includes a cancellation point
> > in the variables scope (usually condlog(), which calls fprintf(), a
> > cancellation point, the way multipathd is usually run).
>
> I have to say I don't like this.
>
> Cleaning up resources is certainly very important, but in most of
> cases it's only about freeing memory on exit: memory which is going to
> be freed by the system anyway when multipathd terminates. Resource
> cleanup matters much more during runtime than on exit. The only threads
> that are cancelled during multipathd runtime are the TUR threads.
> It's nice to have valgrind show zero leaks when we kill multipathd out
> if the sudden. But we should weigh this against the side effects it
> has, which is ugly, hard-to-maintain code.
>
> pthread_cleanup_push()/pop() calls contribute a lot to the bad
> readability and maintainability of much of the multipath code base, not
> to mention the weird errors some gcc versions throw for
> pthread_cleanup() constructs. I'd rather try to get rid of as much of
> them as we can. I know it won't be possible everywhere, because some
> resources (like file descriptors) really need to be cleaned up, but I
> am really unsure whether we need pthread cleanup for every free()
> operation.
>
> __attribute__((cleanup())), on the contrary, goes a long way to make
> code more readable and easier to review. It actually helps a lot to
> ensure resources are properly cleaned up, considering how easily a
> "goto cleanup;" statement may be forgotten. Replacing it by
> pthread_cleanup() and goto statements is undesirable, IMO.
>
> If your concern is really condlog(), let's discuss if we can find a way
> to convert this such that it is no cancellation point any more. I can
> imagine converting the locking in log_safe() from a mutex into some
> lockless scheme, using atomic variables, and using the log thread not
> only for syslog, but also for the fprintf() case.
Actually, I've never been a huge fan of all the work that we do to keep
multipathd from leaking memory when it's killed, but I though that was
the goal. If you don't care about these leaks, then I'm fine with
ignoring them. How about just taking the first and third patches?
libmultipath: don't print garbage keywords
libmultipath: use regular array for field widths
-Ben
> Regards
> Martin
>
>
> >
> > Benjamin Marzinski (4):
> > libmultipath: don't print garbage keywords
> > libmultipath: avoid STRBUF_ON_STACK with cancellation points
> > libmultipath: use regular array for field widths
> > libmultipath: avoid cleanup __attribute__ with cancellation points
> >
> > libmpathutil/parser.c | 13 ++--
> > libmpathutil/strbuf.h | 4 +-
> > libmultipath/alias.c | 59 ++++++++++-------
> > libmultipath/blacklist.c | 4 +-
> > libmultipath/configure.c | 6 +-
> > libmultipath/discovery.c | 34 ++++++----
> > libmultipath/dmparser.c | 23 +++----
> > libmultipath/foreign.c | 9 +--
> > libmultipath/generic.c | 14 ++--
> > libmultipath/libmultipath.version | 4 +-
> > libmultipath/print.c | 82 ++++++++++++++--------
> > --
> > libmultipath/print.h | 4 +-
> > libmultipath/prioritizers/weightedpath.c | 22 ++++---
> > libmultipath/propsel.c | 76 ++++++++++++++++------
> > libmultipath/sysfs.h | 11 +---
> > libmultipath/uevent.c | 6 +-
> > multipath/main.c | 5 +-
> > multipathd/cli_handlers.c | 52 +++++++--------
> > multipathd/main.c | 49 ++++++++------
> > 19 files changed, 286 insertions(+), 191 deletions(-)
> >
--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [dm-devel] [PATCH 3/4] libmultipath: use regular array for field widths
2022-10-11 21:53 ` [dm-devel] [PATCH 3/4] libmultipath: use regular array for field widths Benjamin Marzinski
@ 2022-10-21 7:04 ` Martin Wilck
2022-10-21 17:51 ` Benjamin Marzinski
0 siblings, 1 reply; 11+ messages in thread
From: Martin Wilck @ 2022-10-21 7:04 UTC (permalink / raw)
To: bmarzins, christophe.varoqui; +Cc: dm-devel
On Tue, 2022-10-11 at 16:53 -0500, Benjamin Marzinski wrote:
> We know the size of these arrays, so we can just allocate them on the
> stack. Also, show_path() doesn't use the width, so don't initialize
> it
> in the first place.
>
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
This isn't wrong, but I'm not sure what it actually achieves except a
few less NULL checks (I'm sure you're aware that this doesn't mean
better protection against out-of-memory situations). It comes at the
cost of an ABI change. I understand that the intention is to eliminate
__attribute__((cleanup())). But if we agree we don't want to do that
everywhere, I see no particular reason to do it in this code path.
I'm not totally against it, but I'm not enthusiastic, either.
Regards,
Martin
> ---
> libmultipath/foreign.c | 5 ++--
> libmultipath/libmultipath.version | 4 +--
> libmultipath/print.c | 32 +++++++++++-------------
> libmultipath/print.h | 4 +--
> multipath/main.c | 5 ++--
> multipathd/cli_handlers.c | 41 ++++++++++++-----------------
> --
> 6 files changed, 38 insertions(+), 53 deletions(-)
>
> diff --git a/libmultipath/foreign.c b/libmultipath/foreign.c
> index 8981ff58..4cc2a8e3 100644
> --- a/libmultipath/foreign.c
> +++ b/libmultipath/foreign.c
> @@ -550,10 +550,9 @@ void print_foreign_topology(int verbosity)
> struct strbuf buf = STRBUF_INIT;
> struct foreign *fgn;
> int i;
> - fieldwidth_t *width __attribute__((cleanup(cleanup_ucharp)))
> = NULL;
> + fieldwidth_t width[path_layout_size()];
>
> - if ((width = alloc_path_layout()) == NULL)
> - return;
> + memset(width, 0, sizeof(width));
> rdlock_foreigns();
> if (foreigns == NULL) {
> unlock_foreigns(NULL);
> diff --git a/libmultipath/libmultipath.version
> b/libmultipath/libmultipath.version
> index 8a447f7f..af7c5ed2 100644
> --- a/libmultipath/libmultipath.version
> +++ b/libmultipath/libmultipath.version
> @@ -38,9 +38,7 @@ global:
> add_map_with_path;
> adopt_paths;
> alloc_multipath;
> - alloc_multipath_layout;
> alloc_path;
> - alloc_path_layout;
> alloc_path_with_pathinfo;
> change_foreign;
> check_alias_settings;
> @@ -126,6 +124,7 @@ global:
> libmultipath_exit;
> libmultipath_init;
> load_config;
> + multipath_layout_size;
> need_io_err_check;
> orphan_path;
> parse_prkey_flags;
> @@ -133,6 +132,7 @@ global:
> path_discovery;
> path_get_tpgs;
> pathinfo;
> + path_layout_size;
> path_offline;
> print_all_paths;
> print_foreign_topology;
> diff --git a/libmultipath/print.c b/libmultipath/print.c
> index 97f9a177..87d6a329 100644
> --- a/libmultipath/print.c
> +++ b/libmultipath/print.c
> @@ -805,6 +805,12 @@ static const struct multipath_data mpd[] = {
> {'g', "vpd page data", snprint_multipath_vpd_data},
> };
>
> +
> +int multipath_layout_size(void)
> +{
> + return ARRAY_SIZE(mpd);
> +}
> +
> static const struct path_data pd[] = {
> {'w', "uuid", snprint_path_uuid},
> {'i', "hcil", snprint_hcil},
> @@ -834,6 +840,11 @@ static const struct path_data pd[] = {
> {'L', "LUN hex", snprint_path_lunhex},
> };
>
> +int path_layout_size(void)
> +{
> + return ARRAY_SIZE(pd);
> +}
> +
> static const struct pathgroup_data pgd[] = {
> {'s', "selector", snprint_pg_selector},
> {'p', "pri", snprint_pg_pri},
> @@ -871,10 +882,6 @@ int snprint_wildcards(struct strbuf *buff)
> return get_strbuf_len(buff) - initial_len;
> }
>
> -fieldwidth_t *alloc_path_layout(void) {
> - return calloc(ARRAY_SIZE(pd), sizeof(fieldwidth_t));
> -}
> -
> void get_path_layout(vector pathvec, int header, fieldwidth_t
> *width)
> {
> vector gpvec = vector_convert(NULL, pathvec, struct path,
> @@ -929,11 +936,6 @@ void _get_path_layout (const struct _vector
> *gpvec, enum layout_reset reset,
> }
> }
>
> -fieldwidth_t *alloc_multipath_layout(void) {
> -
> - return calloc(ARRAY_SIZE(mpd), sizeof(fieldwidth_t));
> -}
> -
> void get_multipath_layout (vector mpvec, int header, fieldwidth_t
> *width) {
> vector gmvec = vector_convert(NULL, mpvec, struct multipath,
> dm_multipath_to_gen);
> @@ -1187,12 +1189,11 @@ int _snprint_pathgroup(const struct
> gen_pathgroup *ggp, struct strbuf *line,
> void _print_multipath_topology(const struct gen_multipath *gmp, int
> verbosity)
> {
> struct strbuf buff = STRBUF_INIT;
> - fieldwidth_t *p_width
> __attribute__((cleanup(cleanup_ucharp))) = NULL;
> + fieldwidth_t p_width[ARRAY_SIZE(pd)] = {0};
> const struct gen_pathgroup *gpg;
> const struct _vector *pgvec, *pathvec;
> int j;
>
> - p_width = alloc_path_layout();
> pgvec = gmp->ops->get_pathgroups(gmp);
>
> if (pgvec != NULL) {
> @@ -1236,14 +1237,11 @@ int _snprint_multipath_topology(const struct
> gen_multipath *gmp,
> const struct gen_pathgroup *gpg;
> struct strbuf style = STRBUF_INIT;
> size_t initial_len = get_strbuf_len(buff);
> - fieldwidth_t *width __attribute__((cleanup(cleanup_ucharp)))
> = NULL;
> + fieldwidth_t width[ARRAY_SIZE(mpd)] = {0};
>
> if (verbosity <= 0)
> return 0;
>
> - if ((width = alloc_multipath_layout()) == NULL)
> - return -ENOMEM;
> -
> if (verbosity == 1)
> return _snprint_multipath(gmp, buff, "%n", width);
>
> @@ -2027,7 +2025,7 @@ static void print_all_paths_custo(vector
> pathvec, int banner, const char *fmt)
> int i;
> struct path * pp;
> struct strbuf line = STRBUF_INIT;
> - fieldwidth_t *width __attribute__((cleanup(cleanup_ucharp)))
> = NULL;
> + fieldwidth_t width[ARRAY_SIZE(pd)] = {0};
>
> if (!VECTOR_SIZE(pathvec)) {
> if (banner)
> @@ -2035,8 +2033,6 @@ static void print_all_paths_custo(vector
> pathvec, int banner, const char *fmt)
> return;
> }
>
> - if ((width = alloc_path_layout()) == NULL)
> - return;
> get_path_layout(pathvec, 1, width);
>
> pthread_cleanup_push_cast(reset_strbuf, &line);
> diff --git a/libmultipath/print.h b/libmultipath/print.h
> index 52f5b256..4e50827d 100644
> --- a/libmultipath/print.h
> +++ b/libmultipath/print.h
> @@ -16,11 +16,11 @@ enum layout_reset {
> };
>
> /* fieldwidth_t is defined in generic.h */
> -fieldwidth_t *alloc_path_layout(void);
> +int multipath_layout_size(void);
> +int path_layout_size(void);
> void _get_path_layout (const struct _vector *gpvec, enum
> layout_reset,
> fieldwidth_t *width);
> void get_path_layout (vector pathvec, int header, fieldwidth_t
> *width);
> -fieldwidth_t *alloc_multipath_layout(void);
> void _get_multipath_layout (const struct _vector *gmvec, enum
> layout_reset,
> fieldwidth_t *width);
> void get_multipath_layout (vector mpvec, int header, fieldwidth_t
> *width);
> diff --git a/multipath/main.c b/multipath/main.c
> index 7b69a3ce..f4c85409 100644
> --- a/multipath/main.c
> +++ b/multipath/main.c
> @@ -457,7 +457,7 @@ configure (struct config *conf, enum mpath_cmds
> cmd,
> int di_flag = 0;
> char * refwwid = NULL;
> char * dev = NULL;
> - fieldwidth_t *width __attribute__((cleanup(cleanup_ucharp)))
> = NULL;
> + fieldwidth_t width[path_layout_size()];
>
> /*
> * allocate core vectors to store paths and multipaths
> @@ -544,8 +544,7 @@ configure (struct config *conf, enum mpath_cmds
> cmd,
> if (libmp_verbosity > 2)
> print_all_paths(pathvec, 1);
>
> - if ((width = alloc_path_layout()) == NULL)
> - goto out;
> + memset(width, 0, sizeof(width));
> get_path_layout(pathvec, 0, width);
> foreign_path_layout(width);
>
> diff --git a/multipathd/cli_handlers.c b/multipathd/cli_handlers.c
> index 5b8f647b..ddc807a1 100644
> --- a/multipathd/cli_handlers.c
> +++ b/multipathd/cli_handlers.c
> @@ -38,11 +38,10 @@ show_paths (struct strbuf *reply, struct vectors
> *vecs, char *style, int pretty)
> int i;
> struct path * pp;
> int hdr_len = 0;
> - fieldwidth_t *width __attribute__((cleanup(cleanup_ucharp)))
> = NULL;
> + fieldwidth_t width[path_layout_size()];
>
> + memset(width, 0, sizeof(width));
> if (pretty) {
> - if ((width = alloc_path_layout()) == NULL)
> - return 1;
> get_path_layout(vecs->pathvec, 1, width);
> foreign_path_layout(width);
> }
> @@ -50,10 +49,10 @@ show_paths (struct strbuf *reply, struct vectors
> *vecs, char *style, int pretty)
> return 1;
>
> vector_foreach_slot(vecs->pathvec, pp, i) {
> - if (snprint_path(reply, style, pp, width) < 0)
> + if (snprint_path(reply, style, pp, pretty? width :
> NULL) < 0)
> return 1;
> }
> - if (snprint_foreign_paths(reply, style, width) < 0)
> + if (snprint_foreign_paths(reply, style, pretty? width : NULL)
> < 0)
> return 1;
>
> if (pretty && get_strbuf_len(reply) == (size_t)hdr_len)
> @@ -67,12 +66,7 @@ static int
> show_path (struct strbuf *reply, struct vectors *vecs, struct path
> *pp,
> char *style)
> {
> - fieldwidth_t *width __attribute__((cleanup(cleanup_ucharp)))
> = NULL;
> -
> - if ((width = alloc_path_layout()) == NULL)
> - return 1;
> - get_path_layout(vecs->pathvec, 1, width);
> - if (snprint_path(reply, style, pp, 0) < 0)
> + if (snprint_path(reply, style, pp, NULL) < 0)
> return 1;
> return 0;
> }
> @@ -95,10 +89,9 @@ show_maps_topology (struct strbuf *reply, struct
> vectors * vecs)
> {
> int i;
> struct multipath * mpp;
> - fieldwidth_t *p_width
> __attribute__((cleanup(cleanup_ucharp))) = NULL;
> + fieldwidth_t p_width[path_layout_size()];
>
> - if ((p_width = alloc_path_layout()) == NULL)
> - return 1;
> + memset(p_width, 0, sizeof(p_width));
> get_path_layout(vecs->pathvec, 0, p_width);
> foreign_path_layout(p_width);
>
> @@ -258,10 +251,9 @@ cli_list_map_topology (void *v, struct strbuf
> *reply, void *data)
> struct multipath * mpp;
> struct vectors * vecs = (struct vectors *)data;
> char * param = get_keyparam(v, MAP);
> - fieldwidth_t *p_width
> __attribute__((cleanup(cleanup_ucharp))) = NULL;
> + fieldwidth_t p_width[path_layout_size()];
>
> - if ((p_width = alloc_path_layout()) == NULL)
> - return 1;
> + memset(p_width, 0, sizeof(p_width));
> get_path_layout(vecs->pathvec, 0, p_width);
> param = convert_dev(param, 0);
> mpp = find_mp_by_str(vecs->mpvec, param);
> @@ -357,11 +349,10 @@ show_maps (struct strbuf *reply, struct vectors
> *vecs, char *style,
> int i;
> struct multipath * mpp;
> int hdr_len = 0;
> - fieldwidth_t *width __attribute__((cleanup(cleanup_ucharp)))
> = NULL;
> + fieldwidth_t width[multipath_layout_size()];
>
> + memset(width, 0, sizeof(width));
> if (pretty) {
> - if ((width = alloc_multipath_layout()) == NULL)
> - return 1;
> get_multipath_layout(vecs->mpvec, 1, width);
> foreign_multipath_layout(width);
> }
> @@ -374,10 +365,11 @@ show_maps (struct strbuf *reply, struct vectors
> *vecs, char *style,
> i--;
> continue;
> }
> - if (snprint_multipath(reply, style, mpp, width) < 0)
> + if (snprint_multipath(reply, style, mpp,
> + pretty? width : NULL) < 0)
> return 1;
> }
> - if (snprint_foreign_multipaths(reply, style, width) < 0)
> + if (snprint_foreign_multipaths(reply, style, pretty? width :
> NULL) < 0)
> return 1;
>
> if (pretty && get_strbuf_len(reply) == (size_t)hdr_len)
> @@ -416,10 +408,9 @@ cli_list_map_fmt (void *v, struct strbuf *reply,
> void *data)
> struct vectors * vecs = (struct vectors *)data;
> char * param = get_keyparam(v, MAP);
> char * fmt = get_keyparam(v, FMT);
> - fieldwidth_t *width __attribute__((cleanup(cleanup_ucharp)))
> = NULL;
> + fieldwidth_t width[multipath_layout_size()];
>
> - if ((width = alloc_multipath_layout()) == NULL)
> - return 1;
> + memset(width, 0, sizeof(width));
> get_multipath_layout(vecs->mpvec, 1, width);
> param = convert_dev(param, 0);
> mpp = find_mp_by_str(vecs->mpvec, param);
--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [dm-devel] [PATCH 1/4] libmultipath: don't print garbage keywords
2022-10-11 21:53 ` [dm-devel] [PATCH 1/4] libmultipath: don't print garbage keywords Benjamin Marzinski
@ 2022-10-21 7:05 ` Martin Wilck
0 siblings, 0 replies; 11+ messages in thread
From: Martin Wilck @ 2022-10-21 7:05 UTC (permalink / raw)
To: bmarzins, christophe.varoqui; +Cc: dm-devel
On Tue, 2022-10-11 at 16:53 -0500, Benjamin Marzinski wrote:
> If snprint_keyword() failed to correctly set up sbuf, don't print it.
> Instead, return an error.
>
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
Reviewed-by: Martin Wilck <mwilck@suse.com>
> ---
> libmpathutil/parser.c | 8 +++++---
> 1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/libmpathutil/parser.c b/libmpathutil/parser.c
> index 014d9b83..8d3ac53a 100644
> --- a/libmpathutil/parser.c
> +++ b/libmpathutil/parser.c
> @@ -152,7 +152,7 @@ int
> snprint_keyword(struct strbuf *buff, const char *fmt, struct keyword
> *kw,
> const void *data)
> {
> - int r;
> + int r = 0;
> char *f;
> struct config *conf;
> STRBUF_ON_STACK(sbuf);
> @@ -190,8 +190,10 @@ snprint_keyword(struct strbuf *buff, const char
> *fmt, struct keyword *kw,
> }
> } while (*fmt++);
> out:
> - return __append_strbuf_str(buff, get_strbuf_str(&sbuf),
> - get_strbuf_len(&sbuf));
> + if (r >= 0)
> + r = __append_strbuf_str(buff, get_strbuf_str(&sbuf),
> + get_strbuf_len(&sbuf));
> + return r;
> }
>
> static const char quote_marker[] = { '\0', '"', '\0' };
--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [dm-devel] [PATCH 3/4] libmultipath: use regular array for field widths
2022-10-21 7:04 ` Martin Wilck
@ 2022-10-21 17:51 ` Benjamin Marzinski
2022-10-21 20:12 ` Martin Wilck
0 siblings, 1 reply; 11+ messages in thread
From: Benjamin Marzinski @ 2022-10-21 17:51 UTC (permalink / raw)
To: Martin Wilck; +Cc: dm-devel
On Fri, Oct 21, 2022 at 07:04:55AM +0000, Martin Wilck wrote:
> On Tue, 2022-10-11 at 16:53 -0500, Benjamin Marzinski wrote:
> > We know the size of these arrays, so we can just allocate them on the
> > stack. Also, show_path() doesn't use the width, so don't initialize
> > it
> > in the first place.
> >
> > Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
>
> This isn't wrong, but I'm not sure what it actually achieves except a
> few less NULL checks (I'm sure you're aware that this doesn't mean
> better protection against out-of-memory situations). It comes at the
> cost of an ABI change. I understand that the intention is to eliminate
> __attribute__((cleanup())). But if we agree we don't want to do that
> everywhere, I see no particular reason to do it in this code path.
>
> I'm not totally against it, but I'm not enthusiastic, either.
That's fine. How about I send a patch to just fix show_path().
-Ben
>
> Regards,
> Martin
>
>
>
> > ---
> > libmultipath/foreign.c | 5 ++--
> > libmultipath/libmultipath.version | 4 +--
> > libmultipath/print.c | 32 +++++++++++-------------
> > libmultipath/print.h | 4 +--
> > multipath/main.c | 5 ++--
> > multipathd/cli_handlers.c | 41 ++++++++++++-----------------
> > --
> > 6 files changed, 38 insertions(+), 53 deletions(-)
> >
> > diff --git a/libmultipath/foreign.c b/libmultipath/foreign.c
> > index 8981ff58..4cc2a8e3 100644
> > --- a/libmultipath/foreign.c
> > +++ b/libmultipath/foreign.c
> > @@ -550,10 +550,9 @@ void print_foreign_topology(int verbosity)
> > struct strbuf buf = STRBUF_INIT;
> > struct foreign *fgn;
> > int i;
> > - fieldwidth_t *width __attribute__((cleanup(cleanup_ucharp)))
> > = NULL;
> > + fieldwidth_t width[path_layout_size()];
> >
> > - if ((width = alloc_path_layout()) == NULL)
> > - return;
> > + memset(width, 0, sizeof(width));
> > rdlock_foreigns();
> > if (foreigns == NULL) {
> > unlock_foreigns(NULL);
> > diff --git a/libmultipath/libmultipath.version
> > b/libmultipath/libmultipath.version
> > index 8a447f7f..af7c5ed2 100644
> > --- a/libmultipath/libmultipath.version
> > +++ b/libmultipath/libmultipath.version
> > @@ -38,9 +38,7 @@ global:
> > add_map_with_path;
> > adopt_paths;
> > alloc_multipath;
> > - alloc_multipath_layout;
> > alloc_path;
> > - alloc_path_layout;
> > alloc_path_with_pathinfo;
> > change_foreign;
> > check_alias_settings;
> > @@ -126,6 +124,7 @@ global:
> > libmultipath_exit;
> > libmultipath_init;
> > load_config;
> > + multipath_layout_size;
> > need_io_err_check;
> > orphan_path;
> > parse_prkey_flags;
> > @@ -133,6 +132,7 @@ global:
> > path_discovery;
> > path_get_tpgs;
> > pathinfo;
> > + path_layout_size;
> > path_offline;
> > print_all_paths;
> > print_foreign_topology;
> > diff --git a/libmultipath/print.c b/libmultipath/print.c
> > index 97f9a177..87d6a329 100644
> > --- a/libmultipath/print.c
> > +++ b/libmultipath/print.c
> > @@ -805,6 +805,12 @@ static const struct multipath_data mpd[] = {
> > {'g', "vpd page data", snprint_multipath_vpd_data},
> > };
> >
> > +
> > +int multipath_layout_size(void)
> > +{
> > + return ARRAY_SIZE(mpd);
> > +}
> > +
> > static const struct path_data pd[] = {
> > {'w', "uuid", snprint_path_uuid},
> > {'i', "hcil", snprint_hcil},
> > @@ -834,6 +840,11 @@ static const struct path_data pd[] = {
> > {'L', "LUN hex", snprint_path_lunhex},
> > };
> >
> > +int path_layout_size(void)
> > +{
> > + return ARRAY_SIZE(pd);
> > +}
> > +
> > static const struct pathgroup_data pgd[] = {
> > {'s', "selector", snprint_pg_selector},
> > {'p', "pri", snprint_pg_pri},
> > @@ -871,10 +882,6 @@ int snprint_wildcards(struct strbuf *buff)
> > return get_strbuf_len(buff) - initial_len;
> > }
> >
> > -fieldwidth_t *alloc_path_layout(void) {
> > - return calloc(ARRAY_SIZE(pd), sizeof(fieldwidth_t));
> > -}
> > -
> > void get_path_layout(vector pathvec, int header, fieldwidth_t
> > *width)
> > {
> > vector gpvec = vector_convert(NULL, pathvec, struct path,
> > @@ -929,11 +936,6 @@ void _get_path_layout (const struct _vector
> > *gpvec, enum layout_reset reset,
> > }
> > }
> >
> > -fieldwidth_t *alloc_multipath_layout(void) {
> > -
> > - return calloc(ARRAY_SIZE(mpd), sizeof(fieldwidth_t));
> > -}
> > -
> > void get_multipath_layout (vector mpvec, int header, fieldwidth_t
> > *width) {
> > vector gmvec = vector_convert(NULL, mpvec, struct multipath,
> > dm_multipath_to_gen);
> > @@ -1187,12 +1189,11 @@ int _snprint_pathgroup(const struct
> > gen_pathgroup *ggp, struct strbuf *line,
> > void _print_multipath_topology(const struct gen_multipath *gmp, int
> > verbosity)
> > {
> > struct strbuf buff = STRBUF_INIT;
> > - fieldwidth_t *p_width
> > __attribute__((cleanup(cleanup_ucharp))) = NULL;
> > + fieldwidth_t p_width[ARRAY_SIZE(pd)] = {0};
> > const struct gen_pathgroup *gpg;
> > const struct _vector *pgvec, *pathvec;
> > int j;
> >
> > - p_width = alloc_path_layout();
> > pgvec = gmp->ops->get_pathgroups(gmp);
> >
> > if (pgvec != NULL) {
> > @@ -1236,14 +1237,11 @@ int _snprint_multipath_topology(const struct
> > gen_multipath *gmp,
> > const struct gen_pathgroup *gpg;
> > struct strbuf style = STRBUF_INIT;
> > size_t initial_len = get_strbuf_len(buff);
> > - fieldwidth_t *width __attribute__((cleanup(cleanup_ucharp)))
> > = NULL;
> > + fieldwidth_t width[ARRAY_SIZE(mpd)] = {0};
> >
> > if (verbosity <= 0)
> > return 0;
> >
> > - if ((width = alloc_multipath_layout()) == NULL)
> > - return -ENOMEM;
> > -
> > if (verbosity == 1)
> > return _snprint_multipath(gmp, buff, "%n", width);
> >
> > @@ -2027,7 +2025,7 @@ static void print_all_paths_custo(vector
> > pathvec, int banner, const char *fmt)
> > int i;
> > struct path * pp;
> > struct strbuf line = STRBUF_INIT;
> > - fieldwidth_t *width __attribute__((cleanup(cleanup_ucharp)))
> > = NULL;
> > + fieldwidth_t width[ARRAY_SIZE(pd)] = {0};
> >
> > if (!VECTOR_SIZE(pathvec)) {
> > if (banner)
> > @@ -2035,8 +2033,6 @@ static void print_all_paths_custo(vector
> > pathvec, int banner, const char *fmt)
> > return;
> > }
> >
> > - if ((width = alloc_path_layout()) == NULL)
> > - return;
> > get_path_layout(pathvec, 1, width);
> >
> > pthread_cleanup_push_cast(reset_strbuf, &line);
> > diff --git a/libmultipath/print.h b/libmultipath/print.h
> > index 52f5b256..4e50827d 100644
> > --- a/libmultipath/print.h
> > +++ b/libmultipath/print.h
> > @@ -16,11 +16,11 @@ enum layout_reset {
> > };
> >
> > /* fieldwidth_t is defined in generic.h */
> > -fieldwidth_t *alloc_path_layout(void);
> > +int multipath_layout_size(void);
> > +int path_layout_size(void);
> > void _get_path_layout (const struct _vector *gpvec, enum
> > layout_reset,
> > fieldwidth_t *width);
> > void get_path_layout (vector pathvec, int header, fieldwidth_t
> > *width);
> > -fieldwidth_t *alloc_multipath_layout(void);
> > void _get_multipath_layout (const struct _vector *gmvec, enum
> > layout_reset,
> > fieldwidth_t *width);
> > void get_multipath_layout (vector mpvec, int header, fieldwidth_t
> > *width);
> > diff --git a/multipath/main.c b/multipath/main.c
> > index 7b69a3ce..f4c85409 100644
> > --- a/multipath/main.c
> > +++ b/multipath/main.c
> > @@ -457,7 +457,7 @@ configure (struct config *conf, enum mpath_cmds
> > cmd,
> > int di_flag = 0;
> > char * refwwid = NULL;
> > char * dev = NULL;
> > - fieldwidth_t *width __attribute__((cleanup(cleanup_ucharp)))
> > = NULL;
> > + fieldwidth_t width[path_layout_size()];
> >
> > /*
> > * allocate core vectors to store paths and multipaths
> > @@ -544,8 +544,7 @@ configure (struct config *conf, enum mpath_cmds
> > cmd,
> > if (libmp_verbosity > 2)
> > print_all_paths(pathvec, 1);
> >
> > - if ((width = alloc_path_layout()) == NULL)
> > - goto out;
> > + memset(width, 0, sizeof(width));
> > get_path_layout(pathvec, 0, width);
> > foreign_path_layout(width);
> >
> > diff --git a/multipathd/cli_handlers.c b/multipathd/cli_handlers.c
> > index 5b8f647b..ddc807a1 100644
> > --- a/multipathd/cli_handlers.c
> > +++ b/multipathd/cli_handlers.c
> > @@ -38,11 +38,10 @@ show_paths (struct strbuf *reply, struct vectors
> > *vecs, char *style, int pretty)
> > int i;
> > struct path * pp;
> > int hdr_len = 0;
> > - fieldwidth_t *width __attribute__((cleanup(cleanup_ucharp)))
> > = NULL;
> > + fieldwidth_t width[path_layout_size()];
> >
> > + memset(width, 0, sizeof(width));
> > if (pretty) {
> > - if ((width = alloc_path_layout()) == NULL)
> > - return 1;
> > get_path_layout(vecs->pathvec, 1, width);
> > foreign_path_layout(width);
> > }
> > @@ -50,10 +49,10 @@ show_paths (struct strbuf *reply, struct vectors
> > *vecs, char *style, int pretty)
> > return 1;
> >
> > vector_foreach_slot(vecs->pathvec, pp, i) {
> > - if (snprint_path(reply, style, pp, width) < 0)
> > + if (snprint_path(reply, style, pp, pretty? width :
> > NULL) < 0)
> > return 1;
> > }
> > - if (snprint_foreign_paths(reply, style, width) < 0)
> > + if (snprint_foreign_paths(reply, style, pretty? width : NULL)
> > < 0)
> > return 1;
> >
> > if (pretty && get_strbuf_len(reply) == (size_t)hdr_len)
> > @@ -67,12 +66,7 @@ static int
> > show_path (struct strbuf *reply, struct vectors *vecs, struct path
> > *pp,
> > char *style)
> > {
> > - fieldwidth_t *width __attribute__((cleanup(cleanup_ucharp)))
> > = NULL;
> > -
> > - if ((width = alloc_path_layout()) == NULL)
> > - return 1;
> > - get_path_layout(vecs->pathvec, 1, width);
> > - if (snprint_path(reply, style, pp, 0) < 0)
> > + if (snprint_path(reply, style, pp, NULL) < 0)
> > return 1;
> > return 0;
> > }
> > @@ -95,10 +89,9 @@ show_maps_topology (struct strbuf *reply, struct
> > vectors * vecs)
> > {
> > int i;
> > struct multipath * mpp;
> > - fieldwidth_t *p_width
> > __attribute__((cleanup(cleanup_ucharp))) = NULL;
> > + fieldwidth_t p_width[path_layout_size()];
> >
> > - if ((p_width = alloc_path_layout()) == NULL)
> > - return 1;
> > + memset(p_width, 0, sizeof(p_width));
> > get_path_layout(vecs->pathvec, 0, p_width);
> > foreign_path_layout(p_width);
> >
> > @@ -258,10 +251,9 @@ cli_list_map_topology (void *v, struct strbuf
> > *reply, void *data)
> > struct multipath * mpp;
> > struct vectors * vecs = (struct vectors *)data;
> > char * param = get_keyparam(v, MAP);
> > - fieldwidth_t *p_width
> > __attribute__((cleanup(cleanup_ucharp))) = NULL;
> > + fieldwidth_t p_width[path_layout_size()];
> >
> > - if ((p_width = alloc_path_layout()) == NULL)
> > - return 1;
> > + memset(p_width, 0, sizeof(p_width));
> > get_path_layout(vecs->pathvec, 0, p_width);
> > param = convert_dev(param, 0);
> > mpp = find_mp_by_str(vecs->mpvec, param);
> > @@ -357,11 +349,10 @@ show_maps (struct strbuf *reply, struct vectors
> > *vecs, char *style,
> > int i;
> > struct multipath * mpp;
> > int hdr_len = 0;
> > - fieldwidth_t *width __attribute__((cleanup(cleanup_ucharp)))
> > = NULL;
> > + fieldwidth_t width[multipath_layout_size()];
> >
> > + memset(width, 0, sizeof(width));
> > if (pretty) {
> > - if ((width = alloc_multipath_layout()) == NULL)
> > - return 1;
> > get_multipath_layout(vecs->mpvec, 1, width);
> > foreign_multipath_layout(width);
> > }
> > @@ -374,10 +365,11 @@ show_maps (struct strbuf *reply, struct vectors
> > *vecs, char *style,
> > i--;
> > continue;
> > }
> > - if (snprint_multipath(reply, style, mpp, width) < 0)
> > + if (snprint_multipath(reply, style, mpp,
> > + pretty? width : NULL) < 0)
> > return 1;
> > }
> > - if (snprint_foreign_multipaths(reply, style, width) < 0)
> > + if (snprint_foreign_multipaths(reply, style, pretty? width :
> > NULL) < 0)
> > return 1;
> >
> > if (pretty && get_strbuf_len(reply) == (size_t)hdr_len)
> > @@ -416,10 +408,9 @@ cli_list_map_fmt (void *v, struct strbuf *reply,
> > void *data)
> > struct vectors * vecs = (struct vectors *)data;
> > char * param = get_keyparam(v, MAP);
> > char * fmt = get_keyparam(v, FMT);
> > - fieldwidth_t *width __attribute__((cleanup(cleanup_ucharp)))
> > = NULL;
> > + fieldwidth_t width[multipath_layout_size()];
> >
> > - if ((width = alloc_multipath_layout()) == NULL)
> > - return 1;
> > + memset(width, 0, sizeof(width));
> > get_multipath_layout(vecs->mpvec, 1, width);
> > param = convert_dev(param, 0);
> > mpp = find_mp_by_str(vecs->mpvec, param);
--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [dm-devel] [PATCH 3/4] libmultipath: use regular array for field widths
2022-10-21 17:51 ` Benjamin Marzinski
@ 2022-10-21 20:12 ` Martin Wilck
0 siblings, 0 replies; 11+ messages in thread
From: Martin Wilck @ 2022-10-21 20:12 UTC (permalink / raw)
To: bmarzins; +Cc: dm-devel
On Fri, 2022-10-21 at 12:51 -0500, Benjamin Marzinski wrote:
> On Fri, Oct 21, 2022 at 07:04:55AM +0000, Martin Wilck wrote:
> > On Tue, 2022-10-11 at 16:53 -0500, Benjamin Marzinski wrote:
> > > We know the size of these arrays, so we can just allocate them on
> > > the
> > > stack. Also, show_path() doesn't use the width, so don't
> > > initialize
> > > it
> > > in the first place.
> > >
> > > Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> >
> > This isn't wrong, but I'm not sure what it actually achieves except
> > a
> > few less NULL checks (I'm sure you're aware that this doesn't mean
> > better protection against out-of-memory situations). It comes at
> > the
> > cost of an ABI change. I understand that the intention is to
> > eliminate
> > __attribute__((cleanup())). But if we agree we don't want to do
> > that
> > everywhere, I see no particular reason to do it in this code path.
> >
> > I'm not totally against it, but I'm not enthusiastic, either.
>
> That's fine. How about I send a patch to just fix show_path().
Yes, please.
Martin
--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2022-10-21 20:13 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-10-11 21:52 [dm-devel] [PATCH 0/4] remove dangerous cleanup __attribute__ uses Benjamin Marzinski
2022-10-11 21:53 ` [dm-devel] [PATCH 1/4] libmultipath: don't print garbage keywords Benjamin Marzinski
2022-10-21 7:05 ` Martin Wilck
2022-10-11 21:53 ` [dm-devel] [PATCH 2/4] libmultipath: avoid STRBUF_ON_STACK with cancellation points Benjamin Marzinski
2022-10-11 21:53 ` [dm-devel] [PATCH 3/4] libmultipath: use regular array for field widths Benjamin Marzinski
2022-10-21 7:04 ` Martin Wilck
2022-10-21 17:51 ` Benjamin Marzinski
2022-10-21 20:12 ` Martin Wilck
2022-10-11 21:53 ` [dm-devel] [PATCH 4/4] libmultipath: avoid cleanup __attribute__ with cancellation points Benjamin Marzinski
2022-10-20 16:03 ` [dm-devel] [PATCH 0/4] remove dangerous cleanup __attribute__ uses Martin Wilck
2022-10-20 22:57 ` Benjamin Marzinski
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.