| Message ID | 20200122110852.2741617-1-wd@denx.de |
|---|---|
| State | New |
| Headers | show |
| Series | [FS] Print error message for unknown device type | expand |
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
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 :)
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
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
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 --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) {
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(-)