| Message ID | 20181030210653.29677-23-mwilck@suse.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | christophe varoqui |
| Headers | show |
| Series | libmultipath: checkers overhaul | expand |
On Tue, Oct 30, 2018 at 10:06:53PM +0100, Martin Wilck wrote: > Get rid of the static char buffer in checker_message() > introduced in the previous "replace message by msgid" patch. What you have is fine, but why not something like this, which doesn't need to do any allocating? diff --git a/libmultipath/checkers.c b/libmultipath/checkers.c index 9d9e3ff..ed75150 100644 --- a/libmultipath/checkers.c +++ b/libmultipath/checkers.c @@ -291,7 +291,16 @@ static const char *generic_msg[CHECKER_GENERIC_MSGTABLE_SIZE] = { [CHECKER_MSGID_UNSUPPORTED] = " doesn't support this device", }; -static const char *_checker_message(const struct checker *c) +int checker_has_message(const struct checker *c) +{ + const char *msg = checker_message(c); + + if (msg == NULL || *msg == '\0') + return 0; + return 1; +} + +const char *checker_message(const struct checker *c) { int id; @@ -309,19 +318,6 @@ static const char *_checker_message(const struct checker *c) return NULL; } -char *checker_message(const struct checker *c) -{ - static char buf[CHECKER_MSG_LEN]; - const char *msg = _checker_message(c); - - if (msg == NULL || *msg == '\0') - *buf = '\0'; - else - snprintf(buf, sizeof(buf), "%s checker%s", - c->cls->name, msg); - return buf; -} - void checker_clear_message (struct checker *c) { if (!c) diff --git a/libmultipath/checkers.h b/libmultipath/checkers.h index 4e1fdc1..51fd054 100644 --- a/libmultipath/checkers.h +++ b/libmultipath/checkers.h @@ -145,7 +145,8 @@ int checker_check (struct checker *, int); int checker_selected(const struct checker *); int checker_is_sync(const struct checker *); const char *checker_name (const struct checker *); -char *checker_message (const struct checker *); +const char *checker_message (const struct checker *); +int checker_has_message(const struct checker *); void checker_clear_message (struct checker *c); void checker_get(const char *, struct checker *, const char *); diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c index 467ece7..a3d8fee 100644 --- a/libmultipath/discovery.c +++ b/libmultipath/discovery.c @@ -1588,9 +1588,9 @@ get_state (struct path * pp, struct config *conf, int daemon, int oldstate) condlog(3, "%s: %s state = %s", pp->dev, checker_name(c), checker_state_name(state)); if (state != PATH_UP && state != PATH_GHOST && - strlen(checker_message(c))) - condlog(3, "%s: checker msg is \"%s\"", - pp->dev, checker_message(c)); + checker_has_message(c)) + condlog(3, "%s: checker msg is \"%s checker%s\"", + pp->dev, checker_name(c), checker_message(c)); return state; } diff --git a/multipathd/main.c b/multipathd/main.c index e80ac90..52d6855 100644 --- a/multipathd/main.c +++ b/multipathd/main.c @@ -95,15 +95,11 @@ do { \ if (pp->offline) \ condlog(lvl, "%s: %s - path offline", \ pp->mpp->alias, pp->dev); \ - else { \ - const char *__m = \ - checker_message(&pp->checker); \ - \ - if (strlen(__m)) \ - condlog(lvl, "%s: %s - %s", \ - pp->mpp->alias, \ - pp->dev, __m); \ - } \ + else if (checker_has_message(&pp->checker)) \ + condlog(lvl, "%s: %s - %s checker%s", \ + pp->mpp->alias, pp->dev, \ + checker_name(&pp->checker), \ + checker_message(&pp->checker)); \ } \ } while(0) > Signed-off-by: Martin Wilck <mwilck@suse.com> > --- > libmultipath/checkers.c | 23 ++++++++++++++++++----- > libmultipath/checkers.h | 4 +++- > libmultipath/discovery.c | 9 ++++++--- > multipathd/main.c | 11 +++++------ > 4 files changed, 32 insertions(+), 15 deletions(-) > > diff --git a/libmultipath/checkers.c b/libmultipath/checkers.c > index 9a19c9a6..f65a9476 100644 > --- a/libmultipath/checkers.c > +++ b/libmultipath/checkers.c > @@ -309,16 +309,29 @@ static const char *_checker_message(const struct checker *c) > return NULL; > } > > +int checker_has_message(const struct checker *c) > +{ > + const char *msg = _checker_message(c); > + > + if (msg == NULL || *msg == '\0') > + return 0; > + return 1; > +} > + > char *checker_message(const struct checker *c) > { > - static char buf[CHECKER_MSG_LEN]; > + char *buf; > const char *msg = _checker_message(c); > + int len; > > if (msg == NULL || *msg == '\0') > - *buf = '\0'; > - else > - snprintf(buf, sizeof(buf), "%s checker%s", > - c->cls->name, msg); > + return NULL; > + > + len = CHECKER_NAME_LEN + 8 + CHECKER_MSG_LEN; > + buf = MALLOC(len); > + if (buf == NULL) > + return NULL; > + snprintf(buf, len, "%s checker%s", c->cls->name, msg); > return buf; > } > > diff --git a/libmultipath/checkers.h b/libmultipath/checkers.h > index 834efcfe..2c71e40c 100644 > --- a/libmultipath/checkers.h > +++ b/libmultipath/checkers.h > @@ -145,7 +145,9 @@ int checker_check (struct checker *, int); > int checker_selected(const struct checker *); > int checker_is_sync(const struct checker *); > const char *checker_name (const struct checker *); > -char *checker_message (const struct checker *); > +/* checker_message(): returned pointer is malloc()d */ > +char *checker_message(const struct checker *); > +int checker_has_message(const struct checker *); > void checker_clear_message (struct checker *c); > void checker_get(const char *, struct checker *, const char *); > > diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c > index 467ece7a..0bcd3c39 100644 > --- a/libmultipath/discovery.c > +++ b/libmultipath/discovery.c > @@ -1588,9 +1588,12 @@ get_state (struct path * pp, struct config *conf, int daemon, int oldstate) > condlog(3, "%s: %s state = %s", pp->dev, > checker_name(c), checker_state_name(state)); > if (state != PATH_UP && state != PATH_GHOST && > - strlen(checker_message(c))) > - condlog(3, "%s: checker msg is \"%s\"", > - pp->dev, checker_message(c)); > + checker_has_message(c)) { > + char *msg = checker_message(c); > + > + condlog(3, "%s: checker msg is \"%s\"", pp->dev, msg); > + FREE(msg); > + } > return state; > } > > diff --git a/multipathd/main.c b/multipathd/main.c > index e80ac906..fcf0d2db 100644 > --- a/multipathd/main.c > +++ b/multipathd/main.c > @@ -95,14 +95,13 @@ do { \ > if (pp->offline) \ > condlog(lvl, "%s: %s - path offline", \ > pp->mpp->alias, pp->dev); \ > - else { \ > - const char *__m = \ > + else if (checker_has_message(&pp->checker)) { \ > + char *__m = \ > checker_message(&pp->checker); \ > \ > - if (strlen(__m)) \ > - condlog(lvl, "%s: %s - %s", \ > - pp->mpp->alias, \ > - pp->dev, __m); \ > + condlog(lvl, "%s: %s - %s", \ > + pp->mpp->alias, pp->dev, __m); \ > + FREE(__m); \ > } \ > } \ > } while(0) > -- > 2.19.1 -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
On Thu, 2018-11-01 at 14:53 -0500, Benjamin Marzinski wrote: > On Tue, Oct 30, 2018 at 10:06:53PM +0100, Martin Wilck wrote: > > Get rid of the static char buffer in checker_message() > > introduced in the previous "replace message by msgid" patch. > > What you have is fine, but why not something like this, which doesn't > need to do any allocating? The reason (apart from myself being blind) was that checker_message() prints some extra text around the message. But with only two callers, that's actually over-complicating matters. Thanks for pointint this out, I'll re-post. Martin
diff --git a/libmultipath/checkers.c b/libmultipath/checkers.c index 9a19c9a6..f65a9476 100644 --- a/libmultipath/checkers.c +++ b/libmultipath/checkers.c @@ -309,16 +309,29 @@ static const char *_checker_message(const struct checker *c) return NULL; } +int checker_has_message(const struct checker *c) +{ + const char *msg = _checker_message(c); + + if (msg == NULL || *msg == '\0') + return 0; + return 1; +} + char *checker_message(const struct checker *c) { - static char buf[CHECKER_MSG_LEN]; + char *buf; const char *msg = _checker_message(c); + int len; if (msg == NULL || *msg == '\0') - *buf = '\0'; - else - snprintf(buf, sizeof(buf), "%s checker%s", - c->cls->name, msg); + return NULL; + + len = CHECKER_NAME_LEN + 8 + CHECKER_MSG_LEN; + buf = MALLOC(len); + if (buf == NULL) + return NULL; + snprintf(buf, len, "%s checker%s", c->cls->name, msg); return buf; } diff --git a/libmultipath/checkers.h b/libmultipath/checkers.h index 834efcfe..2c71e40c 100644 --- a/libmultipath/checkers.h +++ b/libmultipath/checkers.h @@ -145,7 +145,9 @@ int checker_check (struct checker *, int); int checker_selected(const struct checker *); int checker_is_sync(const struct checker *); const char *checker_name (const struct checker *); -char *checker_message (const struct checker *); +/* checker_message(): returned pointer is malloc()d */ +char *checker_message(const struct checker *); +int checker_has_message(const struct checker *); void checker_clear_message (struct checker *c); void checker_get(const char *, struct checker *, const char *); diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c index 467ece7a..0bcd3c39 100644 --- a/libmultipath/discovery.c +++ b/libmultipath/discovery.c @@ -1588,9 +1588,12 @@ get_state (struct path * pp, struct config *conf, int daemon, int oldstate) condlog(3, "%s: %s state = %s", pp->dev, checker_name(c), checker_state_name(state)); if (state != PATH_UP && state != PATH_GHOST && - strlen(checker_message(c))) - condlog(3, "%s: checker msg is \"%s\"", - pp->dev, checker_message(c)); + checker_has_message(c)) { + char *msg = checker_message(c); + + condlog(3, "%s: checker msg is \"%s\"", pp->dev, msg); + FREE(msg); + } return state; } diff --git a/multipathd/main.c b/multipathd/main.c index e80ac906..fcf0d2db 100644 --- a/multipathd/main.c +++ b/multipathd/main.c @@ -95,14 +95,13 @@ do { \ if (pp->offline) \ condlog(lvl, "%s: %s - path offline", \ pp->mpp->alias, pp->dev); \ - else { \ - const char *__m = \ + else if (checker_has_message(&pp->checker)) { \ + char *__m = \ checker_message(&pp->checker); \ \ - if (strlen(__m)) \ - condlog(lvl, "%s: %s - %s", \ - pp->mpp->alias, \ - pp->dev, __m); \ + condlog(lvl, "%s: %s - %s", \ + pp->mpp->alias, pp->dev, __m); \ + FREE(__m); \ } \ } \ } while(0)
Get rid of the static char buffer in checker_message() introduced in the previous "replace message by msgid" patch. Signed-off-by: Martin Wilck <mwilck@suse.com> --- libmultipath/checkers.c | 23 ++++++++++++++++++----- libmultipath/checkers.h | 4 +++- libmultipath/discovery.c | 9 ++++++--- multipathd/main.c | 11 +++++------ 4 files changed, 32 insertions(+), 15 deletions(-)