[PATCH] User definable terminfo support

[PATCH] User definable terminfo support

[Omniflux]

[-- Attachment #1: Type: text/plain, Size: 1005 bytes --]

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!

-- 
Omniflux

[-- Attachment #2: terminfo-definition-support.diff --]
[-- Type: text/plain, Size: 17880 bytes --]

diff -uNr grub2.cvs/include/grub/terminfo.h grub2/include/grub/terminfo.h
--- grub2.cvs/include/grub/terminfo.h	2005-09-03 10:54:26.000000000 -0600
+++ grub2/include/grub/terminfo.h	2005-12-27 16:57:57.000000000 -0700
@@ -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);
diff -uNr grub2.cvs/term/terminfo.c grub2/term/terminfo.c
--- grub2.cvs/term/terminfo.c	2005-11-13 08:47:09.000000000 -0700
+++ grub2/term/terminfo.c	2005-12-27 16:57:57.000000000 -0700
@@ -33,78 +33,244 @@
 #include <grub/terminfo.h>
 #include <grub/tparm.h>
 
+/* A terminfo definition.  */
 struct terminfo
 {
   char *name;
-
   char *gotoxy;
   char *cls;
   char *reverse_video_on;
   char *reverse_video_off;
   char *cursor_on;
   char *cursor_off;
+
+  struct terminfo *next;
 };
+typedef struct terminfo terminfo_t;
+
+/* The list of terminfo definitions.  */
+static terminfo_t *terminfo_list;
+
+/* The current terminfo definition.  */
+static terminfo_t *cur_terminfo;
+
+/* Argument options.  */
+static const struct grub_arg_option options[] =
+{
+  {"add",            'a', 0, "Add a definition", 0, 0},
+  {"modify",         'm', 0, "Modify a definition", 0, 0},
+  {"delete",         'd', 0, "Delete a definition", 0, 0},
+  {"view",           'v', 0, "View a definition", 0, 0},
+  {"cursor-address", 'p', 0, "Sequence to position cursor", 0, ARG_TYPE_STRING},
+  {"clear-screen",   'c', 0, "Sequence to clear screen", 0, ARG_TYPE_STRING},
+  {"reverse-on",     'w', 0, "Sequence to enable reverse video mode", 0, ARG_TYPE_STRING},
+  {"reverse-off",    'x', 0, "Sequence to disable reverse video mode", 0, ARG_TYPE_STRING},
+  {"cursor-on",      'y', 0, "Sequence to enable cursor", 0, ARG_TYPE_STRING},
+  {"cursor-off",     'z', 0, "Sequence to disable cursor", 0, ARG_TYPE_STRING},
+  {0, 0, 0, 0, 0, 0}
+};
+
+/* Add new terminfo definition to list.  */
+static grub_err_t
+add_definition (terminfo_t *ti)
+{
+  terminfo_t *p;
 
-static struct terminfo term;
+  for (p = terminfo_list; p; p = p->next)
+    if (grub_strcasecmp (p->name, ti->name) == 0)
+      return grub_error (GRUB_ERR_BAD_ARGUMENT, "terminfo definition name already exists.");
 
-/* Get current terminfo name.  */
-char *
-grub_terminfo_get_current (void)
+  ti->next = terminfo_list;
+  terminfo_list = ti;
+
+  return GRUB_ERR_NONE;
+}
+
+/* Delete terminfo definition from list.  */
+static void
+delete_definition (terminfo_t *ti)
 {
-  return term.name;
+  terminfo_t *p, *q;
+
+  for (p = 0, q = terminfo_list; q; p = q, q = q->next)
+    if (q == ti)
+      {
+        if (p)
+          p->next = q->next;
+        else
+          terminfo_list = q->next;
+
+        grub_free (q->name);
+        grub_free (q->gotoxy);
+        grub_free (q->cls);
+        grub_free (q->reverse_video_on);
+        grub_free (q->reverse_video_off);
+        grub_free (q->cursor_on);
+        grub_free (q->cursor_off);
+        grub_free (q);
+
+        break;
+      }
 }
 
-/* Free *PTR and set *PTR to NULL, to prevent double-free.  */
+/* Iterate through terminfo definitions.  */
 static void
-grub_terminfo_free (char **ptr)
+iterate_definitions (int (*hook) (terminfo_t *ti))
 {
-  grub_free (*ptr);
-  *ptr = 0;
+  terminfo_t *p;
+
+  for (p = terminfo_list; p; p = p->next)
+    if (hook (p))
+      break;
+}
+
+/* Escape a string.  */
+static char *
+escape_string (const char *in)
+{
+  char *q, *new_string;
+
+  new_string = (char *) grub_malloc (grub_strlen (in) * 4 + 1);	/* Max escaped string size is 4x.  */
+
+  for (q = new_string; *in; in++, q++)
+    {
+      switch (*in)
+        {
+          case '\a':	/* Common escape sequences.  */
+            *q++ = '\\';
+            *q = 'a';
+            break;
+	  case '\b':
+            *q++ = '\\';
+            *q = 'b';
+            break;
+          case '\e':
+            *q++ = '\\';
+            *q = 'e';
+            break;
+          case '\f':
+            *q++ = '\\';
+            *q = 'f';
+            break;
+          case '\n':
+            *q++ = '\\';
+            *q = 'n';
+            break;
+          case '\r':
+            *q++ = '\\';
+            *q = 'r';
+            break;
+          case '\t':
+            *q++ = '\\';
+            *q = 't';
+            break;
+          case '\v':
+            *q++ = '\\';
+            *q = 'v';
+            break;
+          case '\\':
+            *q++ = '\\';
+            *q = '\\';
+            break;
+          case 32 ... 91:	/* Typeable character. Leave as is.  */
+          case 93 ... 126:
+            *q = *in;
+            break;
+          default:	/* Convert to octal sequence.  */
+            *q++ = '\\';
+            *q++ = ((*in >> 8) & 7) + '0';
+            *q++ = ((*in >> 4) & 7) + '0';
+            *q = ((*in >> 0) & 7) + '0';
+        }
+    }
+
+  *q = 0;
+  return grub_realloc (new_string, grub_strlen (new_string) + 1);	/* Shrink alloc.  */
 }
 
-/* Set current terminfo type.  */
-grub_err_t
-grub_terminfo_set_current (const char *str)
-{
-  /* TODO
-   * Lookup user specified terminfo type. If found, set term variables
-   * as appropriate. Otherwise return an error.
-   *
-   * How should this be done?
-   *  a. A static table included in this module.
-   *     - I do not like this idea.
-   *  b. A table stored in the configuration directory.
-   *     - Users must convert their terminfo settings if we have not already.
-   *  c. Look for terminfo files in the configuration directory.
-   *     - /usr/share/terminfo is 6.3M on my system.
-   *     - /usr/share/terminfo is not on most users boot partition.
-   *     + Copying the terminfo files you want to use to the grub
-   *       configuration directory is easier then (b).
-   *  d. Your idea here.
-   */
-
-  /* Free previously allocated memory.  */
-  grub_terminfo_free (&term.name);
-  grub_terminfo_free (&term.gotoxy);
-  grub_terminfo_free (&term.cls);
-  grub_terminfo_free (&term.reverse_video_on);
-  grub_terminfo_free (&term.reverse_video_off);
-  grub_terminfo_free (&term.cursor_on);
-  grub_terminfo_free (&term.cursor_off);
-  
-  if (grub_strcmp ("vt100", str) == 0)
-    {
-      term.name              = grub_strdup ("vt100");
-      term.gotoxy            = grub_strdup ("\e[%i%p1%d;%p2%dH");
-      term.cls               = grub_strdup ("\e[H\e[J");
-      term.reverse_video_on  = grub_strdup ("\e[7m");
-      term.reverse_video_off = grub_strdup ("\e[m");
-      term.cursor_on         = grub_strdup ("\e[?25l");
-      term.cursor_off        = grub_strdup ("\e[?25h");
-      return grub_errno;
+/* Unescape a string.  */
+static char *
+unescape_string (const char *in)
+{
+  char *q, *new_string;
+
+  new_string = (char *) grub_malloc (grub_strlen (in) + 1);	/* New string will be <= current string.  */
+
+  for (q = new_string; *in; in++, q++)
+    {
+      switch (*in)
+        {
+          case '\\':	/* Escape sequence.  */
+            in++;
+            if (*in >= '0' && *in <= '3')	/* Looks like the begining of an octal.  */
+              {
+                *q = *in - '0';
+                in++;
+                if (*in >= '0' && *in <= '7')	/* Middle of an octal.  */
+                  {
+                   *q = (*q << 3) | (*in - '0');
+                   in++;
+                   if (*in >= '0' && *in <= '7')	/* End of an octal.  */
+                     {
+                       *q = (*q << 3) | (*in - '0'); 
+                       break;
+                     }
+                   in--;	/* Was not an octal; rewind.  */
+                  }
+                in -= 2;	/* Was not an octal; rewind.  */
+                q--;	/* Nothing to put here; rewind.  */
+		break;
+              }
+            switch (*in)	/* Does not look like an octal.  */
+              {
+                case 'a':	/* Alert.  */
+                 *q = '\a';
+                 break;
+
+                case 'b':	/* Backspace.  */
+                  *q = '\b';
+                  break;
+
+                case 'e':	/* Escape.  */
+                case 'E':
+                  *q = '\e';
+                  break;
+
+                case 'f':	/* Form Feed.  */
+                  *q = '\f';
+                  break;
+
+                case 'n':	/* Newline.  */
+                  *q = '\n';
+                  break;
+
+                case 'r':	/* Carriage Return.  */
+                  *q = '\r';
+                  break;
+
+                case 't':	/* Tab.  */
+                  *q = '\t';
+                  break;
+
+                case 'v':	/* Vertical Tab.  */
+                  *q = '\v';
+                  break;
+
+                case '\\':	/* Backslash.  */
+                  *q = '\\';
+                  break;
+
+                default:	/* Anything else.  */
+                  *q = *in;
+              }
+	    break;
+
+          default:	/* Does not need any interpretation.  */
+            *q = *in;
+        }
     }
-  
-  return grub_error (GRUB_ERR_BAD_ARGUMENT, "unknown terminfo type.");
+  *q = 0;
+  return grub_realloc (new_string, grub_strlen (new_string) + 1);	/* Shrink alloc.  */
 }
 
 /* Wrapper for grub_putchar to write strings.  */
@@ -119,70 +285,273 @@
 void
 grub_terminfo_gotoxy (grub_uint8_t x, grub_uint8_t y)
 {
-  putstr (grub_terminfo_tparm (term.gotoxy, y, x));
+  putstr (grub_terminfo_tparm (cur_terminfo->gotoxy, y, x));
 }
 
 /* Clear the screen.  */
 void
 grub_terminfo_cls (void)
 {
-  putstr (grub_terminfo_tparm (term.cls));
+  putstr (grub_terminfo_tparm (cur_terminfo->cls));
 }
 
 /* Set reverse video mode on.  */
 void
 grub_terminfo_reverse_video_on (void)
 {
-  putstr (grub_terminfo_tparm (term.reverse_video_on));
+  putstr (grub_terminfo_tparm (cur_terminfo->reverse_video_on));
 }
 
 /* Set reverse video mode off.  */
 void
 grub_terminfo_reverse_video_off (void)
 {
-  putstr (grub_terminfo_tparm (term.reverse_video_off));
+  putstr (grub_terminfo_tparm (cur_terminfo->reverse_video_off));
 }
 
 /* Show cursor.  */
 void
 grub_terminfo_cursor_on (void)
 {
-  putstr (grub_terminfo_tparm (term.cursor_on));
+  putstr (grub_terminfo_tparm (cur_terminfo->cursor_on));
 }
 
 /* Hide cursor.  */
 void
 grub_terminfo_cursor_off (void)
 {
-  putstr (grub_terminfo_tparm (term.cursor_off));
+  putstr (grub_terminfo_tparm (cur_terminfo->cursor_off));
 }
 
 /* GRUB Command.  */
 
 static grub_err_t
-grub_cmd_terminfo (struct grub_arg_list *state __attribute__ ((unused)),
-		int argc, char **args)
+grub_cmd_terminfo (struct grub_arg_list *state, int argc, char **args)
 {
+  int i;
+  char *p;
+  terminfo_t *ti = 0;
+
+  auto int print_terminfo (terminfo_t *);
+  auto int find_terminfo (terminfo_t *);
+
+  int print_terminfo (terminfo_t *t)
+    {
+      grub_printf (" %s", t->name);
+      return 0;
+    }
+
+  int find_terminfo (terminfo_t *t)
+    {
+      if (grub_strcasecmp (t->name, args[0]) == 0)
+        {
+          ti = t;
+          return 1;
+        }
+
+      return 0;
+    }
+
+  i = state[0].set + state[1].set + state[2].set + state[3].set;
+
+  if (i > 1)
+    return grub_error (GRUB_ERR_INVALID_COMMAND, "add, modify, delete and view are exclusive operations.");
+
   if (argc == 0)
-  {
-    grub_printf ("Current terminfo type: %s\n", grub_terminfo_get_current());
-    return GRUB_ERR_NONE;
-  }
-  else if (argc != 1)
+    {
+      if (i == 0)
+        {
+          grub_printf ("Available terminfo definition(s):");
+            iterate_definitions (print_terminfo);
+          grub_putchar ('\n');
+          grub_printf ("Current terminfo definition: %s\n", cur_terminfo->name);
+	  
+	  return GRUB_ERR_NONE;
+        }
+      else
+        return grub_error (GRUB_ERR_BAD_ARGUMENT, "terminfo definition name must be specified.");
+    }
+
+  if (argc > 1)
     return grub_error (GRUB_ERR_BAD_ARGUMENT, "too many parameters.");
+
+  iterate_definitions (find_terminfo);
+  if ((! ti) && (! state[0].set))
+    return grub_error (GRUB_ERR_BAD_ARGUMENT, "no such terminfo definition.");
+
+  if (ti && state[0].set)
+    return grub_error (GRUB_ERR_BAD_ARGUMENT, "terminfo definition name already exists");
+
+  if (i == 0)
+    cur_terminfo = ti;
   else
-    return grub_terminfo_set_current (args[0]);
+    {
+      if (state[0].set)	/* Add new definition.  */
+        {
+          if (! state[4].set)
+            return grub_error (GRUB_ERR_INVALID_COMMAND, "Missing mandatory option for `cursor-address'.");
+
+          p = unescape_string (state[4].arg);	/* Cursor address.  */
+          if (! *p)
+            {
+              grub_free (p);
+              return grub_error (GRUB_ERR_INVALID_COMMAND, "option `cursor-address' must not be blank.");
+            }
+
+          ti = (terminfo_t *) grub_malloc (sizeof (terminfo_t));
+	  if (! ti)
+            return grub_error (GRUB_ERR_OUT_OF_MEMORY, "cannot allocate memory for new definition."); 
+          grub_memset (ti, 0, sizeof (terminfo_t));
+
+          ti->name   = grub_strdup (args[0]);
+
+          ti->gotoxy = p;
+
+          if (state[5].set)	/* Clear screen.  */
+            ti->cls = unescape_string (state[5].arg);
+          else
+            ti->cls = grub_strdup ("");
+
+          if (state[6].set)	/* Reverse video on.  */
+            ti->reverse_video_on = unescape_string (state[6].arg);
+          else
+            ti->reverse_video_on = grub_strdup ("");
+
+          if (state[7].set)	/* Reverse video off.  */
+            ti->reverse_video_off = unescape_string (state[7].arg);
+          else
+            ti->reverse_video_off = grub_strdup ("");
+
+          if (state[8].set)	/* Cursor on.  */
+            ti->cursor_on = unescape_string (state[8].arg);
+          else
+            ti->cursor_on = grub_strdup ("");
+
+          if (state[9].set)	/* Cursor off.  */
+            ti->cursor_off = unescape_string (state[9].arg);
+          else
+            ti->cursor_off = grub_strdup ("");
+
+          add_definition (ti);
+        }
+      else if (state[1].set)	/* Modify definition.  */
+        {
+          if (state[4].set)
+            {
+              p = unescape_string (state[4].arg);
+              if (! *p)
+                {
+                  grub_free (p);
+                  return grub_error (GRUB_ERR_INVALID_COMMAND, "option `cursor-address' must not be blank.");
+                }
+
+              grub_free (ti->gotoxy);
+              ti->gotoxy = p;
+            }
+
+          if (state[5].set)
+            {
+              grub_free (ti->cls);
+              ti->cls = unescape_string (state[5].arg);
+            }
+
+          if (state[6].set)
+            {
+              grub_free (ti->reverse_video_on);
+              ti->reverse_video_on = unescape_string (state[6].arg);
+            }
+
+          if (state[7].set)
+            {
+              grub_free (ti->reverse_video_off);
+              ti->reverse_video_off = unescape_string (state[7].arg);
+            }
+
+          if (state[8].set)
+            {
+              grub_free (ti->cursor_on);
+              ti->cursor_on = unescape_string (state[8].arg);
+            }
+
+          if (state[9].set)
+            {
+              grub_free (ti->cursor_off);
+              ti->cursor_off = unescape_string (state[9].arg);
+            }
+        }
+      else if (state[2].set)	/* Delete definition.  */
+        {
+          if (cur_terminfo == ti)
+            return grub_error (GRUB_ERR_BAD_ARGUMENT, "cannot delete in use definition.");
+
+          delete_definition (ti);
+        }
+      else if (state[3].set)	/* View definition.  */
+        {
+          grub_printf ("Terminfo definition: %s\n", ti->name);
+
+          p = escape_string (ti->gotoxy);
+          grub_printf ("Cursor address: %s\n", p);
+          grub_free (p);
+
+          p = escape_string (ti->cls);
+          grub_printf ("Clear screen: %s\n", p);
+          grub_free (p);
+
+          p = escape_string (ti->reverse_video_on);
+          grub_printf ("Reverse video on: %s\n", p);
+          grub_free (p);
+
+          p = escape_string (ti->reverse_video_off);
+          grub_printf ("Reverse video off: %s\n", p);
+          grub_free (p);
+
+          p = escape_string (ti->cursor_on);
+          grub_printf ("Cursor on: %s\n", p);
+          grub_free (p);
+
+          p = escape_string (ti->cursor_off);
+          grub_printf ("Cursor off: %s\n", p);
+          grub_free (p);
+        }
+    }
+
+  return GRUB_ERR_NONE;
+}
+
+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? */
+
+  /* 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;
+
+  add_definition (ti);
+  cur_terminfo = ti;
 }
 
 GRUB_MOD_INIT(terminfo)
 {
   (void) mod;			/* To stop warning. */
+  setup_defaults ();
   grub_register_command ("terminfo", grub_cmd_terminfo, GRUB_COMMAND_FLAG_BOTH,
-			 "terminfo [TERM]", "Set terminfo type.", 0);
-  grub_terminfo_set_current ("vt100");
+			 "terminfo [TERMTYPE [-a|-m|-d|-v [ARGS...]]]", "Select or define terminfo definition(s). Escape sequences must be double quoted.", options);
 }
 
 GRUB_MOD_FINI(terminfo)
 {
   grub_unregister_command ("terminfo");
+  while (terminfo_list)
+    delete_definition (terminfo_list);
 }

[-- Attachment #3: enable-serial-module.diff --]
[-- Type: text/plain, Size: 548 bytes --]

diff -uNr grub2.cvs/conf/i386-pc.rmk grub2/conf/i386-pc.rmk
--- grub2.cvs/conf/i386-pc.rmk	2005-12-25 08:59:50.000000000 -0700
+++ grub2/conf/i386-pc.rmk	2005-12-27 16:56:54.000000000 -0700
@@ -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

[-- Attachment #4: strcasecmp.diff --]
[-- Type: text/plain, Size: 1291 bytes --]

diff -uNr grub2.cvs/include/grub/misc.h grub2/include/grub/misc.h
--- grub2.cvs/include/grub/misc.h	2005-10-24 04:23:46.000000000 -0600
+++ grub2/include/grub/misc.h	2005-12-27 16:57:45.000000000 -0700
@@ -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);
diff -uNr grub2.cvs/kern/misc.c grub2/kern/misc.c
--- grub2.cvs/kern/misc.c	2005-10-27 21:14:33.000000000 -0600
+++ grub2/kern/misc.c	2005-12-27 16:57:45.000000000 -0700
@@ -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;

Re: [PATCH] User definable terminfo support

[Marco Gerards]

Omniflux <omniflux+lists@omniflux.com> writes:

Hi,

Thanks a lot for your patch!

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

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

> 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

Re: [PATCH] User definable terminfo support

[Marco Gerards]

Omniflux <omniflux+lists@omniflux.com> writes:

Hi,

Here is the review of the code that I promised.  Please see my other
email about generic comments.

> +/* 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;

> +/* Unescape a string.  */
> +static char *
> +unescape_string (const char *in)
> +{
> +  char *q, *new_string;

Same here.

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


> +  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");

> +          ti->name   = grub_strdup (args[0]);

Typo.

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

> +  /* 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

Re: [PATCH] User definable terminfo support

[Omniflux]

Marco Gerards wrote:
>>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...

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?

> 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?

> 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!

-- 
Omniflux

Re: [PATCH] User definable terminfo support

[Omniflux]

Marco Gerards wrote:
> 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 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?

>>+          ti->name   = grub_strdup (args[0]);
> 
> 
> Typo.

Can you be more specific? This looks correct to me...

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

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.

-- 
Omniflux

Re: [PATCH] User definable terminfo support

[Marco Gerards]

Omniflux <omniflux+lists@omniflux.com> writes:

> Marco Gerards wrote:
>>>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...
>
> 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.

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

> 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. ;-)

>
> Did I interpret your comments correctly?

Partially. ;)

I'm sorry for my terrible English.

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

Re: [PATCH] User definable terminfo support

[Marco Gerards]

Omniflux <omniflux+lists@omniflux.com> writes:

> Marco Gerards wrote:
>> 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. ;-)

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

>>>+          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 `=')

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

Re: [PATCH] User definable terminfo support

[Omniflux]

Marco Gerards wrote:
> 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.

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.

> 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!

-- 
Omniflux
-- 
Omniflux

Re: [PATCH] User definable terminfo support

[Marco Gerards]

Omniflux <omniflux+lists@omniflux.com> writes:

> Marco Gerards wrote:
>> 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.
>
> 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.

> 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