Re: [PATCH v2 29/48] multipathd: uxlsnr: merge uxsock_trigger() into state machine

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

 



On Thu, Nov 18, 2021 at 11:58:21PM +0100, mwilck@xxxxxxxx wrote:
> From: Martin Wilck <mwilck@xxxxxxxx>
> 
> This patch sets up the bulk of the state machine. client_state_machine()
> is called in a loop, proceeding from state to state until it needs
> to poll for input or wait for a lock, in which case it returns
> STM_BREAK.
> 
> 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".
> 

Actually, one nitpick. See below

> Signed-off-by: Martin Wilck <mwilck@xxxxxxxx>
> ---
>  multipathd/uxlsnr.c | 160 ++++++++++++++++++++++++--------------------
>  1 file changed, 89 insertions(+), 71 deletions(-)
> 
> diff --git a/multipathd/uxlsnr.c b/multipathd/uxlsnr.c
> index ff9604f..87134d5 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,30 @@ 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:
> +		/* 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:
> +		/* cli_handler functions return 1 on unspecified error */
> +		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 +380,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,11 +392,20 @@ static void set_client_state(struct client *c, int state)
>  	c->state = state;
>  }
>  
> -static void handle_client(struct client *c, void *trigger_data)
> +enum {
> +	STM_CONT,
> +	STM_BREAK,
> +};
> +
> +static int client_state_machine(struct client *c, struct vectors *vecs)
>  {
>  	ssize_t n;
> +	const char *buf;
>  
> -	switch (c->state) {
> +	condlog(4, "%s: cli[%d] state=%d cmd=\"%s\" repl \"%s\"", __func__,
> +		c->fd, c->state, c->cmd, get_strbuf_str(&c->reply));
> +

This switch statement is indented with 8 spaces, instead of a tab

-Ben

> +        switch (c->state) {
>  	case CLT_RECV:
>  		if (c->cmd_len == 0) {
>  			/*
> @@ -449,31 +430,59 @@ static void handle_client(struct client *c, void *trigger_data)
>  				condlog(4, "%s: cli[%d]: connected", __func__, c->fd);
>  			}
>  			/* poll for data */
> -			return;
> +			return STM_BREAK;
>  		} else if (c->len < c->cmd_len) {
>  			n = recv(c->fd, c->cmd + c->len, c->cmd_len - c->len, 0);
>  			if (n <= 0 && errno != EINTR && errno != EAGAIN) {
>  				condlog(1, "%s: cli[%d]: error in recv: %m",
>  					__func__, c->fd);
>  				c->error = -ECONNRESET;
> -				return;
> +				return STM_BREAK;
>  			}
>  			c->len += n;
>  			if (c->len < c->cmd_len)
>  				/* continue polling */
> -				return;
> -			set_client_state(c, CLT_PARSE);
> +				return STM_BREAK;
>  		}
> -		break;
> -	default:
> -		break;
> -	}
> +		condlog(4, "cli[%d]: Got request [%s]", c->fd, c->cmd);
> +		set_client_state(c, CLT_PARSE);
> +		return STM_CONT;
>  
> -	condlog(4, "cli[%d]: Got request [%s]", c->fd, c->cmd);
> -	uxsock_trigger(c, trigger_data);
> +	case CLT_PARSE:
> +		c->error = parse_cmd(c);
> +		if (!c->error) {
> +			/* Permission check */
> +			struct key *kw = VECTOR_SLOT(c->cmdvec, 0);
>  
> -	if (get_strbuf_len(&c->reply) > 0) {
> -		const char *buf = get_strbuf_str(&c->reply);
> +			if (!c->is_root && kw->code != LIST) {
> +				c->error = -EPERM;
> +				condlog(0, "%s: cli[%d]: unauthorized cmd \"%s\"",
> +					__func__, c->fd, c->cmd);
> +			}
> +		}
> +		if (c->error)
> +			set_client_state(c, CLT_SEND);
> +		else
> +			set_client_state(c, CLT_WORK);
> +		return STM_CONT;
> +
> +	case CLT_WAIT_LOCK:
> +		/* tbd */
> +		set_client_state(c, CLT_WORK);
> +		return STM_CONT;
> +
> +	case CLT_WORK:
> +		c->error = execute_handler(c, vecs, uxsock_timeout / 1000);
> +		free_keys(c->cmdvec);
> +		c->cmdvec = NULL;
> +		set_client_state(c, CLT_SEND);
> +		return STM_CONT;
> +
> +	case CLT_SEND:
> +		if (get_strbuf_len(&c->reply) == 0)
> +			default_reply(c, c->error);
> +
> +		buf = get_strbuf_str(&c->reply);
>  
>  		if (send_packet(c->fd, buf) != 0)
>  			dead_client(c);
> @@ -481,9 +490,18 @@ 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);
> +		return STM_BREAK;
> +
> +	default:
> +		return STM_BREAK;
> +	}
> +}
> +
> +static void handle_client(struct client *c, struct vectors *vecs)
> +{
> +	while (client_state_machine(c, vecs) == STM_CONT);
>  }
>  
>  /*
> -- 
> 2.33.1

--
dm-devel mailing list
dm-devel@xxxxxxxxxx
https://listman.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