User definable terminfo support
PATCH 1/1 User definable terminfo support
Attached are three patches....
enable-serial-module.diff
* conf/i386-pc.rmk (pkgdata_MODULES): Add serial.mod.
strcasecmp.diff
* kern/misc.c (grub_strcasecmp): New function.
* include/grub/misc.h (grub_strcasecmp): New function prototype.
terminfo-definition-support.diff
* term/terminfo.c: Replaced static vt100 definition with user
definable definition support.
* include/grub/terminfo.h (grub_terminfo_get_current): Removed. (grub_terminfo_set_current): Likewise
As with the previous serial patch, some code here is based on the terminfo support from GRUB Legacy. Any mistakes, however, are mine. :)
I would appreciate comments and suggestions, especially for the short parameter selection for the reverse video and cursor options.
I tried not using a short parameter, but state[x].set does not get set in that case, or I tried incorrectly.
Also can someone point me to instructions for creating a menu so that I may test other then the command line?
Thanks!
Signature · 1 line
Omniflux
include/grub/terminfo.h +0 -3
@@ -23,9 +23,6 @@#include <grub/err.h> #include <grub/types.h> -char *grub_terminfo_get_current (void ); -grub_err_t grub_terminfo_set_current (const char *); -void grub_terminfo_gotoxy (grub_uint8_t x,grub_uint8_t y);void grub_terminfo_cls (void );void grub_terminfo_reverse_video_on (void );
term/terminfo.c +437 -68
585 lines not on this page · raw message
conf/i386-pc.rmk +1 -1
@@ -115,7 +115,7 @@ # Modules. pkgdata_MODULES = _chain.mod _linux.mod linux.mod normal.mod vga.mod \ _multiboot.mod chain.mod multiboot.mod reboot.mod halt.mod \ - vbe.mod vesafb.mod vbetest.mod vbeinfo.mod play.mod + vbe.mod vesafb.mod vbetest.mod vbeinfo.mod play.mod serial.mod # For _chain.mod. _chain_mod_SOURCES = loader/i386/pc/chainloader.c
include/grub/misc.h +1 -0
@@ -44,6 +44,7 @@int EXPORT_FUNC (grub_memcmp ) (const void *s1,const void *s2,grub_size_t n);int EXPORT_FUNC (grub_strcmp ) (const char *s1,const char *s2);int EXPORT_FUNC (grub_strncmp ) (const char *s1,const char *s2,grub_size_t n); +int EXPORT_FUNC (grub_strcasecmp ) (const char *s1,const char *s2);int EXPORT_FUNC (grub_strncasecmp ) (const char *s1,const char *s2,int c);char *EXPORT_FUNC (grub_strchr ) (const char *s,int c);char *EXPORT_FUNC (grub_strrchr ) (const char *s,int c);
kern/misc.c +15 -0
@@ -209,6 +209,21 @@ }int +grub_strcasecmp (const char *s1,const char *s2) +{ +while (grub_tolower (*s1) &&grub_tolower (*s2)) + { +if (grub_tolower (*s1) !=grub_tolower (*s2)) +return (int )grub_tolower (*s1) - (int )grub_tolower (*s2); + + s1++; + s2++; + } + +return (int ) *s1 - (int ) *s2; +} + +int grub_strncasecmp (const char *s1,const char *s2,int c) {int p =1 ;
Omniflux <[email protected]> writes:
Hi,
Thanks a lot for your patch!
Quoted 5 lines
terminfo-definition-support.diff
*term/terminfo.c: Replaced static vt100 definition with user
definable definition support.
*include/grub/terminfo.h(grub_terminfo_get_current): Removed. (grub_terminfo_set_current): Likewise
Is it possible to do this in a GRUB env. variable, like:
set TERM=vt100
And perhaps one variable or function to set the possible terminal descriptions. I think that would be easier for the user to do it this way.
Quoted 8 lines
As with the previous serial patch, some code here is based on the terminfo support from GRUB Legacy. Any mistakes, however, are mine. :)
I would appreciate comments and suggestions, especially for the short parameter selection for the reverse video and cursor options.
I tried not using a short parameter, but state[x].set does not get set in that case, or I tried incorrectly.
Sure, I'll look at this code after the weekend. The feedback I will give will be more useful than the feedback above... Can you provide a ChangeLog entry so the patch can be reviewed and applied? See the GNU Coding Standards (GCS) for information about how to write a changelog entry. If you have questions about this, feel free to contact me.
Quoted 2 lines
Also can someone point me to instructions for creating a menu so that I may test other then the command line?
At the moment I am working on the menu code, so the syntax will be changed really soon. But at the moment you can do something like:
title foo
linux /vmlinux ...
title bar
...
Thanks,
Marco
Quoted 4 lines · Marco Gerards
Quoted 3 lines
terminfo-definition-support.diff
*term/terminfo.c: Replaced static vt100 definition with user
definable definition support.
Quoted 8 lines
Is it possible to do this in a GRUB env. variable, like:
set TERM=vt100
And perhaps one variable or function to set the possible terminal descriptions. I think that would be easier for the user to do it this way.
This is possible, however...
The reason I chose to use a linked list allowing multiple terminal definitions was so multiple entries could be defined at load time, and, ideally, the user could then choose the correct one at boot.
This would be helpful in cases where the choice of definitions is unknown to the entity creating the configuration, such as a live CD distribution or a generalized system recovery disk for computers without video cards.
As to using an env. variable to select which terminal definition to use,
I have not looked at the env. code. I doubt I can tie into the env. code to tell when the variable has changed, so this would leave the problem of no immediate user feedback if an invalid definition name is set.
If you can point to a good place to provide this feedback, I see no problem with changing the selection code to use an environment variable instead, but I would like to keep support for multiple terminfo definitions.
Did I interpret your comments correctly?
Quoted 2 lines
give will be more useful than the feedback above... Can you provide a ChangeLog entry so the patch can be reviewed and applied? See the GNU
Should this be provided as part of the patch, or separate, but with the patch email?
Quoted 8 lines
At the moment I am working on the menu code, so the syntax will be changed really soon. But at the moment you can do something like:
title foo
linux /vmlinux ...title bar
...
I will try this.
Thanks!
Signature · 1 line
Omniflux
Quoted 13 lines · Omniflux
Quoted 9 lines · Marco Gerards
Quoted 3 lines
terminfo-definition-support.diff
*term/terminfo.c: Replaced static vt100 definition with user
definable definition support.Is it possible to do this in a GRUB env. variable, like:
set TERM=vt100
And perhaps one variable or function to set the possible terminal descriptions. I think that would be easier for the user to do it this way.This is possible, however...
Quoted 8 lines
The reason I chose to use a linked list allowing multiple terminal definitions was so multiple entries could be defined at load time, and, ideally, the user could then choose the correct one at boot.
This would be helpful in cases where the choice of definitions is unknown to the entity creating the configuration, such as a live CD distribution or a generalized system recovery disk for computers without video cards.
Understood. But I think you misunderstood me.
What I had in mind is:
TERM=vt100, or whatever can be used to select the terminal. People are used to this from the UNIX commandline. Besides that, we prefer having variables in GRUB instead of commands.
I was wondering if it would be easier to set terminfo descriptions using variables as well. For example:
set vt100='cub=\E[%p1%dD, cub1=^H, cud=\E[%p1%dB, cud1=^J' (I just used some random stuff, but I assume you get the idea)
After that:
set TERM=vt100
That will read and parse the vt100 variable. We can add several pre-defined variables.
Hopefully my explanation makes sense.
Quoted 5 lines
As to using an env. variable to select which terminal definition to use, I have not looked at the env. code. I doubt I can tie into the env. code to tell when the variable has changed, so this would leave the problem of no immediate user feedback if an invalid definition name is set.
It's possible. You can define a call-back function for when a variable is read from or written to. So you can provide feedback (although I wonder if it is sane to do), change stuff, etc.
Quoted 4 lines
If you can point to a good place to provide this feedback, I see no problem with changing the selection code to use an environment variable instead, but I would like to keep support for multiple terminfo definitions.
Multiple terminfo definitions is nice. That's what this patch was all about, so I definately have nothing against that. ;-)
Quoted 2 lines
Did I interpret your comments correctly?
Partially. ;)
I'm sorry for my terrible English.
Quoted 5 lines
Quoted 2 lines
give will be more useful than the feedback above... Can you provide a ChangeLog entry so the patch can be reviewed and applied? See the GNU
Should this be provided as part of the patch, or separate, but with the patch email?
Separate in the same email.
Thanks,
Marco
Quoted 12 lines · Marco Gerards
What I had in mind is:
TERM=vt100, or whatever can be used to select the terminal. People are used to this from the UNIX commandline. Besides that, we prefer having variables in GRUB instead of commands.
I was wondering if it would be easier to set terminfo descriptions using variables as well. For example:
set vt100='cub=\E[%p1%dD, cub1=^H, cud=\E[%p1%dB, cud1=^J' (I just used some random stuff, but I assume you get the idea)
Quoted 7 lines
After that:
set TERM=vt100That will read and parse the vt100 variable. We can add several pre-defined variables.
Hopefully my explanation makes sense.
This makes sense. I do not like allowing TERM to be set to just any variable name (set TERM=PROMPT), but as this is really in the users configuration space, I suppose we can place the requirement on the user to issue sane assignments, and if they do not, it is their own fault, provided I document it well.
Quoted 1 line
I'm sorry for my terrible English.
Actually, I cannot tell it is not your native language from your writing.
I will make your suggested changes and submit another patch to the list as soon as I have time again.
Thanks!
Signature · 3 lines
Omniflux -- Omniflux
Quoted 15 lines · Omniflux
Quoted 5 lines · Marco Gerards
What I had in mind is:
TERM=vt100, or whatever can be used to select the terminal. People are used to this from the UNIX commandline. Besides that, we prefer having variables in GRUB instead of commands.
I was wondering if it would be easier to set terminfo descriptionsQuoted 8 lines
using variables as well. For example:
set vt100='cub=\E[%p1%dD, cub1=^H, cud=\E[%p1%dB, cud1=^J' (I just used some random stuff, but I assume you get the idea) After that:
set TERM=vt100
That will read and parse the vt100 variable. We can add several pre-defined variables.
Hopefully my explanation makes sense.
Quoted 5 lines
This makes sense. I do not like allowing TERM to be set to just any variable name (set TERM=PROMPT), but as this is really in the users configuration space, I suppose we can place the requirement on the user to issue sane assignments, and if they do not, it is their own fault, provided I document it well.
Agreed.
Quoted 2 lines
I will make your suggested changes and submit another patch to the list as soon as I have time again.
Great! I am looking forwards to it!
Thanks,
Marco
Omniflux <[email protected]> writes:
Hi,
Here is the review of the code that I promised. Please see my other email about generic comments.
term/terminfo.c line 93
Quoted 6 lines
term/terminfo.c +4
-1+/* Delete terminfo definition from list. */ +static void +delete_definition (terminfo_t *ti) { -return term.name; +terminfo_t *p, *q;
Please just declare one declaration at a time. Better would be:
terminfo_t *p;
terminfo_t *q;term/terminfo.c line 195
Quoted 5 lines
term/terminfo.c +5
-0+/* Unescape a string. */ +static char * +unescape_string (const char *in) +{ +char *q, *new_string;
Same here.
Quoted 1 line
+
new_string= (char *)grub_malloc(grub_strlen(in) + 1); /* New string will be <= current string. */
This line is way too long. Please make sure lines are 78 characters or smaller. The best thing to do here is putting the comment above this line. Same for the comments below.
Most error strings below are also too long:
term/terminfo.c line 358
Quoted 2 lines
term/terminfo.c +2
-0+if (i >1 ) +return grub_error (GRUB_ERR_INVALID_COMMAND ,"add, modify, delete and view are exclusive operations." );
You can put the string on a new line. If the string is still too long you can make two strings out of it, like:
grub_error (FOO,
"bar"
"baz");Quoted 1 line
+
ti->name=grub_strdup(args[0]);
Typo.
term/terminfo.c line 528
Quoted 7 lines
term/terminfo.c +7
-0+static void +setup_defaults (void ) +{ +terminfo_t *ti; + + ti = (terminfo_t *)grub_malloc (sizeof (terminfo_t)); +/* Do I need to test if malloc succeeded here? What do I do if it did not? */
Good one :-)
In that case default could be set to NULL, if that is allowed?
term/terminfo.c line 538
Quoted 9 lines
term/terminfo.c +9
-0+/* Default terminal definition vt100. */ + ti->name =grub_strdup ("vt100" ); + ti->gotoxy =grub_strdup ("\e[%i%p1%d;%p2%dH" ); + ti->cls =grub_strdup ("\e[H\e[J" ); + ti->reverse_video_on =grub_strdup ("\e[7m" ); + ti->reverse_video_off =grub_strdup ("\e[m" ); + ti->cursor_on =grub_strdup ("\e[?25l" ); + ti->cursor_off =grub_strdup ("\e[?25h" ); + ti->next =0 ;
Thanks,
Marco
Quoted 2 lines · Marco Gerards
Here is the review of the code that I promised. Please see my other email about generic comments.
What is the subject line of this message? I sadly do not read all messages on this list.
Quoted 6 lines
You can put the string on a new line. If the string is still too long you can make two strings out of it, like:
grub_error(FOO,"bar" "baz");
grub_error (FOO, "bar" "baz");
No operator or separator between strings?
Quoted 4 lines
Quoted 1 line
+
ti->name=grub_strdup(args[0]);Typo.
Can you be more specific? This looks correct to me...
Quoted 11 lines
term/terminfo.c line 528
Quoted 7 lines
term/terminfo.c +7
-0+static void +setup_defaults (void ) +{ +terminfo_t *ti; + + ti = (terminfo_t *)grub_malloc (sizeof (terminfo_t)); +/* Do I need to test if malloc succeeded here? What do I do if it did not? */ Good one :-)
Quoted 1 line
In that case default could be set to NULL, if that is allowed?
I think in this case, there will be many other problems. I think the best thing in this case will be, as you suggest, to set the definition to NULL and have all terminfo functions return their strings unmodified.
The only consequence to this I can think of is loss of menu support on terminals which depend on terminfo (serial), and should be a rare case even there.
Signature · 1 line
Omniflux
Quoted 6 lines · Omniflux
Quoted 2 lines · Marco Gerards
Here is the review of the code that I promised. Please see my other email about generic comments.
What is the subject line of this message? I sadly do not read all messages on this list.
You just answered it, so I guess you found it. ;-)
Quoted 8 lines
Quoted 5 lines
You can put the string on a new line. If the string is still too long you can make two strings out of it, like:
grub_error(FOO,"bar" "baz");
grub_error(FOO, "bar" "baz");
No operator or separator between strings?
Right. That will concatenate them and result in the string "barbaz".
Quoted 4 lines
Quoted 2 lines
Quoted 1 line
+
ti->name=grub_strdup(args[0]);Typo.
Can you be more specific? This looks correct to me...
Can you change that to:
ti->name = grub_strdup (args[0]);
(So only one space before `=')
Quoted 10 lines
Quoted 9 lines
term/terminfo.c line 528
Quoted 7 lines
term/terminfo.c +7
-0+static void +setup_defaults (void ) +{ +terminfo_t *ti; + + ti = (terminfo_t *)grub_malloc (sizeof (terminfo_t)); +/* Do I need to test if malloc succeeded here? What do I do if it did not? */ Good one :-)
In that case default could be set to NULL, if that is allowed?
Quoted 8 lines
I think in this case, there will be many other problems. I think the best thing in this case will be, as you suggest, to set the definition to NULL and have all terminfo functions return their strings unmodified.
The only consequence to this I can think of is loss of menu support on terminals which depend on terminfo (serial), and should be a rare case even there.
Another thing you could do is not malloc'ing memory, but using a pointer to a pre-defined struct instead. When free'ing you need a special case to check if it is this pre-defined struct.
--
Marco