PATCH [0/2] ls core command: cleanup and fix

PATCH [0/2] ls core command: cleanup and fix

[Francesco Lavra]

This 2-patch series contains changes to grub-core/kern/corecmd.c
(grub_core_cmd_ls): the first patch removes unreachable code, while the
second one handles correctly listing of the root directory of a device
without requiring duplicated handling from each filesystem driver.

--
Francesco

[PATCH 1/2] ls core command: remove unreachable code

[Francesco Lavra]

The path local variable in grub_core_cmd_ls() is assigned values such 
that it cannot be NULL, so a couple of if blocks can be removed as they 
are never executed.

2013-05-12  Francesco Lavra  <francescolavra.fl@gmail.com>

	* grub-core/kern/corecmd.c (grub_core_cmd_ls): Remove unreachable code.

=== modified file 'grub-core/kern/corecmd.c'
--- grub-core/kern/corecmd.c	2013-03-03 00:34:27 +0000
+++ grub-core/kern/corecmd.c	2013-05-12 09:36:16 +0000
@@ -139,21 +139,7 @@
       else
 	path++;
 
-      if (! path && ! device_name)
-	{
-	  grub_error (GRUB_ERR_BAD_ARGUMENT, "invalid argument");
-	  goto fail;
-	}
-
-      if (! path)
-	{
-	  if (grub_errno == GRUB_ERR_UNKNOWN_FS)
-	    grub_errno = GRUB_ERR_NONE;
-
-	  grub_printf ("(%s): Filesystem is %s.\n",
-		       device_name, fs ? fs->name : "unknown");
-	}
-      else if (fs)
+      if (fs)
 	{
 	  (fs->dir) (dev, path, grub_mini_print_files, NULL);
 	  grub_xputs ("\n");

[PATCH 2/2] ls core command: handle listing of the root directory

[Francesco Lavra]

Currently, listing of the root directory of a device with the command:
ls (device_name)
requires the underlying filesystem driver to handle an empty path 
string as if it was the root directory path "/". This introduces 
duplicated code across the different filesystem drivers. If a given 
filesystem driver does not implement special handling of the empty 
path string, the above command gives "error: invalid file name `'."
This error happens for instance with the ext4 filesystem.
The best place to handle correctly the empty path string and transform 
it in "/" is the function grub_core_cmd_ls(), so that handling from 
each filesystem driver is not required anymore.

2013-05-12  Francesco Lavra  <francescolavra.fl@gmail.com>

	* grub-core/kern/corecmd.c (grub_core_cmd_ls): Handle listing of the
	root directory.

=== modified file 'grub-core/kern/corecmd.c'
--- grub-core/kern/corecmd.c	2013-05-12 09:45:56 +0000
+++ grub-core/kern/corecmd.c	2013-05-12 09:55:53 +0000
@@ -137,7 +137,13 @@
       if (! path)
 	path = argv[0];
       else
-	path++;
+	{
+	  path++;
+	  if (*path == '\0')
+	    /* The argument is a device name: list all files at the root
+	       directory of the device. */
+	    path = (char *) "/";
+	}
 
       if (fs)
 	{

Re: [PATCH 2/2] ls core command: handle listing of the root directory

[Francesco Lavra]

On 05/12/2013 12:56 PM, Francesco Lavra wrote:
> Currently, listing of the root directory of a device with the command:
> ls (device_name)
> requires the underlying filesystem driver to handle an empty path 
> string as if it was the root directory path "/". This introduces 
> duplicated code across the different filesystem drivers. If a given 
> filesystem driver does not implement special handling of the empty 
> path string, the above command gives "error: invalid file name `'."
> This error happens for instance with the ext4 filesystem.
> The best place to handle correctly the empty path string and transform 
> it in "/" is the function grub_core_cmd_ls(), so that handling from 
> each filesystem driver is not required anymore.

After revision 5010, issuing the ls command with a device name as
parameter gives a response such as:
(device_name): Filesystem is <filesystem name>.
But grub.texi says:
"
@deffn Command ls [arg @dots{}]
List devices or files.

With no arguments, print all devices known to GRUB.

If the argument is a device name enclosed in parentheses (@pxref{Device
syntax}), then list all files at the root directory of that device.

If the argument is a directory given as an absolute file name (@pxref{File
name syntax}), then list the contents of that directory.
@end deffn
"

Which is the correct behavior?

Re: [PATCH 2/2] ls core command: handle listing of the root directory

[Francesco Lavra]

On 05/18/2013 12:26 PM, Francesco Lavra wrote:
> On 05/12/2013 12:56 PM, Francesco Lavra wrote:
>> Currently, listing of the root directory of a device with the command:
>> ls (device_name)
>> requires the underlying filesystem driver to handle an empty path 
>> string as if it was the root directory path "/". This introduces 
>> duplicated code across the different filesystem drivers. If a given 
>> filesystem driver does not implement special handling of the empty 
>> path string, the above command gives "error: invalid file name `'."
>> This error happens for instance with the ext4 filesystem.
>> The best place to handle correctly the empty path string and transform 
>> it in "/" is the function grub_core_cmd_ls(), so that handling from 
>> each filesystem driver is not required anymore.
> 
> After revision 5010, issuing the ls command with a device name as
> parameter gives a response such as:
> (device_name): Filesystem is <filesystem name>.
> But grub.texi says:
> "
> @deffn Command ls [arg @dots{}]
> List devices or files.
> 
> With no arguments, print all devices known to GRUB.
> 
> If the argument is a device name enclosed in parentheses (@pxref{Device
> syntax}), then list all files at the root directory of that device.
> 
> If the argument is a directory given as an absolute file name (@pxref{File
> name syntax}), then list the contents of that directory.
> @end deffn
> "
> 
> Which is the correct behavior?

Depending on the answer to the above question, you might want to 
consider applying one of the two patches below, as you see fit.

Regards,
Francesco

2013-06-02  Francesco Lavra  <francescolavra.fl@gmail.com>

	* docs/grub.texi (ls): Fix command description in case of a device name
	passed as argument.

=== modified file 'docs/grub.texi'
--- docs/grub.texi	2013-05-11 05:23:26 +0000
+++ docs/grub.texi	2013-06-02 14:35:24 +0000
@@ -4049,7 +4049,7 @@
 With no arguments, print all devices known to GRUB.
 
 If the argument is a device name enclosed in parentheses (@pxref{Device
-syntax}), then list all files at the root directory of that device.
+syntax}), then print the name of the filesystem of that device.
 
 If the argument is a directory given as an absolute file name (@pxref{File
 name syntax}), then list the contents of that directory.


