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

[Bhupinder Thakur <bhupinder.thakur@xxxxxxxxxx>]

Hi Wei,


>>
>>  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.
ok. I will make all the new fields as const char *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.
ok.
>

>> +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.
I have set  my editor to use 8 spaces for a tab. I will fix these misalignments.

>
>> +{
>> +     int i = 0;
>> +     struct console *con = &(d->console[0]);
>> +
>
> No need to have the (), I think.
ok.
>
>> -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.
>
I am checking that the pointer is not null before freeing them.

>> +     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.

I have split the changes such that each new function and the related
changes appear in one patch.

Regards,
Bhupinder

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