Messages in this thread | | | Subject | Re: [PATCH v3] console: Add persistent scrollback buffers for all VGA consoles | From | Manuel Schölling <> | Date | Wed, 16 Nov 2016 18:40:19 +0100 |
| |
Hi,
I finally found the time to rework the patches, but there are a few things I want to discuss:
> > This breaks tools like clear_console that rely on flushing the > > scrollback history by switching back and forth between consoles > > which is why this feature is disabled by default. > > Use the escape sequence \e[3J instead for flushing the buffer. > > Have never heard of such tool as clear_console. Why mention it at all, > where is it used, which are practical configurations which are broken in > this way? clear_console(1) is part of Debian's bash package and (I think) a inofficial 'downstream' patch developed by Ubuntu [1]. There has been security concerns (which is also why I disabled this feature by default). However, if there might be security issues, I would strongly suggest to keep this part in the documentation - it does no harm anyway, does it?
> Does this break clear(1)? No, my tests with clear(1) worked fine.
> > - Enter the amount of System RAM to allocate for the scrollback > > - buffer. Each 64KB will give you approximately 16 80x25 > > - screenfuls of scrollback buffer > > + Enter the amount of System RAM to allocate for scrollback > > + buffers of VGA consoles. Each 64KB will give you approximately > > + 16 80x25 screenfuls of scrollback buffer. > > Indentation amended, I don't know which way it should be, please > double-check or mimic old indentation to stay on safe side. Double-checked! That's at least how I read the docs.
> > +static struct vgacon_scrollback_info *vgacon_scrollback_cur; > > Could you use name vgacon_scrollback instead of new > vgacon_scrollback_cur? It doesn't change anything conceptually, but is a > bit shorter and seems to mean current scrollback context anyway, right? > > I think I see nothing to comment on below. There is a already an array called 'vgacon_scrollbacks[N]', so I would not like to have two variables named 'vgacon_scrollback' and 'vgacon_scrollbacks'. Either {'vgacon_scrollback_cur' and 'vgacon_scrollbacks'} or {'vgacon_scrollback' and 'vgacon_scrollback_list'}. You can make a suggestion, if you like.
All other point you mentioned where fixed as you suggested.
Thanks again for your help, Andrey!
Best,
Manuel
[1] http://manpages.ubuntu.com/manpages/xenial/en/man1/clear_console.1.html
| |