Re: [PATCH] User definable terminfo support

[Marco Gerards <mgerards@xs4all.nl>]

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