Re: [PATCH 21/21] libmultipath/checkers: cleanup class/instance model

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, Oct 12, 2018 at 12:27:07AM +0200, Martin Wilck wrote:
> The checkers code implicitly uses a sort-of OOP class/instance model,
> but very clumsily. Separate the checker "class" and "instance" cleanly,
> and do a few further cleanups (constifications etc) on the way.
> 

Reviewed-by: Benjamin Marzinski <bmarzins@xxxxxxxxxx>

> Signed-off-by: Martin Wilck <mwilck@xxxxxxxx>
> ---
>  libmultipath/checkers.c          | 137 ++++++++++++++++---------------
>  libmultipath/checkers.h          |  23 ++----
>  libmultipath/checkers/directio.c |   2 +-
>  libmultipath/checkers/tur.c      |   2 +-
>  libmultipath/print.c             |   2 +-
>  libmultipath/propsel.c           |  19 +++--
>  6 files changed, 92 insertions(+), 93 deletions(-)
> 
> diff --git a/libmultipath/checkers.c b/libmultipath/checkers.c
> index 19c8ad58..7733471c 100644
> --- a/libmultipath/checkers.c
> +++ b/libmultipath/checkers.c
> @@ -8,6 +8,19 @@
>  #include "checkers.h"
>  #include "vector.h"
>  
> +struct checker_class {
> +	struct list_head node;
> +	void *handle;
> +	int refcount;
> +	int sync;
> +	char name[CHECKER_NAME_LEN];
> +	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;
> +};
> +
>  char *checker_state_names[] = {
>  	"wild",
>  	"unchecked",
> @@ -23,38 +36,30 @@ char *checker_state_names[] = {
>  
>  static LIST_HEAD(checkers);
>  
> -char * checker_state_name (int i)
> +const char *checker_state_name(int i)
>  {
>  	return checker_state_names[i];
>  }
>  
> -int init_checkers (char *multipath_dir)
> -{
> -	if (!add_checker(multipath_dir, DEFAULT_CHECKER))
> -		return 1;
> -	return 0;
> -}
> -
> -struct checker * alloc_checker (void)
> +static struct checker_class *alloc_checker_class(void)
>  {
> -	struct checker *c;
> +	struct checker_class *c;
>  
> -	c = MALLOC(sizeof(struct checker));
> +	c = MALLOC(sizeof(struct checker_class));
>  	if (c) {
>  		INIT_LIST_HEAD(&c->node);
>  		c->refcount = 1;
> -		c->fd = -1;
>  	}
>  	return c;
>  }
>  
> -void free_checker (struct checker * c)
> +void free_checker_class(struct checker_class *c)
>  {
>  	if (!c)
>  		return;
>  	c->refcount--;
>  	if (c->refcount) {
> -		condlog(3, "%s checker refcount %d",
> +		condlog(4, "%s checker refcount %d",
>  			c->name, c->refcount);
>  		return;
>  	}
> @@ -71,17 +76,17 @@ void free_checker (struct checker * c)
>  
>  void cleanup_checkers (void)
>  {
> -	struct checker * checker_loop;
> -	struct checker * checker_temp;
> +	struct checker_class *checker_loop;
> +	struct checker_class *checker_temp;
>  
>  	list_for_each_entry_safe(checker_loop, checker_temp, &checkers, node) {
> -		free_checker(checker_loop);
> +		free_checker_class(checker_loop);
>  	}
>  }
>  
> -struct checker * checker_lookup (char * name)
> +static struct checker_class *checker_class_lookup(const char *name)
>  {
> -	struct checker * c;
> +	struct checker_class *c;
>  
>  	if (!name || !strlen(name))
>  		return NULL;
> @@ -92,14 +97,15 @@ struct checker * checker_lookup (char * name)
>  	return NULL;
>  }
>  
> -struct checker * add_checker (char *multipath_dir, char * name)
> +static struct checker_class *add_checker_class(const char *multipath_dir,
> +					       const char *name)
>  {
>  	char libname[LIB_CHECKER_NAMELEN];
>  	struct stat stbuf;
> -	struct checker * c;
> +	struct checker_class *c;
>  	char *errstr;
>  
> -	c = alloc_checker();
> +	c = alloc_checker_class();
>  	if (!c)
>  		return NULL;
>  	snprintf(c->name, CHECKER_NAME_LEN, "%s", name);
> @@ -158,12 +164,11 @@ struct checker * add_checker (char *multipath_dir, char * name)
>  		c->name, c->msgtable_size);
>  
>  done:
> -	c->fd = -1;
>  	c->sync = 1;
>  	list_add(&c->node, &checkers);
>  	return c;
>  out:
> -	free_checker(c);
> +	free_checker_class(c);
>  	return NULL;
>  }
>  
> @@ -176,16 +181,16 @@ void checker_set_fd (struct checker * c, int fd)
>  
>  void checker_set_sync (struct checker * c)
>  {
> -	if (!c)
> +	if (!c || !c->cls)
>  		return;
> -	c->sync = 1;
> +	c->cls->sync = 1;
>  }
>  
>  void checker_set_async (struct checker * c)
>  {
> -	if (!c)
> +	if (!c || !c->cls)
>  		return;
> -	c->sync = 0;
> +	c->cls->sync = 0;
>  }
>  
>  void checker_enable (struct checker * c)
> @@ -204,11 +209,11 @@ void checker_disable (struct checker * c)
>  
>  int checker_init (struct checker * c, void ** mpctxt_addr)
>  {
> -	if (!c)
> +	if (!c || !c->cls)
>  		return 1;
>  	c->mpcontext = mpctxt_addr;
> -	if (c->init)
> -		return c->init(c);
> +	if (c->cls->init)
> +		return c->cls->init(c);
>  	return 0;
>  }
>  
> @@ -220,15 +225,16 @@ void checker_clear (struct checker *c)
>  
>  void checker_put (struct checker * dst)
>  {
> -	struct checker * src;
> +	struct checker_class *src;
>  
> -	if (!dst || !strlen(dst->name))
> +	if (!dst)
>  		return;
> -	src = checker_lookup(dst->name);
> -	if (dst->free)
> -		dst->free(dst);
> +	src = dst->cls;
> +
> +	if (src && src->free)
> +		src->free(dst);
>  	checker_clear(dst);
> -	free_checker(src);
> +	free_checker_class(src);
>  }
>  
>  int checker_check (struct checker * c, int path_state)
> @@ -243,32 +249,35 @@ int checker_check (struct checker * c, int path_state)
>  		c->msgid = CHECKER_MSGID_DISABLED;
>  		return PATH_UNCHECKED;
>  	}
> -	if (!strncmp(c->name, NONE, 4))
> +	if (!strncmp(c->cls->name, NONE, 4))
>  		return path_state;
>  
>  	if (c->fd < 0) {
>  		c->msgid = CHECKER_MSGID_NO_FD;
>  		return PATH_WILD;
>  	}
> -	r = c->check(c);
> +	r = c->cls->check(c);
>  
>  	return r;
>  }
>  
> -int checker_selected (struct checker * c)
> +int checker_selected(const struct checker *c)
>  {
>  	if (!c)
>  		return 0;
> -	if (!strncmp(c->name, NONE, 4))
> -		return 1;
> -	return (c->check) ? 1 : 0;
> +	return c->cls != NULL;
>  }
>  
>  const char *checker_name(const struct checker *c)
>  {
> -	if (!c)
> +	if (!c || !c->cls)
>  		return NULL;
> -	return c->name;
> +	return c->cls->name;
> +}
> +
> +int checker_is_sync(const struct checker *c)
> +{
> +	return c && c->cls && c->cls->sync;
>  }
>  
>  static const char *generic_msg[CHECKER_LAST_GENERIC_MSGID] = {
> @@ -295,8 +304,8 @@ static const char *_checker_message(const struct checker *c)
>  		return generic_msg[c->msgid];
>  
>  	id = c->msgid - CHECKER_FIRST_MSGID;
> -	if (id < c->msgtable_size)
> -		return c->msgtable[id];
> +	if (id < c->cls->msgtable_size)
> +		return c->cls->msgtable[id];
>  	return NULL;
>  }
>  
> @@ -309,7 +318,7 @@ const char *checker_message(const struct checker *c)
>  		*buf = '\0';
>  	else
>  		snprintf(buf, sizeof(buf), "%s checker%s",
> -			 c->name, msg);
> +			 c->cls->name, msg);
>  	return buf;
>  }
>  
> @@ -320,31 +329,29 @@ void checker_clear_message (struct checker *c)
>  	c->msgid = CHECKER_MSGID_NONE;
>  }
>  
> -void checker_get (char *multipath_dir, struct checker * dst, char * name)
> +void checker_get(const char *multipath_dir, struct checker *dst,
> +		 const char *name)
>  {
> -	struct checker * src = NULL;
> +	struct checker_class *src = NULL;
>  
>  	if (!dst)
>  		return;
>  
>  	if (name && strlen(name)) {
> -		src = checker_lookup(name);
> +		src = checker_class_lookup(name);
>  		if (!src)
> -			src = add_checker(multipath_dir, name);
> +			src = add_checker_class(multipath_dir, name);
>  	}
> -	if (!src) {
> -		dst->check = NULL;
> +	dst->cls = src;
> +	if (!src)
>  		return;
> -	}
> -	dst->fd = src->fd;
> -	dst->sync = src->sync;
> -	strncpy(dst->name, src->name, CHECKER_NAME_LEN);
> -	dst->msgid = src->msgid;
> -	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++;
>  }
> +
> +int init_checkers(const char *multipath_dir)
> +{
> +	if (!add_checker_class(multipath_dir, DEFAULT_CHECKER))
> +		return 1;
> +	return 0;
> +}
> diff --git a/libmultipath/checkers.h b/libmultipath/checkers.h
> index 98fec2c4..c46f7ffa 100644
> --- a/libmultipath/checkers.h
> +++ b/libmultipath/checkers.h
> @@ -116,32 +116,22 @@ enum {
>  	CHECKER_MSGTABLE_SIZE = 100,	/* max msg table size for checkers */
>  };
>  
> +struct checker_class;
>  struct checker {
> -	struct list_head node;
> -	void *handle;
> -	int refcount;
> +	struct checker_class *cls;
>  	int fd;
> -	int sync;
>  	unsigned int timeout;
>  	int disable;
> -	char name[CHECKER_NAME_LEN];
>  	short msgid;		             /* checker-internal extra status */
>  	void * context;                      /* store for persistent data */
>  	void ** mpcontext;                   /* store for persistent data shared
>  						multipath-wide. Use MALLOC if
>  						you want to stuff data in. */
> -	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;
>  };
>  
> -char * checker_state_name (int);
> -int init_checkers (char *);
> +const char *checker_state_name(int);
> +int init_checkers(const char *);
>  void cleanup_checkers (void);
> -struct checker * add_checker (char *, char *);
> -struct checker * checker_lookup (char *);
>  int checker_init (struct checker *, void **);
>  void checker_clear (struct checker *);
>  void checker_put (struct checker *);
> @@ -152,11 +142,12 @@ void checker_set_fd (struct checker *, int);
>  void checker_enable (struct checker *);
>  void checker_disable (struct checker *);
>  int checker_check (struct checker *, int);
> -int checker_selected (struct checker *);
> +int checker_selected(const struct checker *);
> +int checker_is_sync(const struct checker *);
>  const char *checker_name (const struct checker *);
>  const char *checker_message (const struct checker *);
>  void checker_clear_message (struct checker *c);
> -void checker_get (char *, struct checker *, char *);
> +void checker_get(const char *, struct checker *, const char *);
>  
>  /* Prototypes for symbols exported by path checker dynamic libraries (.so) */
>  int libcheck_check(struct checker *);
> diff --git a/libmultipath/checkers/directio.c b/libmultipath/checkers/directio.c
> index c4a0712e..1b00b775 100644
> --- a/libmultipath/checkers/directio.c
> +++ b/libmultipath/checkers/directio.c
> @@ -202,7 +202,7 @@ int libcheck_check (struct checker * c)
>  	if (!ct)
>  		return PATH_UNCHECKED;
>  
> -	ret = check_state(c->fd, ct, c->sync, c->timeout);
> +	ret = check_state(c->fd, ct, checker_is_sync(c), c->timeout);
>  
>  	switch (ret)
>  	{
> diff --git a/libmultipath/checkers/tur.c b/libmultipath/checkers/tur.c
> index 8f4bdc8b..05e7bed6 100644
> --- a/libmultipath/checkers/tur.c
> +++ b/libmultipath/checkers/tur.c
> @@ -323,7 +323,7 @@ int libcheck_check(struct checker * c)
>  	if (!ct)
>  		return PATH_UNCHECKED;
>  
> -	if (c->sync)
> +	if (checker_is_sync(c))
>  		return tur_check(c->fd, c->timeout, &c->msgid);
>  
>  	/*
> diff --git a/libmultipath/print.c b/libmultipath/print.c
> index 7b610b94..fc40b0f0 100644
> --- a/libmultipath/print.c
> +++ b/libmultipath/print.c
> @@ -615,7 +615,7 @@ static int
>  snprint_path_checker (char * buff, size_t len, const struct path * pp)
>  {
>  	const struct checker * c = &pp->checker;
> -	return snprint_str(buff, len, c->name);
> +	return snprint_str(buff, len, checker_name(c));
>  }
>  
>  static int
> diff --git a/libmultipath/propsel.c b/libmultipath/propsel.c
> index fdb5953a..970a3b5c 100644
> --- a/libmultipath/propsel.c
> +++ b/libmultipath/propsel.c
> @@ -479,26 +479,27 @@ check_rdac(struct path * pp)
>  int select_checker(struct config *conf, struct path *pp)
>  {
>  	const char *origin;
> -	char *checker_name;
> +	char *ckr_name;
>  	struct checker * c = &pp->checker;
>  
>  	if (pp->detect_checker == DETECT_CHECKER_ON) {
>  		origin = autodetect_origin;
>  		if (check_rdac(pp)) {
> -			checker_name = RDAC;
> +			ckr_name = RDAC;
>  			goto out;
>  		} else if (pp->tpgs > 0) {
> -			checker_name = TUR;
> +			ckr_name = TUR;
>  			goto out;
>  		}
>  	}
> -	do_set(checker_name, conf->overrides, checker_name, overrides_origin);
> -	do_set_from_hwe(checker_name, pp, checker_name, hwe_origin);
> -	do_set(checker_name, conf, checker_name, conf_origin);
> -	do_default(checker_name, DEFAULT_CHECKER);
> +	do_set(checker_name, conf->overrides, ckr_name, overrides_origin);
> +	do_set_from_hwe(checker_name, pp, ckr_name, hwe_origin);
> +	do_set(checker_name, conf, ckr_name, conf_origin);
> +	do_default(ckr_name, DEFAULT_CHECKER);
>  out:
> -	checker_get(conf->multipath_dir, c, checker_name);
> -	condlog(3, "%s: path_checker = %s %s", pp->dev, c->name, origin);
> +	checker_get(conf->multipath_dir, c, ckr_name);
> +	condlog(3, "%s: path_checker = %s %s", pp->dev,
> +		checker_name(c), origin);
>  	if (conf->checker_timeout) {
>  		c->timeout = conf->checker_timeout;
>  		condlog(3, "%s: checker timeout = %u s %s",
> -- 
> 2.19.0

--
dm-devel mailing list
dm-devel@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/dm-devel



[Index of Archives]     [DM Crypt]     [Fedora Desktop]     [ATA RAID]     [Fedora Marketing]     [Fedora Packaging]     [Fedora SELinux]     [Yosemite Discussion]     [KDE Users]     [Fedora Docs]

  Powered by Linux