grub-devel
[Top][All Lists]
Advanced

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




reply via email to

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