2013-06-02  Francesco Lavra  <francescolavra.fl@gmail.com>

	* grub-core/kern/corecmd.c (grub_core_cmd_ls): Fix listing of root
	directory contents.

=== modified file 'grub-core/kern/corecmd.c'
--- grub-core/kern/corecmd.c	2013-06-02 14:23:14 +0000
+++ grub-core/kern/corecmd.c	2013-06-02 14:28:24 +0000
@@ -147,13 +147,11 @@
 
       if (! *path)
 	{
-	  if (grub_errno == GRUB_ERR_UNKNOWN_FS)
-	    grub_errno = GRUB_ERR_NONE;
-
-	  grub_printf ("(%s): Filesystem is %s.\n",
-		       device_name, fs ? fs->name : "unknown");
+	  /* The argument is a device name: list all files at the root directory
+	     of the device. */
+	  path = (char *) "/";
 	}
-      else if (fs)
+      if (fs)
 	{
 	  (fs->dir) (dev, path, grub_mini_print_files, NULL);
 	  grub_xputs ("\n");

Re: [PATCH 2/2] ls core command: handle listing of the root directory

[Andrey Borzenkov]

В Sun, 02 Jun 2013 16:48:23 +0200
Francesco Lavra <francescolavra.fl@gmail.com> пишет:

> On 05/18/2013 12:26 PM, Francesco Lavra wrote:
> > On 05/12/2013 12:56 PM, Francesco Lavra wrote:
> >> Currently, listing of the root directory of a device with the command:
> >> ls (device_name)
> >> requires the underlying filesystem driver to handle an empty path 
> >> string as if it was the root directory path "/". This introduces 
> >> duplicated code across the different filesystem drivers. If a given 
> >> filesystem driver does not implement special handling of the empty 
> >> path string, the above command gives "error: invalid file name `'."
> >> This error happens for instance with the ext4 filesystem.
> >> The best place to handle correctly the empty path string and transform 
> >> it in "/" is the function grub_core_cmd_ls(), so that handling from 
> >> each filesystem driver is not required anymore.
> > 
> > After revision 5010, issuing the ls command with a device name as
> > parameter gives a response such as:
> > (device_name): Filesystem is <filesystem name>.
> > But grub.texi says:
> > "
> > @deffn Command ls [arg @dots{}]
> > List devices or files.
> > 
> > With no arguments, print all devices known to GRUB.
> > 
> > If the argument is a device name enclosed in parentheses (@pxref{Device
> > syntax}), then list all files at the root directory of that device.
> > 
> > If the argument is a directory given as an absolute file name (@pxref{File
> > name syntax}), then list the contents of that directory.
> > @end deffn
> > "
> > 
> > Which is the correct behavior?
> 
> Depending on the answer to the above question, you might want to 
> consider applying one of the two patches below, as you see fit.
> 
> Regards,
> Francesco
> 
> 2013-06-02  Francesco Lavra  <francescolavra.fl@gmail.com>
> 
> 	* docs/grub.texi (ls): Fix command description in case of a device name
> 	passed as argument.
> 
> === modified file 'docs/grub.texi'
> --- docs/grub.texi	2013-05-11 05:23:26 +0000
> +++ docs/grub.texi	2013-06-02 14:35:24 +0000
> @@ -4049,7 +4049,7 @@
>  With no arguments, print all devices known to GRUB.
>  
>  If the argument is a device name enclosed in parentheses (@pxref{Device
> -syntax}), then list all files at the root directory of that device.
> +syntax}), then print the name of the filesystem of that device.
>  
>  If the argument is a directory given as an absolute file name (@pxref{File
>  name syntax}), then list the contents of that directory.
> 

Makes more sense to me than duplicating "ls (a,b)/". 


> 
> 2013-06-02  Francesco Lavra  <francescolavra.fl@gmail.com>
> 
> 	* grub-core/kern/corecmd.c (grub_core_cmd_ls): Fix listing of root
> 	directory contents.
> 
> === modified file 'grub-core/kern/corecmd.c'
> --- grub-core/kern/corecmd.c	2013-06-02 14:23:14 +0000
> +++ grub-core/kern/corecmd.c	2013-06-02 14:28:24 +0000
> @@ -147,13 +147,11 @@
>  
>        if (! *path)
>  	{
> -	  if (grub_errno == GRUB_ERR_UNKNOWN_FS)
> -	    grub_errno = GRUB_ERR_NONE;
> -
> -	  grub_printf ("(%s): Filesystem is %s.\n",
> -		       device_name, fs ? fs->name : "unknown");
> +	  /* The argument is a device name: list all files at the root directory
> +	     of the device. */
> +	  path = (char *) "/";
>  	}
> -      else if (fs)
> +      if (fs)
>  	{
>  	  (fs->dir) (dev, path, grub_mini_print_files, NULL);
>  	  grub_xputs ("\n");
> 
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH 2/2] ls core command: handle listing of the root directory

[Andrey Borzenkov]

What about this patch? Looks correct to me and reflects actual behavior.


В Sun, 02 Jun 2013 16:48:23 +0200
Francesco Lavra <francescolavra.fl@gmail.com> пишет:

> On 05/18/2013 12:26 PM, Francesco Lavra wrote:
> > On 05/12/2013 12:56 PM, Francesco Lavra wrote:
> >> Currently, listing of the root directory of a device with the command:
> >> ls (device_name)
> >> requires the underlying filesystem driver to handle an empty path 
> >> string as if it was the root directory path "/". This introduces 
> >> duplicated code across the different filesystem drivers. If a given 
> >> filesystem driver does not implement special handling of the empty 
> >> path string, the above command gives "error: invalid file name `'."
> >> This error happens for instance with the ext4 filesystem.
> >> The best place to handle correctly the empty path string and transform 
> >> it in "/" is the function grub_core_cmd_ls(), so that handling from 
> >> each filesystem driver is not required anymore.
> > 
> > After revision 5010, issuing the ls command with a device name as
> > parameter gives a response such as:
> > (device_name): Filesystem is <filesystem name>.
> > But grub.texi says:
> > "
> > @deffn Command ls [arg @dots{}]
> > List devices or files.
> > 
> > With no arguments, print all devices known to GRUB.
> > 
> > If the argument is a device name enclosed in parentheses (@pxref{Device
> > syntax}), then list all files at the root directory of that device.
> > 
> > If the argument is a directory given as an absolute file name (@pxref{File
> > name syntax}), then list the contents of that directory.
> > @end deffn
> > "
> > 
> > Which is the correct behavior?
> 
> Depending on the answer to the above question, you might want to 
> consider applying one of the two patches below, as you see fit.
> 
> Regards,
> Francesco
> 
> 2013-06-02  Francesco Lavra  <francescolavra.fl@gmail.com>
> 
> 	* docs/grub.texi (ls): Fix command description in case of a device name
> 	passed as argument.
> 
> === modified file 'docs/grub.texi'
> --- docs/grub.texi	2013-05-11 05:23:26 +0000
> +++ docs/grub.texi	2013-06-02 14:35:24 +0000
> @@ -4049,7 +4049,7 @@
>  With no arguments, print all devices known to GRUB.
>  
>  If the argument is a device name enclosed in parentheses (@pxref{Device
> -syntax}), then list all files at the root directory of that device.
> +syntax}), then print the name of the filesystem of that device.
>  
>  If the argument is a directory given as an absolute file name (@pxref{File
>  name syntax}), then list the contents of that directory.
> 
> 
> 2013-06-02  Francesco Lavra  <francescolavra.fl@gmail.com>
> 
> 	* grub-core/kern/corecmd.c (grub_core_cmd_ls): Fix listing of root
> 	directory contents.
> 
> === modified file 'grub-core/kern/corecmd.c'
> --- grub-core/kern/corecmd.c	2013-06-02 14:23:14 +0000
> +++ grub-core/kern/corecmd.c	2013-06-02 14:28:24 +0000
> @@ -147,13 +147,11 @@
>  
>        if (! *path)
>  	{
> -	  if (grub_errno == GRUB_ERR_UNKNOWN_FS)
> -	    grub_errno = GRUB_ERR_NONE;
> -
> -	  grub_printf ("(%s): Filesystem is %s.\n",
> -		       device_name, fs ? fs->name : "unknown");
> +	  /* The argument is a device name: list all files at the root directory
> +	     of the device. */
> +	  path = (char *) "/";
>  	}
> -      else if (fs)
> +      if (fs)
>  	{
>  	  (fs->dir) (dev, path, grub_mini_print_files, NULL);
>  	  grub_xputs ("\n");
> 
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH 2/2] ls core command: handle listing of the root directory

[Vladimir 'phcoder' Serbinenko]

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

this very confusing with 2 opposite patches in same mail. I don't have my
keys here. Feel free to commit documentation patch. If you commit on
Francesco's behalf don't forget --author
On Nov 27, 2013 7:42 PM, "Andrey Borzenkov" <arvidjaar@gmail.com> wrote:

> What about this patch? Looks correct to me and reflects actual behavior.
>
>
> В Sun, 02 Jun 2013 16:48:23 +0200
> Francesco Lavra <francescolavra.fl@gmail.com> пишет:
>
> > On 05/18/2013 12:26 PM, Francesco Lavra wrote:
> > > On 05/12/2013 12:56 PM, Francesco Lavra wrote:
> > >> Currently, listing of the root directory of a device with the command:
> > >> ls (device_name)
> > >> requires the underlying filesystem driver to handle an empty path
> > >> string as if it was the root directory path "/". This introduces
> > >> duplicated code across the different filesystem drivers. If a given
> > >> filesystem driver does not implement special handling of the empty
> > >> path string, the above command gives "error: invalid file name `'."
> > >> This error happens for instance with the ext4 filesystem.
> > >> The best place to handle correctly the empty path string and transform
> > >> it in "/" is the function grub_core_cmd_ls(), so that handling from
> > >> each filesystem driver is not required anymore.
> > >
> > > After revision 5010, issuing the ls command with a device name as
> > > parameter gives a response such as:
> > > (device_name): Filesystem is <filesystem name>.
> > > But grub.texi says:
> > > "
> > > @deffn Command ls [arg @dots{}]
> > > List devices or files.
> > >
> > > With no arguments, print all devices known to GRUB.
> > >
> > > If the argument is a device name enclosed in parentheses (@pxref{Device
> > > syntax}), then list all files at the root directory of that device.
> > >
> > > If the argument is a directory given as an absolute file name
> (@pxref{File
> > > name syntax}), then list the contents of that directory.
> > > @end deffn
> > > "
> > >
> > > Which is the correct behavior?
> >
> > Depending on the answer to the above question, you might want to
> > consider applying one of the two patches below, as you see fit.
> >
> > Regards,
> > Francesco
> >
> > 2013-06-02  Francesco Lavra  <francescolavra.fl@gmail.com>
> >
> >       * docs/grub.texi (ls): Fix command description in case of a device
> name
> >       passed as argument.
> >
> > === modified file 'docs/grub.texi'
> > --- docs/grub.texi    2013-05-11 05:23:26 +0000
> > +++ docs/grub.texi    2013-06-02 14:35:24 +0000
> > @@ -4049,7 +4049,7 @@
> >  With no arguments, print all devices known to GRUB.
> >
> >  If the argument is a device name enclosed in parentheses (@pxref{Device
> > -syntax}), then list all files at the root directory of that device.
> > +syntax}), then print the name of the filesystem of that device.
> >
> >  If the argument is a directory given as an absolute file name
> (@pxref{File
> >  name syntax}), then list the contents of that directory.
> >
> >
> > 2013-06-02  Francesco Lavra  <francescolavra.fl@gmail.com>
> >
> >       * grub-core/kern/corecmd.c (grub_core_cmd_ls): Fix listing of root
> >       directory contents.
> >
> > === modified file 'grub-core/kern/corecmd.c'
> > --- grub-core/kern/corecmd.c  2013-06-02 14:23:14 +0000
> > +++ grub-core/kern/corecmd.c  2013-06-02 14:28:24 +0000
> > @@ -147,13 +147,11 @@
> >
> >        if (! *path)
> >       {
> > -       if (grub_errno == GRUB_ERR_UNKNOWN_FS)
> > -         grub_errno = GRUB_ERR_NONE;
> > -
> > -       grub_printf ("(%s): Filesystem is %s.\n",
> > -                    device_name, fs ? fs->name : "unknown");
> > +       /* The argument is a device name: list all files at the root
> directory
> > +          of the device. */
> > +       path = (char *) "/";
> >       }
> > -      else if (fs)
> > +      if (fs)
> >       {
> >         (fs->dir) (dev, path, grub_mini_print_files, NULL);
> >         grub_xputs ("\n");
> >
> > _______________________________________________
> > Grub-devel mailing list
> > Grub-devel@gnu.org
> > https://lists.gnu.org/mailman/listinfo/grub-devel
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
>

[-- Attachment #2: Type: text/html, Size: 5777 bytes --]

Re: [PATCH 2/2] ls core command: handle listing of the root directory

[Andrey Borzenkov]

В Thu, 28 Nov 2013 07:31:54 +0100
"Vladimir 'phcoder' Serbinenko" <phcoder@gmail.com> пишет:

> this very confusing with 2 opposite patches in same mail. I don't have my
> keys here. Feel free to commit documentation patch. If you commit on
> Francesco's behalf don't forget --author

Done.

> On Nov 27, 2013 7:42 PM, "Andrey Borzenkov" <arvidjaar@gmail.com> wrote:
> 
> > What about this patch? Looks correct to me and reflects actual behavior.
> >
> >
> > В Sun, 02 Jun 2013 16:48:23 +0200
> > Francesco Lavra <francescolavra.fl@gmail.com> пишет:
> >
> > > On 05/18/2013 12:26 PM, Francesco Lavra wrote:
> > > > On 05/12/2013 12:56 PM, Francesco Lavra wrote:
> > > >> Currently, listing of the root directory of a device with the command:
> > > >> ls (device_name)
> > > >> requires the underlying filesystem driver to handle an empty path
> > > >> string as if it was the root directory path "/". This introduces
> > > >> duplicated code across the different filesystem drivers. If a given
> > > >> filesystem driver does not implement special handling of the empty
> > > >> path string, the above command gives "error: invalid file name `'."
> > > >> This error happens for instance with the ext4 filesystem.
> > > >> The best place to handle correctly the empty path string and transform
> > > >> it in "/" is the function grub_core_cmd_ls(), so that handling from
> > > >> each filesystem driver is not required anymore.
> > > >
> > > > After revision 5010, issuing the ls command with a device name as
> > > > parameter gives a response such as:
> > > > (device_name): Filesystem is <filesystem name>.
> > > > But grub.texi says:
> > > > "
> > > > @deffn Command ls [arg @dots{}]
> > > > List devices or files.
> > > >
> > > > With no arguments, print all devices known to GRUB.
> > > >
> > > > If the argument is a device name enclosed in parentheses (@pxref{Device
> > > > syntax}), then list all files at the root directory of that device.
> > > >
> > > > If the argument is a directory given as an absolute file name
> > (@pxref{File
> > > > name syntax}), then list the contents of that directory.
> > > > @end deffn
> > > > "
> > > >
> > > > Which is the correct behavior?
> > >
> > > Depending on the answer to the above question, you might want to
> > > consider applying one of the two patches below, as you see fit.
> > >
> > > Regards,
> > > Francesco
> > >
> > > 2013-06-02  Francesco Lavra  <francescolavra.fl@gmail.com>
> > >
> > >       * docs/grub.texi (ls): Fix command description in case of a device
> > name
> > >       passed as argument.
> > >
> > > === modified file 'docs/grub.texi'
> > > --- docs/grub.texi    2013-05-11 05:23:26 +0000
> > > +++ docs/grub.texi    2013-06-02 14:35:24 +0000
> > > @@ -4049,7 +4049,7 @@
> > >  With no arguments, print all devices known to GRUB.
> > >
> > >  If the argument is a device name enclosed in parentheses (@pxref{Device
> > > -syntax}), then list all files at the root directory of that device.
> > > +syntax}), then print the name of the filesystem of that device.
> > >
> > >  If the argument is a directory given as an absolute file name
> > (@pxref{File
> > >  name syntax}), then list the contents of that directory.
> > >
> > >
> > > 2013-06-02  Francesco Lavra  <francescolavra.fl@gmail.com>
> > >
> > >       * grub-core/kern/corecmd.c (grub_core_cmd_ls): Fix listing of root
> > >       directory contents.
> > >
> > > === modified file 'grub-core/kern/corecmd.c'
> > > --- grub-core/kern/corecmd.c  2013-06-02 14:23:14 +0000
> > > +++ grub-core/kern/corecmd.c  2013-06-02 14:28:24 +0000
> > > @@ -147,13 +147,11 @@
> > >
> > >        if (! *path)
> > >       {
> > > -       if (grub_errno == GRUB_ERR_UNKNOWN_FS)
> > > -         grub_errno = GRUB_ERR_NONE;
> > > -
> > > -       grub_printf ("(%s): Filesystem is %s.\n",
> > > -                    device_name, fs ? fs->name : "unknown");
> > > +       /* The argument is a device name: list all files at the root
> > directory
> > > +          of the device. */
> > > +       path = (char *) "/";
> > >       }
> > > -      else if (fs)
> > > +      if (fs)
> > >       {
> > >         (fs->dir) (dev, path, grub_mini_print_files, NULL);
> > >         grub_xputs ("\n");
> > >
> > > _______________________________________________
> > > Grub-devel mailing list
> > > Grub-devel@gnu.org
> > > https://lists.gnu.org/mailman/listinfo/grub-devel
> >
> >
> > _______________________________________________
> > Grub-devel mailing list
> > Grub-devel@gnu.org
> > https://lists.gnu.org/mailman/listinfo/grub-devel
> >