Re: [PATCH 13/17 v5] xen/arm: vpl011: Modify xenconsole to support multiple consoles

[Wei Liu <wei.liu2@citrix.com>]

On Thu, Jun 22, 2017 at 01:08:48PM +0530, Bhupinder Thakur wrote:
> This patch adds the support for multiple consoles and introduces the iterator
> functions to operate on multiple consoles.
> 
> This patch is in preparation to support a new vuart console.
> 
> Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org>
> ---
> CC: Ian Jackson <ian.jackson@eu.citrix.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> CC: Stefano Stabellini <sstabellini@kernel.org>
> CC: Julien Grall <julien.grall@arm.com>
> 
> Changes since v4:
> - Changes to make event channel handling per console rather than per domain.
> 
> Changes since v3:
> - The changes in xenconsole have been split into four patches. This is the third patch.
> 
>  tools/console/daemon/io.c | 435 ++++++++++++++++++++++++++++++++--------------
>  1 file changed, 302 insertions(+), 133 deletions(-)
> 
> diff --git a/tools/console/daemon/io.c b/tools/console/daemon/io.c
> index a2a3496..baf0e2e 100644
> --- a/tools/console/daemon/io.c
> +++ b/tools/console/daemon/io.c
> @@ -90,12 +90,14 @@ struct buffer {
>  };
>  
>  struct console {
> +	char *ttyname;
>  	int master_fd;
>  	int master_pollfd_idx;
>  	int slave_fd;
>  	int log_fd;
>  	struct buffer buffer;
>  	char *xspath;
> +	char *log_suffix;

I suppose both new fields can be const.

>  	int ring_ref;
>  	xenevtchn_handle *xce_handle;
>  	int xce_pollfd_idx;
> @@ -107,16 +109,112 @@ struct console {
>  	struct domain *d;
>  };
>  
> +struct console_data {
> +	char *xsname;
> +	char *ttyname;
> +	char *log_suffix;

const for all three.

> +};
> +
> +static struct console_data console_data[] = {
> +

Stray line.

> +	{
> +		.xsname = "/console",
> +		.ttyname = "tty",
> +		.log_suffix = "",
> +	},
> +};
> +
> +#define MAX_CONSOLE (sizeof(console_data)/sizeof(struct console_data))
> +
>  struct domain {
>  	int domid;
>  	bool is_dead;
>  	unsigned last_seen;
>  	struct domain *next;
> -	struct console console;
> +	struct console console[MAX_CONSOLE];
>  };
>  
>  static struct domain *dom_head;
>  
> +typedef void (*VOID_ITER_FUNC_ARG1)(struct console *);
> +typedef bool (*BOOL_ITER_FUNC_ARG1)(struct console *);
> +typedef int (*INT_ITER_FUNC_ARG1)(struct console *);
> +typedef void (*VOID_ITER_FUNC_ARG2)(struct console *,  void *);
> +typedef int (*INT_ITER_FUNC_ARG3)(struct console *,
> +			 struct domain *dom, void **);
> +
> +static inline bool console_enabled(struct console *con)
> +{
> +	return con->local_port != -1;
> +}
> +
> +static inline void console_iter_void_arg1(struct domain *d,
> +										  VOID_ITER_FUNC_ARG1 iter_func)

Too many tabs here and below?

You might want to configure your editor to display tab as 8 spaces.

> +{
> +	int i = 0;
> +	struct console *con = &(d->console[0]);
> +

No need to have the (), I think.

> -static struct domain *create_domain(int domid)
> +static int console_init(struct console *con, struct domain *dom, void **data)
>  {
> -	struct domain *dom;
>  	char *s;
> +	int err = -1;
>  	struct timespec ts;
> -	struct console *con;
> +	struct console_data **con_data = (struct console_data **)data;
> +	char *xsname;
>  
>  	if (clock_gettime(CLOCK_MONOTONIC, &ts) < 0) {
>  		dolog(LOG_ERR, "Cannot get time of day %s:%s:L%d",
>  		      __FILE__, __FUNCTION__, __LINE__);
> -		return NULL;
> +		return err;
> +	}
> +

There is a danger that you return at this point, the cleanup path in
caller will free garbage.

I suggest you at least initialise all pointers to NULL at the beginning.

> +	con->master_fd = -1;
> +	con->master_pollfd_idx = -1;
> +	con->slave_fd = -1;
> +	con->log_fd = -1;
> +	con->ring_ref = -1;
> +	con->local_port = -1;
> +	con->remote_port = -1;
> +	con->xce_pollfd_idx = -1;
> +	con->next_period = ((long long)ts.tv_sec * 1000) + (ts.tv_nsec / 1000000) + RATE_LIMIT_PERIOD;
> +	con->d = dom;
> +	con->ttyname = (*con_data)->ttyname;
> +	con->log_suffix = (*con_data)->log_suffix;
> +	xsname = (*con_data)->xsname;
> +	con->xspath = xs_get_domain_path(xs, dom->domid);
> +	s = realloc(con->xspath, strlen(con->xspath) +
> +				strlen(xsname) + 1);
> +	if (s)
> +	{
> +		con->xspath = s;
> +		strcat(con->xspath, xsname);
> +		err = 0;
>  	}
>  
> +	(*con_data)++;
> +
> +	return err;
> +}
> +
> +
[...]
> +static void handle_console_ring(struct console *con)
> +{
> +	if (con->event_count < RATE_LIMIT_ALLOWANCE) {
> +		if (con->xce_handle != NULL &&
> +			con->xce_pollfd_idx != -1 &&
> +			!(fds[con->xce_pollfd_idx].revents &
> +			  ~(POLLIN|POLLOUT|POLLPRI)) &&
> +			  (fds[con->xce_pollfd_idx].revents &
> +			   POLLIN))
> +			handle_ring_read(con);
> +	}

Refactoring like this should go to its own patch(es).

It is currently very hard to review this patch because refactoring is
mixed with all the iterator changes.

I can't really continue at this point. Sorry. Please split the
refactoring of all the buffer_* and handle_console_* functions to
separate patches.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel