PATCH 1/2: Make gotoxy & siblings use unsigned variables

PATCH Nigel Cunningham 6 participants 9 messages Jan 9, 2004 – Jan 13, 2004

Subsystems (2)

TTY layer and serial drivers Char and misc drivers

lore

Share & tools

lore

Nigel Cunningham

author

2004-01-09 18:43 UTC #0 lore raw

This patch makes console X and Y coordinates unsigned, rather than signed. Issues with wide (> 128 char?) consoles, seen when developing Software Suspend's 'nice display' are thus fixed. A brief examination of related code showed that this use of signed variables was the exception rather than the rule.

Regards,

Nigel

drivers/char/vt.c +6 -6

diff -ruN linux-2.6.1-rc3/drivers/char/vt.c console-patch-1/drivers/char/vt.c
@@ -149,7 +149,7 @@
 static void vc_init(unsigned int console, unsigned int rows,
 		    unsigned int cols, int do_clear);
 static void blank_screen(unsigned long dummy);
-static void gotoxy(int currcons, int new_x, int new_y);
+static void gotoxy(int currcons, unsigned int new_x, unsigned int new_y);
 static void save_cur(int currcons);
 static void reset_terminal(int currcons, int do_clear);
 static void con_flush_chars(struct tty_struct *tty);

@@ -859,9 +859,9 @@
  * might also be negative. If the given position is out of
  * bounds, the cursor is placed at the nearest margin.
  */
-static void gotoxy(int currcons, int new_x, int new_y)
+static void gotoxy(int currcons, unsigned int new_x, unsigned int new_y)
 {
-	int min_y, max_y;
+	unsigned int min_y, max_y;
 
 	if (new_x < 0)
 		x = 0;

@@ -888,7 +888,7 @@
 }
 
 /* for absolute user moves, when decom is set */
-static void gotoxay(int currcons, int new_x, int new_y)
+static void gotoxay(int currcons, unsigned int new_x, unsigned int new_y)
 {
 	gotoxy(currcons, new_x, decom ? (top+new_y) : new_y);
 }

@@ -2996,13 +2996,13 @@
 	return screenpos(currcons, 2 * w_offset, viewed);
 }
 
-void getconsxy(int currcons, char *p)
+void getconsxy(int currcons, unsigned char *p)
 {
 	p[0] = x;
 	p[1] = y;
 }
 
