[prev in list] [next in list] [prev in thread] [next in thread] 

List:       linux-man
Subject:    Re: [patch] vcs.4: broken example code
From:       Michael Witten <mfwitten () gmail ! com>
Date:       2018-06-25 21:32:24
Message-ID: 4252354debfb4394bda2a2567ce5cf38-mfwitten () gmail ! com
[Download RAW message or body]

On Mon, 25 Jun 2018 17:56:21 +0200, Mattias Engdegard wrote:

> Fix broken example code in the vcs.4 man page:
>
> - use of wrong variable (attrib, which is uninitialised, instead of s)
> - variable ch too narrow
> - printing a font char index with %c, as if it were ASCII (it's not)
> - removing the high font bit while changing the background colour
> 
> Also be friendly and use SEEK_* instead of numbers.
>
>
> diff --git a/man4/vcs.4 b/man4/vcs.4
> index aebe8cfda..c8b8cf361 100644
> --- a/man4/vcs.4
> +++ b/man4/vcs.4
> @@ -140,7 +140,8 @@ main(void)
>      struct {unsigned char lines, cols, x, y;} scrn;
>      unsigned short s;
>      unsigned short mask;
> -    unsigned char ch, attrib;
> +    unsigned char attrib, a;
> +    int ch;
>  
>      fd = open(console, O_RDWR);
>      if (fd < 0) {
> @@ -158,16 +159,16 @@ main(void)
>          exit(EXIT_FAILURE);
>      }
>      (void) read(fd, &scrn, 4);
> -    (void) lseek(fd, 4 + 2*(scrn.y*scrn.cols + scrn.x), 0);
> +    (void) lseek(fd, 4 + 2*(scrn.y*scrn.cols + scrn.x), SEEK_SET);
>      (void) read(fd, &s, 2);
>      ch = s & 0xff;
> -    if (attrib & mask)
> +    if (s & mask)
>          ch |= 0x100;
>      attrib = ((s & ~mask) >> 8);
> -    printf("ch=\(aq%c\(aq attrib=0x%02x\\n", ch, attrib);
> -    attrib ^= 0x10;
> -    (void) lseek(fd, \-1, 1);
> -    (void) write(fd, &attrib, 1);
> +    printf("ch=0x%03x attrib=0x%02x\\n", ch, attrib);
> +    a = (s >> 8) ^ 0x10;
> +    (void) lseek(fd, \-1, SEEK_CUR);
> +    (void) write(fd, &a, 1);
>      exit(EXIT_SUCCESS);
>  }
>  .EE

Ha! This is the erroneous code  example that prompted me to overhaul
all of  `vcs.4' (hopefully,  I'll be submitting  a patch  soon), and
this work further prompted me to write the proposed new page:

  byte.7
  Message-ID: <88a3d23bc0b14bf89ea303ae82468f59-mfwitten@gmail.com>
  https://www.spinics.net/lists/linux-man/msg13143.html

I'm actually a little surprised to see that someone else came across
it, too; I  figured that `vcs.4' is, these days,  a fairly forgotten
topic, but I guess I was wrong!

Anyway, putting aside the fact  that this code example makes certain
assumptions about the  sizes of data types (including  a `short', as
not even POSIX constrains it to exactly 16 bits), there is still the
issue of endianness. Namely, the following works only on machines of
little-endian nature:

  a = (s >> 8) ^ 0x10;
  (void) lseek(fd, -1, SEEK_CUR);
  (void) write(fd, &a, 1);

A more  widely applicable example  cannot merely step back  one byte
and then replace what's there; instead, it must be aware of the byte
order in  which the 2-byte  character/attribute pair is  stored. One
way  to achieve  the correct  results is  to construct  a whole  new
2-byte character/attribute pair,  step back 2 bytes,  and then write
that new pair in place. So, it could be something more like this:

  s ^= (0x10 << 8);
  (void) lseek(fd, -2, SEEK_CUR);
  (void) write(fd, &s, 2);

To add  a little clarity to  our collision here, I  think your patch
should be applied (perhaps with the endianness issue shored up), and
I'll plan to rebase my renovation of `vcs.4' on top of your patch.

Sincerely,
Michael Witten
--
To unsubscribe from this list: send the line "unsubscribe linux-man" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
[prev in list] [next in list] [prev in thread] [next in thread]