Re: Bug#586358: grub-pc: Please handle DOS line endings in "cat"

Re: Bug#586358: grub-pc: Please handle DOS line endings in "cat"

[Colin Watson]

On Fri, Jun 18, 2010 at 11:34:08AM -0700, Josh Triplett wrote:
> The "cat" command, if given a file with DOS line endings, will show the
> \r at the end of every line as a control character.  For convenience,
> please consider adding support for this in "cat", ideally automatically.

Makes sense to me.  grub-devel, how about this patch?

2010-06-20  Colin Watson  <cjwatson@ubuntu.com>

	* commands/cat.c (grub_cmd_cat): Print DOS "\r\n" line endings as
	simple newlines.

=== modified file 'commands/cat.c'
--- commands/cat.c	2010-03-01 19:24:34 +0000
+++ commands/cat.c	2010-06-20 22:11:37 +0000
@@ -54,6 +54,11 @@ grub_cmd_cat (grub_command_t cmd __attri
 
 	  if ((grub_isprint (c) || grub_isspace (c)) && c != '\r')
 	    grub_putchar (c);
+	  else if (c == '\r' && buf[i + 1] == '\n')
+	    {
+	      grub_putchar ('\n');
+	      i++;
+	    }
 	  else
 	    {
 	      grub_setcolorstate (GRUB_TERM_COLOR_HIGHLIGHT);

Thanks,

-- 
Colin Watson                                       [cjwatson@ubuntu.com]

Re: Bug#586358: grub-pc: Please handle DOS line endings in "cat"

[Josh Triplett]

On Sun, Jun 20, 2010 at 11:19:37PM +0100, Colin Watson wrote:
> On Fri, Jun 18, 2010 at 11:34:08AM -0700, Josh Triplett wrote:
> > The "cat" command, if given a file with DOS line endings, will show the
> > \r at the end of every line as a control character.  For convenience,
> > please consider adding support for this in "cat", ideally automatically.
> 
> Makes sense to me.  grub-devel, how about this patch?
> 
> 2010-06-20  Colin Watson  <cjwatson@ubuntu.com>
> 
> 	* commands/cat.c (grub_cmd_cat): Print DOS "\r\n" line endings as
> 	simple newlines.
> 
[...]

Thanks!

- Josh Triplett

Re: Bug#586358: grub-pc: Please handle DOS line endings in "cat"

[Vladimir 'φ-coder/phcoder' Serbinenko]

[-- Attachment #1: Type: text/plain, Size: 1483 bytes --]

>
> On Fri, Jun 18, 2010 at 11:34:08AM -0700, Josh Triplett wrote:
>   
>> The "cat" command, if given a file with DOS line endings, will show the
>> \r at the end of every line as a control character.  For convenience,
>> please consider adding support for this in "cat", ideally automatically.
>>     
> Makes sense to me.  grub-devel, how about this patch?
>
>   
Unconditionaly stripping \r is a bad idea because sometimes cat in grub
is used to determine why the system fails to boot. And if by a stupid
action fstab becomes DOS-formated 'cat /etc/fstab' will fail to show
this important point. But it's ok to add an option for cat'ing DOS
files. In this case we probably need to mark unix newlines somehow to
detect a reverse problem
> 2010-06-20  Colin Watson  <cjwatson@ubuntu.com>
>
> 	* commands/cat.c (grub_cmd_cat): Print DOS "\r\n" line endings as
> 	simple newlines.
>
> === modified file 'commands/cat.c'
> --- commands/cat.c	2010-03-01 19:24:34 +0000
> +++ commands/cat.c	2010-06-20 22:11:37 +0000
> @@ -54,6 +54,11 @@ grub_cmd_cat (grub_command_t cmd __attri
>  
>  	  if ((grub_isprint (c) || grub_isspace (c)) && c != '\r')
>  	    grub_putchar (c);
> +	  else if (c == '\r' && buf[i + 1] == '\n')
> +	    {
> +	      grub_putchar ('\n');
> +	      i++;
> +	    }
>  	  else
>  	    {
>  	      grub_setcolorstate (GRUB_TERM_COLOR_HIGHLIGHT);
>
> Thanks,
>
>   


-- 
Regards
Vladimir 'φ-coder/phcoder' Serbinenko



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 294 bytes --]

Re: Bug#586358: grub-pc: Please handle DOS line endings in "cat"

[Colin Watson]

On Fri, Jun 25, 2010 at 08:51:12PM +0200, Vladimir 'φ-coder/phcoder' Serbinenko wrote:
> Colin Watson wrote:
> > On Fri, Jun 18, 2010 at 11:34:08AM -0700, Josh Triplett wrote:
> >> The "cat" command, if given a file with DOS line endings, will show the
> >> \r at the end of every line as a control character.  For convenience,
> >> please consider adding support for this in "cat", ideally automatically.
> >
> > Makes sense to me.  grub-devel, how about this patch?
> 
> Unconditionaly stripping \r is a bad idea because sometimes cat in grub
> is used to determine why the system fails to boot. And if by a stupid
> action fstab becomes DOS-formated 'cat /etc/fstab' will fail to show
> this important point. But it's ok to add an option for cat'ing DOS
> files.

OK.  How about the following patch, then?

> In this case we probably need to mark unix newlines somehow to
> detect a reverse problem

I think we can live without this.  It adds complexity (I can't
immediately think of a neat way to represent this visually) and I think
it would be needed quite rarely.  Besides, I was looking around for
examples and noticed that when vim is in DOS mode it simply treats both
\r\n and \n as newlines.  That seems a good enough precedent for me.

2010-06-20  Colin Watson  <cjwatson@ubuntu.com>

	* commands/cat.c (options): New variable.
	(grub_cmd_cat): Parse options.  If the --dos option is given, print
	DOS-style "\r\n" line endings as simple newlines.
	(GRUB_MOD_INIT): Use extcmd.
	(GRUB_MOD_FINI): Likewise.
	* docs/grub.texi (cat): Document --dos.

=== modified file 'commands/cat.c'
--- commands/cat.c	2010-03-01 19:24:34 +0000
+++ commands/cat.c	2010-06-27 23:56:11 +0000
@@ -23,19 +23,28 @@
 #include <grub/term.h>
 #include <grub/misc.h>
 #include <grub/gzio.h>
-#include <grub/command.h>
+#include <grub/extcmd.h>
 #include <grub/i18n.h>
 
-static grub_err_t
-grub_cmd_cat (grub_command_t cmd __attribute__ ((unused)),
-	      int argc, char **args)
+static const struct grub_arg_option options[] =
+  {
+    {"dos", -1, 0, N_("Accept DOS-style CR/NL line endings."), 0, 0},
+    {0, 0, 0, 0, 0, 0}
+  };
 
+static grub_err_t
+grub_cmd_cat (grub_extcmd_t cmd, int argc, char **args)
 {
+  struct grub_arg_list *state = cmd->state;
+  int dos = 0;
   grub_file_t file;
   char buf[GRUB_DISK_SECTOR_SIZE];
   grub_ssize_t size;
   int key = 0;
 
+  if (state[0].set)
+    dos = 1;
+
   if (argc != 1)
     return grub_error (GRUB_ERR_BAD_ARGUMENT, "file name required");
 
@@ -54,6 +63,11 @@ grub_cmd_cat (grub_command_t cmd __attri
 
 	  if ((grub_isprint (c) || grub_isspace (c)) && c != '\r')
 	    grub_putchar (c);
+	  else if (dos && c == '\r' && buf[i + 1] == '\n')
+	    {
+	      grub_putchar ('\n');
+	      i++;
+	    }
 	  else
 	    {
 	      grub_setcolorstate (GRUB_TERM_COLOR_HIGHLIGHT);
@@ -74,15 +88,16 @@ grub_cmd_cat (grub_command_t cmd __attri
   return 0;
 }
 
-static grub_command_t cmd;
+static grub_extcmd_t cmd;
 \f
 GRUB_MOD_INIT(cat)
 {
-  cmd = grub_register_command_p1 ("cat", grub_cmd_cat,
-				  N_("FILE"), N_("Show the contents of a file."));
+  cmd = grub_register_extcmd ("cat", grub_cmd_cat, GRUB_COMMAND_FLAG_BOTH,
+			      N_("FILE"), N_("Show the contents of a file."),
+			      options);
 }
 
 GRUB_MOD_FINI(cat)
 {
-  grub_unregister_command (cmd);
+  grub_unregister_extcmd (cmd);
 }

=== modified file 'docs/grub.texi'
--- docs/grub.texi	2010-06-26 09:50:39 +0000
+++ docs/grub.texi	2010-06-27 23:45:11 +0000
@@ -1624,13 +1624,19 @@ a menu entry).
 @node cat
 @subsection cat
 
-@deffn Command cat file
+@deffn Command cat [@option{--dos}] file
 Display the contents of the file @var{file}. This command may be useful
 to remind you of your OS's root partition:
 
 @example
 grub> @kbd{cat /etc/fstab}
 @end example
+
+If the @option{--dos} option is used, then carriage return / new line pairs
+will be displayed as a simple new line.  Otherwise, the carriage return will
+be displayed as a control character (@samp{<d>}) to make it easier to see
+when boot problems are caused by a file formatted using DOS-style line
+endings.
 @end deffn
 
 

Thanks,

-- 
Colin Watson                                       [cjwatson@ubuntu.com]

Re: Bug#586358: grub-pc: Please handle DOS line endings in "cat"

[Colin Watson]

On Mon, Jun 28, 2010 at 01:02:06AM +0100, Colin Watson wrote:
> On Fri, Jun 25, 2010 at 08:51:12PM +0200, Vladimir 'φ-coder/phcoder' Serbinenko wrote:
> > Colin Watson wrote:
> > > On Fri, Jun 18, 2010 at 11:34:08AM -0700, Josh Triplett wrote:
> > >> The "cat" command, if given a file with DOS line endings, will show the
> > >> \r at the end of every line as a control character.  For convenience,
> > >> please consider adding support for this in "cat", ideally automatically.
> > >
> > > Makes sense to me.  grub-devel, how about this patch?
> > 
> > Unconditionaly stripping \r is a bad idea because sometimes cat in grub
> > is used to determine why the system fails to boot. And if by a stupid
> > action fstab becomes DOS-formated 'cat /etc/fstab' will fail to show
> > this important point. But it's ok to add an option for cat'ing DOS
> > files.
> 
> OK.  How about the following patch, then?

Vladimir approved this on IRC.  Committed.

-- 
Colin Watson                                       [cjwatson@ubuntu.com]