dm-devel.redhat.com archive mirror
 help / color / mirror / Atom feed
From: Benjamin Marzinski <bmarzins@redhat.com>
To: mwilck@suse.com
Cc: lixiaokeng@huawei.com, dm-devel@redhat.com,
	Chongyun Wu <wu.chongyun@h3c.com>
Subject: Re: [dm-devel] [PATCH 30/35] multipathd: uxlsnr: merge uxsock_trigger() into state machine
Date: Wed, 15 Sep 2021 22:32:37 -0500	[thread overview]
Message-ID: <20210916033237.GP3087@octiron.msp.redhat.com> (raw)
In-Reply-To: <20210910114120.13665-31-mwilck@suse.com>

On Fri, Sep 10, 2021 at 01:41:15PM +0200, mwilck@suse.com wrote:
> From: Martin Wilck <mwilck@suse.com>
> 
> This patch sets up the bulk of the state machine. The idea is to
> fall through the case labels as long as possible (when steps succeed)
> and return to the caller if either an error occurs, or it becomes
> necessary to wait for some pollable condition.
> 
> While doing this, switch to negative error codes for the functions
> in uxlsnr.c (e.g. parse_cmd()). Positive return codes are reserved
> for the cli_handler functions themselves. This way we can clearly
> distinguish the error source, and avoid confusion and misleading
> error messages. No cli_handler returns negative values.
> 
> Note: with this patch applied, clients may hang and time out if
> the handler fails to acquire the vecs lock. This will be fixed in the
> follow-up patch "multipathd: uxlsnr: add idle notification".
> 
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  multipathd/uxlsnr.c | 145 ++++++++++++++++++++++++--------------------
>  1 file changed, 80 insertions(+), 65 deletions(-)
> 
> diff --git a/multipathd/uxlsnr.c b/multipathd/uxlsnr.c
> index ff9604f..553274b 100644
> --- a/multipathd/uxlsnr.c
> +++ b/multipathd/uxlsnr.c
> @@ -299,22 +299,13 @@ static int parse_cmd(struct client *c)
>  
>  	r = get_cmdvec(c->cmd, &c->cmdvec);
>  
> -	if (r) {
> -		genhelp_handler(c->cmd, r, &c->reply);
> -		if (get_strbuf_len(&c->reply) == 0)
> -			return EINVAL;
> -		return 0;
> -	}
> +	if (r)
> +		return -r;
>  
>  	c->handler = find_handler_for_cmdvec(c->cmdvec);
>  
> -	if (!c->handler || !c->handler->fn) {
> -		genhelp_handler(c->cmd, EINVAL, &c->reply);
> -		if (get_strbuf_len(&c->reply) == 0)
> -			r = EINVAL;
> -		else
> -			r = 0;
> -	}
> +	if (!c->handler || !c->handler->fn)
> +		return -EINVAL;
>  
>  	return r;
>  }
> @@ -325,7 +316,7 @@ static int execute_handler(struct client *c, struct vectors *vecs, int timeout)
>  	struct timespec tmo;
>  
>  	if (!c->handler)
> -		return EINVAL;
> +		return -EINVAL;
>  
>  	if (clock_gettime(CLOCK_REALTIME, &tmo) == 0) {
>  		tmo.tv_sec += timeout;
> @@ -355,50 +346,29 @@ static int execute_handler(struct client *c, struct vectors *vecs, int timeout)
>  	return r;
>  }
>  
> -static int uxsock_trigger(struct client *c, void *trigger_data)
> +void default_reply(struct client *c, int r)
>  {
> -	struct vectors * vecs;
> -	int r = 1;
> -
> -	vecs = (struct vectors *)trigger_data;
> -
> -	r = parse_cmd(c);
> -
> -	if (r == 0 && c->cmdvec && VECTOR_SIZE(c->cmdvec) > 0) {
> -		struct key *kw = VECTOR_SLOT(c->cmdvec, 0);
> -
> -		if (!c->is_root && kw->code != LIST)
> -			r = EPERM;
> -	}
> -
> -	if (r == 0 && c->handler)
> -		r = execute_handler(c, vecs, uxsock_timeout / 1000);
> -
> -	if (c->cmdvec) {
> -		free_keys(c->cmdvec);
> -		c->cmdvec = NULL;
> -	}
> -
> -	if (r > 0) {
> -		switch(r) {
> -		case ETIMEDOUT:
> -			append_strbuf_str(&c->reply, "timeout\n");
> -			break;
> -		case EPERM:
> -			append_strbuf_str(&c->reply,
> -					  "permission deny: need to be root\n");
> -			break;
> -		default:
> -			append_strbuf_str(&c->reply, "fail\n");
> -			break;
> -		}
> -	}
> -	else if (!r && get_strbuf_len(&c->reply) == 0) {
> +	switch(r) {
> +	case -EINVAL:
> +	case -ESRCH:
> +	case -ENOMEM:

get_cmdvec() returns positive errors and do_genhelp() expects positive
errors, but this expects negative errors.

> +		/* return codes from get_cmdvec() */
> +		genhelp_handler(c->cmd, r, &c->reply);
> +		break;
> +	case -EPERM:
> +		append_strbuf_str(&c->reply,
> +				  "permission deny: need to be root\n");
> +		break;
> +	case -ETIMEDOUT:
> +		append_strbuf_str(&c->reply, "timeout\n");
> +		break;
> +	case 0:
>  		append_strbuf_str(&c->reply, "ok\n");
> -		r = 0;
> +		break;
> +	default:
> +		append_strbuf_str(&c->reply, "fail\n");
> +		break;
>  	}
> -	/* else if (r < 0) leave *reply alone */
> -	return r;
>  }
>  
>  static void set_client_state(struct client *c, int state)
> @@ -409,6 +379,7 @@ static void set_client_state(struct client *c, int state)
>  		reset_strbuf(&c->reply);
>  		memset(c->cmd, '\0', sizeof(c->cmd));
>  		c->expires = ts_zero;
> +		c->error = 0;
>  		/* fallthrough */
>  	case CLT_SEND:
>  		/* reuse these fields for next data transfer */
> @@ -420,10 +391,13 @@ static void set_client_state(struct client *c, int state)
>  	c->state = state;
>  }
>  
> -static void handle_client(struct client *c, void *trigger_data)
> +static void handle_client(struct client *c, struct vectors *vecs)
>  {
>  	ssize_t n;
>  
> +	condlog(4, "%s: cli[%d] state=%d cmd=\"%s\" repl \"%s\"", __func__,
> +		c->fd, c->state, c->cmd, get_strbuf_str(&c->reply));
> +
>  	switch (c->state) {
>  	case CLT_RECV:
>  		if (c->cmd_len == 0) {
> @@ -464,15 +438,52 @@ static void handle_client(struct client *c, void *trigger_data)
>  				return;
>  			set_client_state(c, CLT_PARSE);
>  		}
> -		break;
> -	default:
> -		break;
> -	}
> +		condlog(4, "cli[%d]: Got request [%s]", c->fd, c->cmd);
> +		/* fallthrough */
>  
> -	condlog(4, "cli[%d]: Got request [%s]", c->fd, c->cmd);
> -	uxsock_trigger(c, trigger_data);
> +	case CLT_PARSE:

Since you already print this information at the top of the function, it
seems liek it would make more sense to put these prints before the
followthroughs, so that they get double printed immediately when the
function is called for devices not in CLT_RECV.

> +		condlog(4, "%s: cli[%d] state=%d cmd=\"%s\" repl \"%s\"", __func__,
> +			c->fd, c->state, c->cmd,  get_strbuf_str(&c->reply));
> +		c->error = parse_cmd(c);
> +
> +		/* Permission check */
> +		if (c->error == 0 && c->cmdvec && VECTOR_SIZE(c->cmdvec) > 0) {
> +			struct key *kw = VECTOR_SLOT(c->cmdvec, 0);
> +
> +			if (!c->is_root && kw->code != LIST) {
> +				/* this will fall through to CLT_SEND */
> +				c->error = -EPERM;
> +				condlog(0, "%s: cli[%d]: unauthorized cmd \"%s\"",
> +					__func__, c->fd, c->cmd);
> +			}
> +		}
> +		set_client_state(c, CLT_WAIT_LOCK);

I don't have strong feelings about this, but this state machine doesn't
want to always fall through. sometimes, like if you get -EPERM, you want
to swith from CLT_PARSE to CLT_SEND.  If instead of fallthroughs, you
just put the switch statement in a loop, and simply returned when you
wanted to break out to uxsock_listen, you could jump from any state to
any other state, and wouldn't need to have code to skip the actions of
some states, to enable the follow throughs. Just a thought.

-Ben

> +		/* fallthrough */
> +
> +	case CLT_WAIT_LOCK:
> +		condlog(4, "%s: cli[%d] state=%d cmd=\"%s\" repl \"%s\"", __func__,
> +			c->fd, c->state, c->cmd, get_strbuf_str(&c->reply));
> +		/* tbd */
> +		set_client_state(c, CLT_WORK);
> +		/* fallthrough */
> +
> +	case CLT_WORK:
> +		condlog(4, "%s: cli[%d] state=%d cmd=\"%s\" repl \"%s\"", __func__,
> +			c->fd, c->state, c->cmd, get_strbuf_str(&c->reply));
> +		if (c->error == 0 && c->handler)
> +			c->error = execute_handler(c, vecs, uxsock_timeout / 1000);
> +
> +		if (c->cmdvec) {
> +			free_keys(c->cmdvec);
> +			c->cmdvec = NULL;
> +		}
> +		set_client_state(c, CLT_SEND);
> +		/* fallthrough */
> +
> +	case CLT_SEND:
> +		if (get_strbuf_len(&c->reply) == 0)
> +			default_reply(c, c->error);
>  
> -	if (get_strbuf_len(&c->reply) > 0) {
>  		const char *buf = get_strbuf_str(&c->reply);
>  
>  		if (send_packet(c->fd, buf) != 0)
> @@ -481,9 +492,13 @@ static void handle_client(struct client *c, void *trigger_data)
>  			condlog(4, "cli[%d]: Reply [%zu bytes]", c->fd,
>  				get_strbuf_len(&c->reply) + 1);
>  		reset_strbuf(&c->reply);
> -	}
>  
> -	set_client_state(c, CLT_RECV);
> +		set_client_state(c, CLT_RECV);
> +		break;
> +
> +	default:
> +		break;
> +	}
>  }
>  
>  /*
> -- 
> 2.33.0

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


  reply	other threads:[~2021-09-16  3:32 UTC|newest]

Thread overview: 89+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-09-10 11:40 [dm-devel] [PATCH 00/35] multipathd: uxlsnr overhaul mwilck
2021-09-10 11:40 ` [dm-devel] [PATCH 01/35] libmultipath: add timespeccmp() utility function mwilck
2021-09-15 22:07   ` Benjamin Marzinski
2021-09-10 11:40 ` [dm-devel] [PATCH 02/35] libmultipath: add trylock() helper mwilck
2021-09-15 22:07   ` Benjamin Marzinski
2021-09-10 11:40 ` [dm-devel] [PATCH 03/35] libmultipath: add optional wakeup functionality to lock.c mwilck
2021-09-15 22:13   ` Benjamin Marzinski
2021-09-10 11:40 ` [dm-devel] [PATCH 04/35] libmultipath: print: add __snprint_config() mwilck
2021-09-15 22:14   ` Benjamin Marzinski
2021-09-10 11:40 ` [dm-devel] [PATCH 05/35] libmultipath: improve cleanup of uevent queues on exit mwilck
2021-09-15 22:20   ` Benjamin Marzinski
2021-09-16  7:10     ` Martin Wilck
2021-09-16 14:26       ` Benjamin Marzinski
2021-09-10 11:40 ` [dm-devel] [PATCH 06/35] multipathd: fix systemd notification when stopping while reloading mwilck
2021-09-15 22:55   ` Benjamin Marzinski
2021-09-10 11:40 ` [dm-devel] [PATCH 07/35] multipathd: improve delayed reconfigure mwilck
2021-09-15 23:00   ` Benjamin Marzinski
2021-09-16  7:16     ` Martin Wilck
2021-09-10 11:40 ` [dm-devel] [PATCH 08/35] multipathd: cli.h: formatting improvements mwilck
2021-09-15 23:01   ` Benjamin Marzinski
2021-09-10 11:40 ` [dm-devel] [PATCH 09/35] multipathd: cli_del_map: fix reply for delayed action mwilck
2021-09-15 23:40   ` Benjamin Marzinski
2021-09-10 11:40 ` [dm-devel] [PATCH 10/35] multipathd: add prototype for cli_handler functions mwilck
2021-09-15 23:53   ` Benjamin Marzinski
2021-09-10 11:40 ` [dm-devel] [PATCH 11/35] multipathd: make all cli_handlers static mwilck
2021-09-15 23:53   ` Benjamin Marzinski
2021-09-10 11:40 ` [dm-devel] [PATCH 12/35] multipathd: add and set cli_handlers in a single step mwilck
2021-09-16  0:01   ` Benjamin Marzinski
2021-09-16  7:22     ` Martin Wilck
2021-11-12 21:45     ` Martin Wilck
2021-09-10 11:40 ` [dm-devel] [PATCH 13/35] multipathd: cli.c: use ESRCH for "command not found" mwilck
2021-09-16  0:02   ` Benjamin Marzinski
2021-09-10 11:40 ` [dm-devel] [PATCH 14/35] multipathd: add "force_reconfigure" option mwilck
2021-09-16  0:13   ` Benjamin Marzinski
2021-09-16  7:34     ` Martin Wilck
2021-09-16 14:32       ` Benjamin Marzinski
2021-09-10 11:41 ` [dm-devel] [PATCH 15/35] multipathd: uxlsnr: avoid stalled clients during reconfigure mwilck
2021-09-16  2:17   ` Benjamin Marzinski
2021-09-10 11:41 ` [dm-devel] [PATCH 16/35] multipathd: uxlsnr: handle client HUP mwilck
2021-09-16  2:17   ` Benjamin Marzinski
2021-09-10 11:41 ` [dm-devel] [PATCH 17/35] multipathd: uxlsnr: use symbolic values for pollfd indices mwilck
2021-09-16  2:18   ` Benjamin Marzinski
2021-09-10 11:41 ` [dm-devel] [PATCH 18/35] multipathd: uxlsnr: avoid using fd -1 in ppoll() mwilck
2021-09-16  2:18   ` Benjamin Marzinski
2021-09-10 11:41 ` [dm-devel] [PATCH 19/35] multipathd: uxlsnr: data structure for stateful client connection mwilck
2021-09-16  2:19   ` Benjamin Marzinski
2021-09-10 11:41 ` [dm-devel] [PATCH 20/35] multipathd: move uxsock_trigger() to uxlsnr.c mwilck
2021-09-16  2:19   ` Benjamin Marzinski
2021-09-10 11:41 ` [dm-devel] [PATCH 21/35] multipathd: move parse_cmd() " mwilck
2021-09-16  2:19   ` Benjamin Marzinski
2021-09-10 11:41 ` [dm-devel] [PATCH 22/35] multipathd: uxlsnr: remove check_timeout() mwilck
2021-09-16  2:21   ` Benjamin Marzinski
2021-09-10 11:41 ` [dm-devel] [PATCH 23/35] multipathd: uxlsnr: move client handling to separate function mwilck
2021-09-16  2:21   ` Benjamin Marzinski
2021-09-10 11:41 ` [dm-devel] [PATCH 24/35] multipathd: uxlsnr: use main poll loop for receiving mwilck
2021-09-16  2:22   ` Benjamin Marzinski
2021-09-10 11:41 ` [dm-devel] [PATCH 25/35] multipathd: use strbuf in cli_handler functions mwilck
2021-09-16  2:23   ` Benjamin Marzinski
2021-09-10 11:41 ` [dm-devel] [PATCH 26/35] multipathd: uxlsnr: check root on connection startup mwilck
2021-09-16  2:23   ` Benjamin Marzinski
2021-09-10 11:41 ` [dm-devel] [PATCH 27/35] multipathd: uxlsnr: pass struct client to uxsock_trigger() and parse_cmd() mwilck
2021-09-16  2:28   ` Benjamin Marzinski
2021-09-10 11:41 ` [dm-devel] [PATCH 28/35] multipathd: uxlsnr: move handler execution to separate function mwilck
2021-09-16  2:28   ` Benjamin Marzinski
2021-09-10 11:41 ` [dm-devel] [PATCH 29/35] multipathd: uxlsnr: use parser to determine non-root commands mwilck
2021-09-16  2:29   ` Benjamin Marzinski
2021-09-10 11:41 ` [dm-devel] [PATCH 30/35] multipathd: uxlsnr: merge uxsock_trigger() into state machine mwilck
2021-09-16  3:32   ` Benjamin Marzinski [this message]
2021-09-16  8:02     ` Martin Wilck
2021-11-12 22:07     ` Martin Wilck
2021-09-10 11:41 ` [dm-devel] [PATCH 31/35] multipathd: uxlsnr: add idle notification mwilck
2021-09-16  4:14   ` Benjamin Marzinski
2021-09-16  8:54     ` Martin Wilck
2021-09-16 15:06       ` Benjamin Marzinski
2021-09-16 15:54         ` Martin Wilck
2021-09-16 16:10           ` Benjamin Marzinski
2021-09-10 11:41 ` [dm-devel] [PATCH 32/35] multipathd: uxlsnr: add timeout handling mwilck
2021-09-16  4:17   ` Benjamin Marzinski
2021-09-16  8:58     ` Martin Wilck
2021-09-16 15:08       ` Benjamin Marzinski
2021-09-10 11:41 ` [dm-devel] [PATCH 33/35] multipathd: uxlsnr: use poll loop for sending, too mwilck
2021-09-16  4:22   ` Benjamin Marzinski
2021-09-16  9:33     ` Martin Wilck
2021-09-16 15:26       ` Benjamin Marzinski
2021-09-10 11:41 ` [dm-devel] [PATCH 34/35] multipathd: uxlsnr: drop client_lock mwilck
2021-09-16  4:24   ` Benjamin Marzinski
2021-09-16  9:34     ` Martin Wilck
2021-09-10 11:41 ` [dm-devel] [PATCH 35/35] multipathd: uxclt: allow client mode for non-root, too mwilck
2021-09-16  4:24   ` Benjamin Marzinski

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210916033237.GP3087@octiron.msp.redhat.com \
    --to=bmarzins@redhat.com \
    --cc=dm-devel@redhat.com \
    --cc=lixiaokeng@huawei.com \
    --cc=mwilck@suse.com \
    --cc=wu.chongyun@h3c.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).