[PATCH] part: return -ENOSYS when get_info not valid.

[schspa]

在 2021-10-20星期三的 06:44 +0200，Heinrich Schuchardt写道：
> 
> 
> On 10/20/21 04:37, schspa wrote:
> > 在 2021-10-19星期二的 17:23 +0200，Heinrich Schuchardt写道：
> > > On 10/19/21 15:35, zhaohui.shi wrote:
> > > > From: schspa <schspa at gmail.com>
> > > > 
> > > > In some case, get_info() interface can be NULL, add this check to
> > > > stop
> > > > from crash.
> > > 
> > > Thank you for reviewing the partition driver code.
> > > 
> > > There seems to be no driver missing a get_info implementation.
> > > Where
> > > did
> > > you run into a problem?
> > > 
> > Yes, I do run into a problem, In my spl build, CONFIG_SPL_FS_EXT4,
> > CONFIG_SPL_FS_FAT, CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_PARTITION are all
> > not enabled. In this case, get_info implementation is NULL. see
> > 'part_get_info_ptr' and 'part_print_ptr' and part_efi.c for detail.
> > 
> > > Why should we only check .get_info and not .test and not .print?
> > > 
> > 
> > All part type driver have .test implementation, it can't be NULL,
> > and .print have NULL pointer judgement already.
> > 
> > > > 
> > > > Signed-off-by: schspa <schspa at gmail.com>
> > > 
> > > Please, provide a name.
> > > 
> > > > ---
> > > >    disk/part.c | 7 +++++++
> > > >    1 file changed, 7 insertions(+)
> > > > 
> > > > diff --git a/disk/part.c b/disk/part.c
> > > > index a6a8f7052b..7af3240ec7 100644
> > > > --- a/disk/part.c
> > > > +++ b/disk/part.c
> > > > @@ -668,6 +668,13 @@ int part_get_info_by_name_type(struct
> > > > blk_desc
> > > > *dev_desc, const char *name,
> > > >          part_drv = part_driver_lookup_type(dev_desc);
> > > >          if (!part_drv)
> > > >                  return -1;
> > > > +
> > > > +       if (!part_drv->get_info) {
> > > > +               PRINTF("## Driver %s does not have the get_info()
> > > > method\n",
> > > > +                      part_drv->name);
> > > 
> > > Please, use log_debug() to avoid noise on the console on every
> > > boot.
> > > 
> > 
> > I think it's OK to use PRINTF, because of this BUG occurs only when
> > user give a bad part configuration, and this error message can prompt
> > the user that a configuration error has occurred.
> > Besides, 'part_get_info' use PRINTF for this null pointer protection
> > too.
> 
> Above you write that part_drv->get_info = NULL is part of your
> configuration. So it will always be printed. The size of the SPL is
> very
> critical on many boards. We should avoid anything that increases it.
> 

Considering this problem, I will upload a new patch to use log_debug()
for error message.

> Why is part_get_info_by_name_type() being called in your configuration
> which does not provide a partition driver in SPL? Are there some
> dependencies missing in the Kconfig files?
> 

In my case, I have 
CONFIG_SPL_EFI_PARTITION=y
CONFIG_SPL_LIBDISK_SUPPORT=y
but forget to enable CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_USE_PARTITION.


Yes, it seems we can have some dependencies in Kconfig like this:
config SPL_LIBDISK_SUPPORT
	bool "Support disk partitions"
	depends on CONFIG_SPL_EXT_SUPPORT || CONFIG_SPL_FAT_SUPPORT ||
CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_PARTITION
	help

But there is some case for part_iso or part_mac, which have no this
dependencies (because of it don't use part_get_info_ptr to wrap
part_get_info_iso).

How do you think about this ?  Should we add such dependencies ?
part_iso seems shouldn't have dependencies about ext, fat, mmcsd etc.


> Best regards
> 
> Heinrich
> 
> > 
> > > Best regards
> > > 
> > > Heinrich
> > > 
> > > > +               return -ENOSYS;
> > > > +       }
> > > > +
> > > >          for (i = 1; i < part_drv->max_entries; i++) {
> > > >                  ret = part_drv->get_info(dev_desc, i, info);
> > > >                  if (ret != 0) {
> > > > 
> > 

-- 
schspa <schspa at gmail.com>