grub-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCHv2] FIXME: These should be dynamically obtained from a termina


From: Marco Gerards
Subject: Re: [PATCHv2] FIXME: These should be dynamically obtained from a terminal.
Date: Thu, 04 Aug 2005 14:29:10 +0200
User-agent: Gnus/5.1007 (Gnus v5.10.7) Emacs/21.3 (gnu/linux)

Vincent Pelletier <address@hidden> writes:

Hi Vincent,

Finally I had the time and energy to both proofread and test your
patch.  It did work for me after making some changes.

> First patch :
>
> 2005-07-13  Vincent Pelletier  <address@hidden>
>
>       * include/grub/term.h (GRUB_TERM_WIDTH, GRUB_TERM_HEIGHT):
>       Redefined to use grub_getwh.
>       (struct grub_term): New field named getwh.
>       (grub_getwh): New exported prototype.
>       * kern/term.c (grub_getwh): New function.
>       * term/i386/pc/console.c (grub_console_getwh): New function.
>       (grub_console_term): New field named getwh and new initial
>       value.
>       * term/i386/pc/vga.c (grub_vga_getwh): New function.
>       (grub_vga_term): New field named getwh and new initial value.
>
> Second patch :
>
> 2005-07-13  Vincent Pelletier  <address@hidden>
>       * term/sparc64/ofconsole.c (grub_ofconsole_readkey): Use
>       grub_ssize_t.

You forgot a newline.  The sparc64 directory does not exist.  Hollis
changes this to term/ieee1275, can you make that change?

>       (grub_ofconsole_cls): Don't clear screen when debug environment
>       variable is set, regardless of its value.

Please do not check in this change.  If we have to make such change, I
think it should be in grub_cls ().

>       (grub_ofconsole_init): Use grub_ssize_t and unsigned char.
>       (grub_ofconsole_term): New field named getwh and new initial
>       value.

This should be a single changelog entry.  As I see it, it is one
patch.


> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.1 (GNU/Linux)
>
> iD8DBQFC1YfMFEQoKRQyjtURAtliAJ9iZKLpn48u8arpEXsgLlm5GBSFPwCcCOMX
> ECZ8+QD/tK5cmdvciVRrnN8=
> =EbEg
> -----END PGP SIGNATURE-----
> Index: include/grub/term.h
> ===================================================================
> RCS file: /cvsroot/grub/grub2/include/grub/term.h,v
> retrieving revision 1.7
> diff -u -p -r1.7 term.h
> --- include/grub/term.h       19 Feb 2005 20:56:07 -0000      1.7
> +++ include/grub/term.h       13 Jul 2005 21:19:36 -0000
> @@ -71,9 +71,9 @@ grub_term_color_state;
>  
>  /* Menu-related geometrical constants.  */
>  
> -/* FIXME: These should be dynamically obtained from a terminal.  */
> -#define GRUB_TERM_WIDTH              80
> -#define GRUB_TERM_HEIGHT     25
> +/* FIXME: Ugly way to get them form terminal.  */
> +#define GRUB_TERM_WIDTH         ((grub_getwh()&0xFF00)>>8)
> +#define GRUB_TERM_HEIGHT        (grub_getwh()&0xFF)

Please use the GCS, so a space before the () and spaces surrounding
the operators.

> diff -rup powerpc/ieee1275/ofconsole.c sparc64/ieee1275/ofconsole.c
> --- powerpc/ieee1275/ofconsole.c      2005-06-21 04:33:52.000000000 +0200

[...]

> +static grub_uint16_t
> +grub_ofconsole_getwh (void)
> +{
> +  grub_ieee1275_ihandle_t config;
> +  char *val;
> +  grub_ssize_t lval;
> +  static grub_uint8_t w, h;
> +
> +  if (w && h) /* Once we have them, don't ask them again.  */

The check is wrong, it should be:
if (! (w && h))

> +    {
> +      if (! grub_ieee1275_open ("/config", &config) &&

This did not work for me, I changed this to:

if (! grub_ieee1275_finddevice ("/options", &config) &&


Does that change work for you?  If it does not we have to figure out
how to make it work on both the sparc and on the PPC.

> +          config != -1)
> +        {
> +          if (! grub_ieee1275_get_property_length (config, "screen-#columns",
> +                                                   &lval) && lval != -1)
> +            {
> +              if ((val = grub_malloc (lval)) != 0)


I do not like this.  Just use:

val = grub_malloc (...);
if (! val)
 ...

> +  /* XXX: Default values from OpenBoot on my U10.  */
> +  if (! w)
> +    w = 80;
> +  if (! h)
> +    h = 34;

Can you change that to 80x24, the defaults on the pegasos?  It would
be better to use values that are too small instead of too big.

Thanks,
Marco





reply via email to

[Prev in Thread] Current Thread [Next in Thread]