Re: [Xen-devel] [PATCH] xenconsole: add option to avoid escape sequences in log

[Christopher Clark <christopher.w.clark@xxxxxxxxx>]

On Wed, Jul 25, 2018 at 1:53 AM, Wei Liu <wei.liu2@xxxxxxxxxx> wrote:

On Wed, Jul 25, 2018 at 09:49:39AM +0100, Wei Liu wrote:
> On Sat, Jul 21, 2018 at 02:14:12AM +0200, Marek Marczykowski-Górecki wrote:
>
> > +
> > +   memcpy(dest, buf, len);
> > +   for (i = 0; i < len; i++) {
> > +           if (dest[i] == '\033')
> > +                   dest[i] = '.';
> > +   }
>
> This could be made more efficient by using:
>
>         for (i = 0; i < len; i++) {
>              if (src[i] == '\033')
>                  dst[i] = '.';
>              else
>                  dst[i] = src[i];

The above code doesn't write the value that was checked into the destination buffer; instead it does a second copy from the source buffer. That is a problematic code pattern that we shouldn't really encourage.

 

Oh this can even be written in a shorter form:

  dst[i] = src[i] != '\033' ?: '.';

That may be shorter but it is harder to understand, whereas Marek's original code is clear and well structured.

One nit with the proposed patch is the introduction of the dynamically size array on the stack, with input to the function determining its size:

>   static int write_all(int fd, const char* buf, size_t len)

>   {

> +       char buf_replaced[len];

That might be just fine for userspace code, but would it be better replaced with a fixed size buffer and a loop instead?

Christopher

 

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel