On Fri, Nov 02, 2018 at 01:21:07PM +0100, Martin Wilck wrote:
> Replace the character array "message" in struct checker with
> a "message ID" field.
>
> The generic checker code defines a couple of standard message IDs
> and corresponding messages. Checker-specific message IDs start
> at CHECKER_FIRST_MSG. Checkers that implement specific message
> IDs must provide a table for converting the IDs into actual log
> messages.
>
> This simplifies the checker data structure and the handling of
> checker messages in the critical checker code path. It comes at
> the cost that only constant message strings are supported. It
> turns out that only a single checker log message (in the emc_clariion
> checker) was dynamically generated, and the missing information can
> be provided with a standard condlog message.
>
> Follow-up patches implement this for the existing checkers.
>
Reviewed-by: Benjamin Marzinski <bmarzins@xxxxxxxxxx>
> Signed-off-by: Martin Wilck <mwilck@xxxxxxxx>
> ---
> libmultipath/checkers.c | 61 +++++++++++++++++++++++++++++++++-------
> libmultipath/checkers.h | 43 ++++++++++++++++++++++++----
> libmultipath/discovery.c | 4 +--
> 3 files changed, 90 insertions(+), 18 deletions(-)
>
> diff --git a/libmultipath/checkers.c b/libmultipath/checkers.c
> index 0bacc864..a1b5cb45 100644
> --- a/libmultipath/checkers.c
> +++ b/libmultipath/checkers.c
> @@ -141,6 +141,22 @@ struct checker * add_checker (char *multipath_dir, char * name)
> if (!c->free)
> goto out;
>
> + c->msgtable_size = 0;
> + c->msgtable = dlsym(c->handle, "libcheck_msgtable");
> +
> + if (c->msgtable != NULL) {
> + const char **p;
> +
> + for (p = c->msgtable;
> + *p && (p - c->msgtable) < CHECKER_MSGTABLE_SIZE; p++)
> + /* nothing */;
> +
> + c->msgtable_size = p - c->msgtable;
> + } else
> + c->msgtable_size = 0;
> + condlog(3, "checker %s: message table size = %d",
> + c->name, c->msgtable_size);
> +
> done:
> c->fd = -1;
> c->sync = 1;
> @@ -222,16 +238,16 @@ int checker_check (struct checker * c, int path_state)
> if (!c)
> return PATH_WILD;
>
> - c->message[0] = '\0';
> + c->msgid = CHECKER_MSGID_NONE;
> if (c->disable) {
> - MSG(c, "checker disabled");
> + c->msgid = CHECKER_MSGID_DISABLED;
> return PATH_UNCHECKED;
> }
> if (!strncmp(c->name, NONE, 4))
> return path_state;
>
> if (c->fd < 0) {
> - MSG(c, "no usable fd");
> + c->msgid = CHECKER_MSGID_NO_FD;
> return PATH_WILD;
> }
> r = c->check(c);
> @@ -248,25 +264,48 @@ int checker_selected (struct checker * c)
> return (c->check) ? 1 : 0;
> }
>
> -char * checker_name (struct checker * c)
> +const char *checker_name(const struct checker *c)
> {
> if (!c)
> return NULL;
> return c->name;
> }
>
> -char * checker_message (struct checker * c)
> +static const char *generic_msg[CHECKER_GENERIC_MSGTABLE_SIZE] = {
> + [CHECKER_MSGID_NONE] = "",
> + [CHECKER_MSGID_DISABLED] = " is disabled",
> + [CHECKER_MSGID_NO_FD] = " has no usable fd",
> + [CHECKER_MSGID_INVALID] = " provided invalid message id",
> + [CHECKER_MSGID_UP] = " reports path is up",
> + [CHECKER_MSGID_DOWN] = " reports path is down",
> + [CHECKER_MSGID_GHOST] = " reports path is ghost",
> +};
> +
> +const char *checker_message(const struct checker *c)
> {
> - if (!c)
> - return NULL;
> - return c->message;
> + int id;
> +
> + if (!c || c->msgid < 0 ||
> + (c->msgid >= CHECKER_GENERIC_MSGTABLE_SIZE &&
> + c->msgid < CHECKER_FIRST_MSGID))
> + goto bad_id;
> +
> + if (c->msgid < CHECKER_GENERIC_MSGTABLE_SIZE)
> + return generic_msg[c->msgid];
> +
> + id = c->msgid - CHECKER_FIRST_MSGID;
> + if (id < c->cls->msgtable_size)
> + return c->cls->msgtable[id];
> +
> +bad_id:
> + return generic_msg[CHECKER_MSGID_NONE];
> }
>
> void checker_clear_message (struct checker *c)
> {
> if (!c)
> return;
> - c->message[0] = '\0';
> + c->msgid = CHECKER_MSGID_NONE;
> }
>
> void checker_get (char *multipath_dir, struct checker * dst, char * name)
> @@ -288,10 +327,12 @@ void checker_get (char *multipath_dir, struct checker * dst, char * name)
> dst->fd = src->fd;
> dst->sync = src->sync;
> strncpy(dst->name, src->name, CHECKER_NAME_LEN);
> - strncpy(dst->message, src->message, CHECKER_MSG_LEN);
> + dst->msgid = CHECKER_MSGID_NONE;
> dst->check = src->check;
> dst->init = src->init;
> dst->free = src->free;
> + dst->msgtable = src->msgtable;
> + dst->msgtable_size = src->msgtable_size;
> dst->handle = NULL;
> src->refcount++;
> }
> diff --git a/libmultipath/checkers.h b/libmultipath/checkers.h
> index 7b18a1ac..8e26f1df 100644
> --- a/libmultipath/checkers.h
> +++ b/libmultipath/checkers.h
> @@ -97,6 +97,22 @@ enum path_check_state {
> #define CHECKER_DEV_LEN 256
> #define LIB_CHECKER_NAMELEN 256
>
> +/*
> + * Generic message IDs for use in checkers.
> + */
> +enum {
> + CHECKER_MSGID_NONE = 0,
> + CHECKER_MSGID_DISABLED,
> + CHECKER_MSGID_NO_FD,
> + CHECKER_MSGID_INVALID,
> + CHECKER_MSGID_UP,
> + CHECKER_MSGID_DOWN,
> + CHECKER_MSGID_GHOST,
> + CHECKER_GENERIC_MSGTABLE_SIZE,
> + CHECKER_FIRST_MSGID = 100, /* lowest msgid for checkers */
> + CHECKER_MSGTABLE_SIZE = 100, /* max msg table size for checkers */
> +};
> +
> struct checker {
> struct list_head node;
> void *handle;
> @@ -106,7 +122,7 @@ struct checker {
> unsigned int timeout;
> int disable;
> char name[CHECKER_NAME_LEN];
> - char message[CHECKER_MSG_LEN]; /* comm with callers */
> + short msgid; /* checker-internal extra status */
> void * context; /* store for persistent data */
> void ** mpcontext; /* store for persistent data shared
> multipath-wide. Use MALLOC if
> @@ -114,10 +130,10 @@ struct checker {
> int (*check)(struct checker *);
> int (*init)(struct checker *); /* to allocate the context */
> void (*free)(struct checker *); /* to free the context */
> + const char**msgtable;
> + short msgtable_size;
> };
>
> -#define MSG(c, fmt, args...) snprintf((c)->message, CHECKER_MSG_LEN, fmt, ##args);
> -
> char * checker_state_name (int);
> int init_checkers (char *);
> void cleanup_checkers (void);
> @@ -134,14 +150,29 @@ void checker_enable (struct checker *);
> void checker_disable (struct checker *);
> int checker_check (struct checker *, int);
> int checker_selected (struct checker *);
> -char * checker_name (struct checker *);
> -char * checker_message (struct checker *);
> +const char *checker_name (const struct checker *);
> +/*
> + * This returns a string that's best prepended with "$NAME checker",
> + * where $NAME is the return value of checker_name().
> + */
> +const char *checker_message(const struct checker *);
> void checker_clear_message (struct checker *c);
> void checker_get (char *, struct checker *, char *);
>
> -/* Functions exported by path checker dynamic libraries (.so) */
> +/* Prototypes for symbols exported by path checker dynamic libraries (.so) */
> int libcheck_check(struct checker *);
> int libcheck_init(struct checker *);
> void libcheck_free(struct checker *);
> +/*
> + * msgid => message map.
> + *
> + * It only needs to be provided if the checker defines specific
> + * message IDs.
> + * Message IDs available to checkers start at CHECKER_FIRST_MSG.
> + * The msgtable array is 0-based, i.e. msgtable[0] is the message
> + * for msgid == __CHECKER_FIRST_MSG.
> + * The table ends with a NULL element.
> + */
> +extern const char *libcheck_msgtable[];
>
> #endif /* _CHECKERS_H */
> diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
> index 3550c3a7..5e59e273 100644
> --- a/libmultipath/discovery.c
> +++ b/libmultipath/discovery.c
> @@ -1589,8 +1589,8 @@ get_state (struct path * pp, struct config *conf, int daemon, int oldstate)
> 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));
> + condlog(3, "%s: %s checker%s",
> + pp->dev, checker_name(c), checker_message(c));
> return state;
> }
>
> --
> 2.19.1
--
dm-devel mailing list
dm-devel@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/dm-devel