-void putconsxy(int currcons, char *p)
+void putconsxy(int currcons, unsigned char *p)
 {
 	gotoxy(currcons, p[0], p[1]);
 	set_cursor(currcons);

include/linux/selection.h +2 -2

diff -ruN linux-2.6.1-rc3/include/linux/selection.h console-patch-1/include/linux/selection.h
@@ -36,8 +36,8 @@
 extern void complement_pos(int currcons, int offset);
 extern void invert_screen(int currcons, int offset, int count, int shift);
 
-extern void getconsxy(int currcons, char *p);
-extern void putconsxy(int currcons, char *p);
+extern void getconsxy(int currcons, unsigned char *p);
+extern void putconsxy(int currcons, unsigned char *p);
 
 extern u16 vcs_scr_readw(int currcons, const u16 *org);
 extern void vcs_scr_writew(int currcons, u16 val, u16 *org);

Signature · 2 lines

My work on Software Suspend is graciously brought to you by
LinuxFund.org.

Richard B. Johnson

2004-01-09 19:22 UTC #1 lore raw

Quoted 9 lines · Nigel Cunningham

This patch makes console X and Y coordinates unsigned, rather than signed. Issues with wide (> 128 char?) consoles, seen when developing Software Suspend's 'nice display' are thus fixed. A brief examination of related code showed that this use of signed variables was the exception rather than the rule.
Regards,
Nigel

[SNIPPED...]

Question: Shouldn't we be using "size_t" for unsigned int, and "ssize_t" for signed? If the "ints" are going to be changed, they probably should be changed only once. As I recall, size_t was the largest unsigned int that would fit into a register and ssize_t was the largest signed int that would fit.

Cheers,

Dick Johnson
Penguin : Linux version 2.4.22 on an i686 machine (797.90 BogoMips).
Note 96.31% of all statistics are fiction.

Nigel Cunningham

author

2004-01-09 19:59 UTC #2 lore raw

You might be right. I was just being consistent with the other definitions. Andrew? Benjamin?

Regards,

Nigel

Quoted 31 lines · Richard B. Johnson

Quoted 9 lines · Nigel Cunningham
This patch makes console X and Y coordinates unsigned, rather than signed. Issues with wide (> 128 char?) consoles, seen when developing Software Suspend's 'nice display' are thus fixed. A brief examination of related code showed that this use of signed variables was the exception rather than the rule.
Regards,
Nigel
[SNIPPED...]
Question: Shouldn't we be using "size_t" for unsigned int, and "ssize_t" for signed? If the "ints" are going to be changed, they probably should be changed only once. As I recall, size_t was the largest unsigned int that would fit into a register and ssize_t was the largest signed int that would fit.
Cheers,
Dick Johnson
Penguin : Linux version 2.4.22 on an i686 machine (797.90 BogoMips).
Note 96.31% of all statistics are fiction.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Signature · 2 lines

My work on Software Suspend is graciously brought to you by
LinuxFund.org.

Andries Brouwer

2004-01-09 20:33 UTC #3 lore raw

Quoted 3 lines

Quoted 1 line
Shouldn't we be using "size_t" for unsigned int
You might be right. I was just being consistent with the other definitions.

These are character positions on a screen.
When did you last see a console in text mode with a line length of more than 2^31 ?

If you go for a minimal patch then you should replace "char" in one or two places by "unsigned char" and that is all.

Andrew Morton

2004-01-09 21:31 UTC #7 lore raw

In reply to Nigel Cunningham

Quoted 6 lines · Nigel Cunningham

This patch makes console X and Y coordinates unsigned, rather than signed. Issues with wide (> 128 char?) consoles, seen when developing Software Suspend's 'nice display' are thus fixed. A brief examination of related code showed that this use of signed variables was the exception rather than the rule.

You'll probably find that all you needed to do was to convert the chars over to unsigned, not the integers.

But whatever.

Nigel Cunningham

author

2004-01-09 21:36 UTC #4 lore raw

In reply to Andries Brouwer

Hi.

Of course you're right about 2^31 columns, but the rest of the code used unsigned ints as well, not because it expects 2^31 columns, but because (if I understand the code right), the numbers can be part of escape sequences... I'm looking at csi_m in vt.c.

Regards,

Nigel

Quoted 16 lines · Andries Brouwer

Quoted 3 lines
Quoted 1 line
Shouldn't we be using "size_t" for unsigned int
You might be right. I was just being consistent with the other definitions.
These are character positions on a screen.
When did you last see a console in text mode with a line length of more than 2^31 ?
If you go for a minimal patch then you should replace "char" in one or two places by "unsigned char" and that is all.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Signature · 2 lines

My work on Software Suspend is graciously brought to you by
LinuxFund.org.

Andrew Morton

2004-01-09 21:39 UTC #8 lore raw

In reply to Nigel Cunningham

Quoted 2 lines · Nigel Cunningham

X-Mailer: Ximian Evolution 1.4.4-8mdk

Gack. My MUA (sylpheed) is unable to decrypt the plain text after whatever it is that Evolution did to it. Please see if you can get Evolution to play more nicely with plain text, or use attachments.

I fixed that one up by hand.

Thanks.

Edgar Toernig

2004-01-10 03:34 UTC #5 lore raw

In reply to Nigel Cunningham

Quoted 6 lines · Nigel Cunningham

Quoted 3 lines · Andries Brouwer
When did you last see a console in text mode with a line length of more than 2^31 ?

Quoted 7 lines

Quoted 2 lines
If you go for a minimal patch then you should replace "char" in one or two places by "unsigned char" and that is all.
Of course you're right about 2^31 columns, but the rest of the code used unsigned ints as well, not because it expects 2^31 columns, but because (if I understand the code right), the numbers can be part of escape sequences... I'm looking at csi_m in vt.c.

Yes, they may be part of escape sequences and that's why your patch is wrong. See the comment above gotoxy:

/*
* gotoxy() must verify all boundaries, because the arguments
* might also be negative. If the given position is out of
* bounds, the cursor is placed at the nearest margin.
*/

If you make the arguments unsigned it will choose the wrong margin for negative values. That will i.e. happen if you send the sequence to move the cursor 10 spaces left when it is only in column 5.

Afaics, the only thing that needs fixing is putconsxy:

void putconsxy(int currcons, char *p)
{
- gotoxy(currcons, p[0], p[1]);

+        gotoxy(currcons, (unsigned char)p[0], (unsigned char)p[1]);
         set_cursor(currcons);

}

Ciao, ET.

Pavel Machek

2004-01-13 21:57 UTC #6 lore raw

In reply to Nigel Cunningham

Hi!

Quoted 2 lines

You might be right. I was just being consistent with the other definitions. Andrew? Benjamin?

I believe size_t is meant for file size, memory size and similar. You should stuck with int.
Pavel

Signature · 2 lines

When do you have a heart between your knees?
[Johanka's followup: and *two* hearts?]