[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] Clear out gfxterm's virtual text_buffer - fixes junk at end
From: |
Robert Millan |
Subject: |
Re: [PATCH] Clear out gfxterm's virtual text_buffer - fixes junk at end of lines |
Date: |
Tue, 25 Aug 2009 21:54:05 +0200 |
User-agent: |
Mutt/1.5.18 (2008-05-17) |
On Mon, Aug 24, 2009 at 05:45:15PM -0700, Joe Auricchio wrote:
> 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.
No. When I suggested zalloc I hadn't looked at this in detail (and I still
haven't). I was under the impression that you considered zalloc was suitable.
If that's not the case, then there's nothing more to say. Thanks for your
explanation!
--
Robert Millan
The DRM opt-in fallacy: "Your data belongs to us. We will decide when (and
how) you may access your data; but nobody's threatening your freedom: we
still allow you to remove your data and not access it at all."