Re: [PATCH] Clear out gfxterm's virtual text_buffer - fixes junk at end of lines

[Joe Auricchio]

On Sun, Aug 23, 2009 at 06:07, Robert Millan<address@hidden> wrote:
> On Wed, Jul 22, 2009 at 10:42:54AM -0700, Joe Auricchio wrote:
>>>
>>> So perhaps this can be solved simpler by replacing grub_malloc with
>>> grub_zalloc ?
>>
>> I saw the zalloc commit and thought the same thing! :D I'll test it. But
>> fg_color and bg_color must be set to the screen's current colors, which
>> may not be zero.
>>
>> I just discovered that
>> - this patch to grub_virtual_screen_setup()
>> - scroll_up() line 579 /* Clear last line in text buffer. */, and
>> - grub_virtual_screen_cls()
>> do the same thing.
>>
>> An opportunity to refactor? I'll look into this.
>
> Hi,
>
> Note that the initial patch was committed due to lack of coordination (there's
> been a period in which we've been sloppy due to lack of leadership, but this
> won't happen again).
>
> Nevertheless, the initial approach is probably not correct;  my impression
> is that it should be using grub_zalloc() instead.  If you're still interested,
> I'd appreciate if you could fix that.

Using zalloc only makes sense in grub_virtual_screen_setup, when the
buffer is allocated. But the clear_chars loop is used thrice: in
_setup, in grub_virtual_screen_cls, and in scroll_up.

Freeing and allocating a new buffer for each cls or scroll is clearly
insane. So the zalloc can't be in the common function.

The goal of this refactor was to centralize the code for visually
clearing the text buffer. So it doesn't make sense to have _setup use
zalloc but have _cls and scroll_up use a loop without zalloc: then
what has been refactored?

It seemed like a nice idea to use zalloc, but this code isn't the
right nail for that hammer. :)

Let me know how you want to proceed. If you'd still like to use zalloc
I can adjust the patch.