[PATCH v2] part: return -ENOSYS when get_info not valid.
In some case, get_info() interface can be NULL, add this check to stop
from crash.
Signed-off-by: schspa
---
disk/part.c | 7 +++
1 file changed, 7 insertions(+)
diff --git a/disk/part.c b/disk/part.c
index a6a8f7052b..f9b9b89861 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) {
+ log_debug("## Driver %s does not have the get_info() method\n",
+ part_drv->name);
+ return -ENOSYS;
+ }
+
for (i = 1; i < part_drv->max_entries; i++) {
ret = part_drv->get_info(dev_desc, i, info);
if (ret != 0) {
--
2.33.1
Re: [PATCH] part: return -ENOSYS when get_info not valid.
在 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
> > > >
> > > > 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
> > >
> > > 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
Re: [PATCH] part: return -ENOSYS when get_info not valid.
在 2021-10-19星期二的 17:23 +0200，Heinrich Schuchardt写道：
> On 10/19/21 15:35, zhaohui.shi wrote:
> > From: schspa
> >
> > 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
>
> 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.
> 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) {
> >
--
Best regards
schspa
[PATCH] part: return -ENOSYS when get_info not valid.
In some case, get_info() interface can be NULL, add this check to stop
from crash.
Signed-off-by: schspa
---
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);
+ return -ENOSYS;
+ }
+
for (i = 1; i < part_drv->max_entries; i++) {
ret = part_drv->get_info(dev_desc, i, info);
if (ret != 0) {
--
2.33.1
[PATCH] avb: Fix error when partition not found
part_get_info_by_name will return -1 on error, and >0 on success.
Signed-off-by: schspa
Cc: Igor Opaniuk
---
common/avb_verify.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/common/avb_verify.c b/common/avb_verify.c
index db10d0f21f..0520a71455 100644
--- a/common/avb_verify.c
+++ b/common/avb_verify.c
@@ -369,7 +369,7 @@ static struct mmc_part *get_partition(AvbOps *ops, const
char *partition)
}
ret = part_get_info_by_name(mmc_blk, partition, >info);
- if (!ret) {
+ if (ret < 0) {
printf("Can't find partition '%s'\n", partition);
goto err;
}
--
2.24.3 (Apple Git-128)
