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

[Wei Liu <wei.liu2@xxxxxxxxxx>]

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@xxxxxxxxxx>
> ---
> CC: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
> CC: Wei Liu <wei.liu2@xxxxxxxxxx>
> CC: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> CC: Julien Grall <julien.grall@xxxxxxx>
> 
> 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@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel