diff mbox series

[FS] Print error message for unknown device type

Message ID 20200122110852.2741617-1-wd@denx.de
State New
Headers show
Series [FS] Print error message for unknown device type | expand

Commit Message

Wolfgang Denk Jan. 22, 2020, 11:08 a.m. UTC
File system commands like "ls" etc. require a device type parameter.
If an unknown type is specified, they return an error code but no
visible feedback to the user:

 -> ls FOOBAR 1:1 /
 ->

Add an error message to make clear what happens, and why.

Signed-off-by: Wolfgang Denk <wd at denx.de>
---
 disk/part.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Comments

Heiko Schocher Jan. 22, 2020, 11:22 a.m. UTC | #1
Hello Wolfgang,

Am 22.01.2020 um 12:08 schrieb Wolfgang Denk:
> File system commands like "ls" etc. require a device type parameter.
> If an unknown type is specified, they return an error code but no
> visible feedback to the user:
> 
>   -> ls FOOBAR 1:1 /
>   ->
> 
> Add an error message to make clear what happens, and why.
> 
> Signed-off-by: Wolfgang Denk <wd at denx.de>
> ---
>   disk/part.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)

Tested on wandboard.

Tested-by: Heiko Schocher <hs at denx.de>


> diff --git a/disk/part.c b/disk/part.c
> index 8982ef3bae..14000835c8 100644
> --- a/disk/part.c
> +++ b/disk/part.c
> @@ -512,8 +512,10 @@ int blk_get_device_part_str(const char *ifname, const char *dev_part_str,
>   
>   	/* Look up the device */
>   	dev = blk_get_device_by_str(ifname, dev_str, dev_desc);
> -	if (dev < 0)
> +	if (dev < 0) {
> +		printf("** Unknown device type %s **\n", ifname);
>   		goto cleanup;
> +	}

It would be nice to have here a list of supported devices, so a user
can see what are valid arguments for ifname.

>   
>   	/* Convert partition ID string to number */
>   	if (!part_str || !*part_str) {
> 

bye,
Heiko
Nikita Ermakov Jan. 22, 2020, 12:06 p.m. UTC | #2
On Wed, 22 Jan 2020 at 14:09, Wolfgang Denk <wd at denx.de> wrote:

> File system commands like "ls" etc. require a device type parameter.
> If an unknown type is specified, they return an error code but no
> visible feedback to the user:
>
>  -> ls FOOBAR 1:1 /
>  ->
>
> Add an error message to make clear what happens, and why.
>
> Signed-off-by: Wolfgang Denk <wd at denx.de>
> ---
>  disk/part.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/disk/part.c b/disk/part.c
> index 8982ef3bae..14000835c8 100644
> --- a/disk/part.c
> +++ b/disk/part.c
> @@ -512,8 +512,10 @@ int blk_get_device_part_str(const char *ifname, const
> char *dev_part_str,
>
>         /* Look up the device */
>         dev = blk_get_device_by_str(ifname, dev_str, dev_desc);
> -       if (dev < 0)
> +       if (dev < 0) {
> +               printf("** Unknown device type %s **\n", ifname);
>                 goto cleanup;
> +       }
>
>         /* Convert partition ID string to number */
>         if (!part_str || !*part_str) {
> --
> 2.23.0
>
>
Oww, I've submitted the similar patch to this mailing list recently but it
is waiting for moderator approval :)
Wolfgang Denk Jan. 22, 2020, 12:12 p.m. UTC | #3
Dear Heiko,

In message <3546d28c-f638-5357-a20f-5d03db762be0 at denx.de> you wrote:
>
> > File system commands like "ls" etc. require a device type parameter.
> > If an unknown type is specified, they return an error code but no
> > visible feedback to the user:
> > 
> >   -> ls FOOBAR 1:1 /
> >   ->
> > 
> > Add an error message to make clear what happens, and why.
> > 
> > Signed-off-by: Wolfgang Denk <wd at denx.de>
> > ---
> >   disk/part.c | 4 +++-
> >   1 file changed, 3 insertions(+), 1 deletion(-)
>
> Tested on wandboard.
>
> Tested-by: Heiko Schocher <hs at denx.de>

Thanks for testing.

> > -	if (dev < 0)
> > +	if (dev < 0) {
> > +		printf("** Unknown device type %s **\n", ifname);
> >   		goto cleanup;
> > +	}
>
> It would be nice to have here a list of supported devices, so a user
> can see what are valid arguments for ifname.

Yes, you are absolutely right.  I aready thought about this, but I
have to admit that I got stuck in the code; there are several
complexities - code for example for blk_driver_lookup_typename()
is duplicated both in drivers/block/blk_legacy.c   and in
drivers/block/blk-uclass.c;  I was not able to find any exported
interface that actually allows to get a list of supported device
drivers, and the things I tried all looked really ugly to me.

Adding Simon to Cc: - he has designed and written all this code and
should know better.

Simon, what would be a clean and elegant approach to get such a list
of supported drivers ?


In any case I recommend to accept this patch as is; this other thing
is additional information that can /should get added later in a
spearate patch.

Best regards,

Wolfgang Denk
Wolfgang Denk Jan. 24, 2020, 1:21 p.m. UTC | #4
Hello Simon,

ping...

In message <20200122121253.7B0B3240638 at gemini.denx.de> I wrote:
> Dear Heiko,
>
> In message <3546d28c-f638-5357-a20f-5d03db762be0 at denx.de> you wrote:
> >
> > > File system commands like "ls" etc. require a device type parameter.
> > > If an unknown type is specified, they return an error code but no
> > > visible feedback to the user:
> > > 
> > >   -> ls FOOBAR 1:1 /
> > >   ->
> > > 
> > > Add an error message to make clear what happens, and why.
> > > 
> > > Signed-off-by: Wolfgang Denk <wd at denx.de>
> > > ---
> > >   disk/part.c | 4 +++-
> > >   1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > Tested on wandboard.
> >
> > Tested-by: Heiko Schocher <hs at denx.de>
>
> Thanks for testing.
>
> > > -	if (dev < 0)
> > > +	if (dev < 0) {
> > > +		printf("** Unknown device type %s **\n", ifname);
> > >   		goto cleanup;
> > > +	}
> >
> > It would be nice to have here a list of supported devices, so a user
> > can see what are valid arguments for ifname.
>
> Yes, you are absolutely right.  I aready thought about this, but I
> have to admit that I got stuck in the code; there are several
> complexities - code for example for blk_driver_lookup_typename()
> is duplicated both in drivers/block/blk_legacy.c   and in
> drivers/block/blk-uclass.c;  I was not able to find any exported
> interface that actually allows to get a list of supported device
> drivers, and the things I tried all looked really ugly to me.
>
> Adding Simon to Cc: - he has designed and written all this code and
> should know better.
>
> Simon, what would be a clean and elegant approach to get such a list
> of supported drivers ?
>
>
> In any case I recommend to accept this patch as is; this other thing
> is additional information that can /should get added later in a
> spearate patch.
>
> Best regards,
>
> Wolfgang Denk

Best regards,

Wolfgang Denk
Simon Glass Jan. 30, 2020, 2:16 a.m. UTC | #5
Hi,

On Fri, 24 Jan 2020 at 06:21, Wolfgang Denk <wd at denx.de> wrote:
>
> Hello Simon,
>
> ping...
>
> In message <20200122121253.7B0B3240638 at gemini.denx.de> I wrote:
> > Dear Heiko,
> >
> > In message <3546d28c-f638-5357-a20f-5d03db762be0 at denx.de> you wrote:
> > >
> > > > File system commands like "ls" etc. require a device type parameter.
> > > > If an unknown type is specified, they return an error code but no
> > > > visible feedback to the user:
> > > >
> > > >   -> ls FOOBAR 1:1 /
> > > >   ->
> > > >
> > > > Add an error message to make clear what happens, and why.
> > > >
> > > > Signed-off-by: Wolfgang Denk <wd at denx.de>
> > > > ---
> > > >   disk/part.c | 4 +++-
> > > >   1 file changed, 3 insertions(+), 1 deletion(-)
> > >
> > > Tested on wandboard.
> > >
> > > Tested-by: Heiko Schocher <hs at denx.de>
> >
> > Thanks for testing.
> >
> > > > - if (dev < 0)
> > > > + if (dev < 0) {
> > > > +         printf("** Unknown device type %s **\n", ifname);
> > > >                   goto cleanup;
> > > > + }

Reviewed-by: Simon Glass <sjg at chromium.org>

> > >
> > > It would be nice to have here a list of supported devices, so a user
> > > can see what are valid arguments for ifname.

There are two functions involved here, since we still support non-DM
block devices. The function called is blk_get_devnum_by_typename(),
with two implementations.

So we would need two different implementations to read the list of devices:

- blk_get_devnum_by_typename() has a uclass_foreach_dev() loop
- blk_driver_lookup_typename() shows how to iterate through the legacy
block devices

> >
> > Yes, you are absolutely right.  I aready thought about this, but I
> > have to admit that I got stuck in the code; there are several
> > complexities - code for example for blk_driver_lookup_typename()
> > is duplicated both in drivers/block/blk_legacy.c   and in
> > drivers/block/blk-uclass.c;  I was not able to find any exported
> > interface that actually allows to get a list of supported device
> > drivers, and the things I tried all looked really ugly to me.
> >
> > Adding Simon to Cc: - he has designed and written all this code and
> > should know better.
> >
> > Simon, what would be a clean and elegant approach to get such a list
> > of supported drivers ?
> >
> >
> > In any case I recommend to accept this patch as is; this other thing
> > is additional information that can /should get added later in a
> > spearate patch.

Regards,
Simon
diff mbox series

Patch

diff --git a/disk/part.c b/disk/part.c
index 8982ef3bae..14000835c8 100644
--- a/disk/part.c
+++ b/disk/part.c
@@ -512,8 +512,10 @@  int blk_get_device_part_str(const char *ifname, const char *dev_part_str,
 
 	/* Look up the device */
 	dev = blk_get_device_by_str(ifname, dev_str, dev_desc);
-	if (dev < 0)
+	if (dev < 0) {
+		printf("** Unknown device type %s **\n", ifname);
 		goto cleanup;
+	}
 
 	/* Convert partition ID string to number */
 	if (!part_str || !*part_str) {