[PATCH v2 1/3] cmd: fs: Use part_get_info_by_dev_and_name_or_num to parse partitions
This allows using dev#partlabel syntax.
Signed-off-by: Sean Anderson sean.anderson@seco.com ---
Changes in v2: - Add stub for when CONFIG_PARTITIONS=n
fs/fs.c | 4 ++-- include/part.h | 12 +++++++++++- 2 files changed, 13 insertions(+), 3 deletions(-)
diff --git a/fs/fs.c b/fs/fs.c index 900928c394..b7936fd4cf 100644 --- a/fs/fs.c +++ b/fs/fs.c @@ -385,8 +385,8 @@ int fs_set_blk_dev(const char *ifname, const char *dev_part_str, int fstype) } #endif
- part = blk_get_device_part_str(ifname, dev_part_str, &fs_dev_desc, - &fs_partition, 1); + part = part_get_info_by_dev_and_name_or_num(ifname, dev_part_str, &fs_dev_desc, + &fs_partition, 1); if (part < 0) return -1;
diff --git a/include/part.h b/include/part.h index 7f78271a98..419c859708 100644 --- a/include/part.h +++ b/include/part.h @@ -230,7 +230,7 @@ int part_get_info_by_name(struct blk_desc *dev_desc, * @param[in] allow_whole_dev true to allow the user to select partition 0 * (which means the whole device), false to require a valid * partition number >= 1 - * @return 0 on success, or a negative on error + * @return the partition number on success, or negative errno on error */ int part_get_info_by_dev_and_name_or_num(const char *dev_iface, const char *dev_part_str, @@ -275,6 +275,16 @@ static inline int blk_get_device_part_str(const char *ifname, struct disk_partition *info, int allow_whole_dev) { *dev_desc = NULL; return -1; } +static inline int +part_get_info_by_dev_and_name_or_num(const char *dev_iface, + const char *dev_part_str, + struct blk_desc **dev_desc, + struct disk_partition *part_info, + int allow_whole_dev) +{ + *dev_desc = NULL; + return -ENOSYS; +} #endif
/*
blk_get_device_by_str returns the device number on success. So we must check if the return was negative to determine an error.
Signed-off-by: Sean Anderson sean.anderson@seco.com ---
Changes in v2: - New
disk/part.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/disk/part.c b/disk/part.c index 80ced2ba88..5e7e59cf25 100644 --- a/disk/part.c +++ b/disk/part.c @@ -725,7 +725,7 @@ static int part_get_info_by_dev_and_name(const char *dev_iface, }
ret = blk_get_device_by_str(dev_iface, dev_str, dev_desc); - if (ret) + if (ret < 0) goto cleanup;
ret = part_get_info_by_name(*dev_desc, part_str, part_info);
On Mon, 12 Apr 2021 at 23:53, Sean Anderson sean.anderson@seco.com wrote:
blk_get_device_by_str returns the device number on success. So we must check if the return was negative to determine an error.
Signed-off-by: Sean Anderson sean.anderson@seco.com
Changes in v2:
- New
disk/part.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by: Simon Glass sjg@chromium.org
On Mon, Apr 12, 2021 at 06:53:06PM -0400, Sean Anderson wrote:
blk_get_device_by_str returns the device number on success. So we must check if the return was negative to determine an error.
Signed-off-by: Sean Anderson sean.anderson@seco.com Reviewed-by: Simon Glass sjg@chromium.org
Applied to u-boot/master, thanks!
This is technically a library function, but we use MMCs for testing, so it is easier to do it with DM. At the moment, the only block devices in sandbox are MMCs (AFAIK) so we just test with those.
Signed-off-by: Sean Anderson sean.anderson@seco.com ---
Changes in v2: - New
test/dm/Makefile | 1 + test/dm/part.c | 76 ++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 77 insertions(+) create mode 100644 test/dm/part.c
diff --git a/test/dm/Makefile b/test/dm/Makefile index f5cc5540e8..7d017f8750 100644 --- a/test/dm/Makefile +++ b/test/dm/Makefile @@ -98,5 +98,6 @@ endif ifneq ($(CONFIG_EFI_PARTITION),) obj-$(CONFIG_FASTBOOT_FLASH_MMC) += fastboot.o endif +obj-$(CONFIG_EFI_PARTITION) += part.o endif endif # !SPL diff --git a/test/dm/part.c b/test/dm/part.c new file mode 100644 index 0000000000..051e9010b6 --- /dev/null +++ b/test/dm/part.c @@ -0,0 +1,76 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* + * Copyright (C) 2020 Sean Anderson sean.anderson@seco.com + */ + +#include <common.h> +#include <dm.h> +#include <mmc.h> +#include <part.h> +#include <part_efi.h> +#include <dm/test.h> +#include <test/ut.h> + +static int dm_test_part(struct unit_test_state *uts) +{ + char str_disk_guid[UUID_STR_LEN + 1]; + struct blk_desc *mmc_dev_desc; + struct disk_partition part_info; + struct disk_partition parts[2] = { + { + .start = 48, /* GPT data takes up the first 34 blocks or so */ + .size = 1, + .name = "test1", + }, + { + .start = 49, + .size = 1, + .name = "test2", + }, + }; + + ut_asserteq(1, blk_get_device_by_str("mmc", "1", &mmc_dev_desc)); + if (CONFIG_IS_ENABLED(RANDOM_UUID)) { + gen_rand_uuid_str(parts[0].uuid, UUID_STR_FORMAT_STD); + gen_rand_uuid_str(parts[1].uuid, UUID_STR_FORMAT_STD); + gen_rand_uuid_str(str_disk_guid, UUID_STR_FORMAT_STD); + } + ut_assertok(gpt_restore(mmc_dev_desc, str_disk_guid, parts, + ARRAY_SIZE(parts))); + +#define test(expected, part_str, whole) \ + ut_asserteq(expected, \ + part_get_info_by_dev_and_name_or_num("mmc", part_str, \ + &mmc_dev_desc, \ + &part_info, whole)) + + test(-ENODEV, "", true); + env_set("bootdevice", "0"); + test(0, "", true); + env_set("bootdevice", "1"); + test(1, "", false); + test(1, "-", false); + env_set("bootdevice", ""); + test(-EPROTONOSUPPORT, "0", false); + test(0, "0", true); + test(0, ":0", true); + test(0, ".0", true); + test(0, ".0:0", true); + test(-EINVAL, "#test1", true); + test(1, "1", false); + test(1, "1", true); + test(-ENOENT, "1:0", false); + test(0, "1:0", true); + test(1, "1:1", false); + test(2, "1:2", false); + test(1, "1.0", false); + test(0, "1.0:0", true); + test(1, "1.0:1", false); + test(2, "1.0:2", false); + test(-EINVAL, "1#bogus", false); + test(1, "1#test1", false); + test(2, "1#test2", false); + + return 0; +} +DM_TEST(dm_test_part, UT_TESTF_SCAN_PDATA | UT_TESTF_SCAN_FDT);
Hi Sean,
On Mon, 12 Apr 2021 at 23:53, Sean Anderson sean.anderson@seco.com wrote:
This is technically a library function, but we use MMCs for testing, so it is easier to do it with DM. At the moment, the only block devices in sandbox are MMCs (AFAIK) so we just test with those.
Signed-off-by: Sean Anderson sean.anderson@seco.com
Changes in v2:
- New
test/dm/Makefile | 1 + test/dm/part.c | 76 ++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 77 insertions(+) create mode 100644 test/dm/part.c
diff --git a/test/dm/Makefile b/test/dm/Makefile index f5cc5540e8..7d017f8750 100644 --- a/test/dm/Makefile +++ b/test/dm/Makefile @@ -98,5 +98,6 @@ endif ifneq ($(CONFIG_EFI_PARTITION),) obj-$(CONFIG_FASTBOOT_FLASH_MMC) += fastboot.o endif +obj-$(CONFIG_EFI_PARTITION) += part.o endif endif # !SPL diff --git a/test/dm/part.c b/test/dm/part.c new file mode 100644 index 0000000000..051e9010b6 --- /dev/null +++ b/test/dm/part.c @@ -0,0 +1,76 @@ +// SPDX-License-Identifier: GPL-2.0+ +/*
- Copyright (C) 2020 Sean Anderson sean.anderson@seco.com
- */
+#include <common.h> +#include <dm.h> +#include <mmc.h> +#include <part.h> +#include <part_efi.h> +#include <dm/test.h> +#include <test/ut.h>
+static int dm_test_part(struct unit_test_state *uts) +{
char str_disk_guid[UUID_STR_LEN + 1];struct blk_desc *mmc_dev_desc;struct disk_partition part_info;struct disk_partition parts[2] = {{.start = 48, /* GPT data takes up the first 34 blocks or so */.size = 1,.name = "test1",},{.start = 49,.size = 1,.name = "test2",},};ut_asserteq(1, blk_get_device_by_str("mmc", "1", &mmc_dev_desc));if (CONFIG_IS_ENABLED(RANDOM_UUID)) {gen_rand_uuid_str(parts[0].uuid, UUID_STR_FORMAT_STD);gen_rand_uuid_str(parts[1].uuid, UUID_STR_FORMAT_STD);gen_rand_uuid_str(str_disk_guid, UUID_STR_FORMAT_STD);}ut_assertok(gpt_restore(mmc_dev_desc, str_disk_guid, parts,ARRAY_SIZE(parts)));+#define test(expected, part_str, whole) \
Can this be a function instead of a macro?
ut_asserteq(expected, \part_get_info_by_dev_and_name_or_num("mmc", part_str, \&mmc_dev_desc, \&part_info, whole))test(-ENODEV, "", true);env_set("bootdevice", "0");test(0, "", true);env_set("bootdevice", "1");test(1, "", false);test(1, "-", false);env_set("bootdevice", "");test(-EPROTONOSUPPORT, "0", false);test(0, "0", true);test(0, ":0", true);test(0, ".0", true);test(0, ".0:0", true);test(-EINVAL, "#test1", true);test(1, "1", false);test(1, "1", true);test(-ENOENT, "1:0", false);test(0, "1:0", true);test(1, "1:1", false);test(2, "1:2", false);test(1, "1.0", false);test(0, "1.0:0", true);test(1, "1.0:1", false);test(2, "1.0:2", false);test(-EINVAL, "1#bogus", false);test(1, "1#test1", false);test(2, "1#test2", false);return 0;+}
+DM_TEST(dm_test_part, UT_TESTF_SCAN_PDATA | UT_TESTF_SCAN_FDT);
2.25.1
Regards, Simon
On 4/14/21 3:37 PM, Simon Glass wrote:
Hi Sean,
On Mon, 12 Apr 2021 at 23:53, Sean Anderson sean.anderson@seco.com wrote:
This is technically a library function, but we use MMCs for testing, so it is easier to do it with DM. At the moment, the only block devices in sandbox are MMCs (AFAIK) so we just test with those.
Signed-off-by: Sean Anderson sean.anderson@seco.com
Changes in v2:
New
test/dm/Makefile | 1 + test/dm/part.c | 76 ++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 77 insertions(+) create mode 100644 test/dm/part.c
diff --git a/test/dm/Makefile b/test/dm/Makefile index f5cc5540e8..7d017f8750 100644 --- a/test/dm/Makefile +++ b/test/dm/Makefile @@ -98,5 +98,6 @@ endif ifneq ($(CONFIG_EFI_PARTITION),) obj-$(CONFIG_FASTBOOT_FLASH_MMC) += fastboot.o endif +obj-$(CONFIG_EFI_PARTITION) += part.o endif endif # !SPL diff --git a/test/dm/part.c b/test/dm/part.c new file mode 100644 index 0000000000..051e9010b6 --- /dev/null +++ b/test/dm/part.c @@ -0,0 +1,76 @@ +// SPDX-License-Identifier: GPL-2.0+ +/*
- Copyright (C) 2020 Sean Anderson sean.anderson@seco.com
- */
+#include <common.h> +#include <dm.h> +#include <mmc.h> +#include <part.h> +#include <part_efi.h> +#include <dm/test.h> +#include <test/ut.h>
+static int dm_test_part(struct unit_test_state *uts) +{
char str_disk_guid[UUID_STR_LEN + 1];struct blk_desc *mmc_dev_desc;struct disk_partition part_info;struct disk_partition parts[2] = {{.start = 48, /* GPT data takes up the first 34 blocks or so */.size = 1,.name = "test1",},{.start = 49,.size = 1,.name = "test2",},};ut_asserteq(1, blk_get_device_by_str("mmc", "1", &mmc_dev_desc));if (CONFIG_IS_ENABLED(RANDOM_UUID)) {gen_rand_uuid_str(parts[0].uuid, UUID_STR_FORMAT_STD);gen_rand_uuid_str(parts[1].uuid, UUID_STR_FORMAT_STD);gen_rand_uuid_str(str_disk_guid, UUID_STR_FORMAT_STD);}ut_assertok(gpt_restore(mmc_dev_desc, str_disk_guid, parts,ARRAY_SIZE(parts)));+#define test(expected, part_str, whole) \
Can this be a function instead of a macro?
Not one-to-one because ut-asserteq returns on error. This could be changed to
ut_asserteq(-ENODEV, test("", true));
but I think a macro is the simplest option.
--Sean
ut_asserteq(expected, \part_get_info_by_dev_and_name_or_num("mmc", part_str, \&mmc_dev_desc, \&part_info, whole))test(-ENODEV, "", true);env_set("bootdevice", "0");test(0, "", true);env_set("bootdevice", "1");test(1, "", false);test(1, "-", false);env_set("bootdevice", "");test(-EPROTONOSUPPORT, "0", false);test(0, "0", true);test(0, ":0", true);test(0, ".0", true);test(0, ".0:0", true);test(-EINVAL, "#test1", true);test(1, "1", false);test(1, "1", true);test(-ENOENT, "1:0", false);test(0, "1:0", true);test(1, "1:1", false);test(2, "1:2", false);test(1, "1.0", false);test(0, "1.0:0", true);test(1, "1.0:1", false);test(2, "1.0:2", false);test(-EINVAL, "1#bogus", false);test(1, "1#test1", false);test(2, "1#test2", false);return 0;+}
+DM_TEST(dm_test_part, UT_TESTF_SCAN_PDATA | UT_TESTF_SCAN_FDT);
2.25.1
Regards, Simon
Hi Sean,
On Fri, 16 Apr 2021 at 02:36, Sean Anderson sean.anderson@seco.com wrote:
On 4/14/21 3:37 PM, Simon Glass wrote:
Hi Sean,
On Mon, 12 Apr 2021 at 23:53, Sean Anderson sean.anderson@seco.com wrote:
This is technically a library function, but we use MMCs for testing, so it is easier to do it with DM. At the moment, the only block devices in sandbox are MMCs (AFAIK) so we just test with those.
Signed-off-by: Sean Anderson sean.anderson@seco.com
Changes in v2:
New
test/dm/Makefile | 1 + test/dm/part.c | 76 ++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 77 insertions(+) create mode 100644 test/dm/part.c
diff --git a/test/dm/Makefile b/test/dm/Makefile index f5cc5540e8..7d017f8750 100644 --- a/test/dm/Makefile +++ b/test/dm/Makefile @@ -98,5 +98,6 @@ endif ifneq ($(CONFIG_EFI_PARTITION),) obj-$(CONFIG_FASTBOOT_FLASH_MMC) += fastboot.o endif +obj-$(CONFIG_EFI_PARTITION) += part.o endif endif # !SPL diff --git a/test/dm/part.c b/test/dm/part.c new file mode 100644 index 0000000000..051e9010b6 --- /dev/null +++ b/test/dm/part.c @@ -0,0 +1,76 @@ +// SPDX-License-Identifier: GPL-2.0+ +/*
- Copyright (C) 2020 Sean Anderson sean.anderson@seco.com
- */
+#include <common.h> +#include <dm.h> +#include <mmc.h> +#include <part.h> +#include <part_efi.h> +#include <dm/test.h> +#include <test/ut.h>
+static int dm_test_part(struct unit_test_state *uts) +{
char str_disk_guid[UUID_STR_LEN + 1];struct blk_desc *mmc_dev_desc;struct disk_partition part_info;struct disk_partition parts[2] = {{.start = 48, /* GPT data takes up the first 34 blocks or so */.size = 1,.name = "test1",},{.start = 49,.size = 1,.name = "test2",},};ut_asserteq(1, blk_get_device_by_str("mmc", "1", &mmc_dev_desc));if (CONFIG_IS_ENABLED(RANDOM_UUID)) {gen_rand_uuid_str(parts[0].uuid, UUID_STR_FORMAT_STD);gen_rand_uuid_str(parts[1].uuid, UUID_STR_FORMAT_STD);gen_rand_uuid_str(str_disk_guid, UUID_STR_FORMAT_STD);}ut_assertok(gpt_restore(mmc_dev_desc, str_disk_guid, parts,ARRAY_SIZE(parts)));+#define test(expected, part_str, whole) \
Can this be a function instead of a macro?
Not one-to-one because ut-asserteq returns on error. This could be changed to
ut_asserteq(-ENODEV, test("", true));but I think a macro is the simplest option.
Well you are using ut_asserteq() in the macro so I don't see why the macro is better than what you have above, with the code in a function? That is what we normally do.
--Sean
ut_asserteq(expected, \part_get_info_by_dev_and_name_or_num("mmc", part_str, \&mmc_dev_desc, \&part_info, whole))test(-ENODEV, "", true);env_set("bootdevice", "0");test(0, "", true);env_set("bootdevice", "1");test(1, "", false);test(1, "-", false);env_set("bootdevice", "");test(-EPROTONOSUPPORT, "0", false);test(0, "0", true);test(0, ":0", true);test(0, ".0", true);test(0, ".0:0", true);test(-EINVAL, "#test1", true);test(1, "1", false);test(1, "1", true);test(-ENOENT, "1:0", false);test(0, "1:0", true);test(1, "1:1", false);test(2, "1:2", false);test(1, "1.0", false);test(0, "1.0:0", true);test(1, "1.0:1", false);test(2, "1.0:2", false);test(-EINVAL, "1#bogus", false);test(1, "1#test1", false);test(2, "1#test2", false);return 0;+}
+DM_TEST(dm_test_part, UT_TESTF_SCAN_PDATA | UT_TESTF_SCAN_FDT);
2.25.1
Regards, Simon
On 4/21/21 3:14 AM, Simon Glass wrote:
Hi Sean,
On Fri, 16 Apr 2021 at 02:36, Sean Anderson sean.anderson@seco.com wrote:
On 4/14/21 3:37 PM, Simon Glass wrote:
Hi Sean,
On Mon, 12 Apr 2021 at 23:53, Sean Anderson sean.anderson@seco.com wrote:
This is technically a library function, but we use MMCs for testing, so it is easier to do it with DM. At the moment, the only block devices in sandbox are MMCs (AFAIK) so we just test with those.
Signed-off-by: Sean Anderson sean.anderson@seco.com
Changes in v2:
New
test/dm/Makefile | 1 + test/dm/part.c | 76 ++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 77 insertions(+) create mode 100644 test/dm/part.c
diff --git a/test/dm/Makefile b/test/dm/Makefile index f5cc5540e8..7d017f8750 100644 --- a/test/dm/Makefile +++ b/test/dm/Makefile @@ -98,5 +98,6 @@ endif ifneq ($(CONFIG_EFI_PARTITION),) obj-$(CONFIG_FASTBOOT_FLASH_MMC) += fastboot.o endif +obj-$(CONFIG_EFI_PARTITION) += part.o endif endif # !SPL diff --git a/test/dm/part.c b/test/dm/part.c new file mode 100644 index 0000000000..051e9010b6 --- /dev/null +++ b/test/dm/part.c @@ -0,0 +1,76 @@ +// SPDX-License-Identifier: GPL-2.0+ +/*
- Copyright (C) 2020 Sean Anderson sean.anderson@seco.com
- */
+#include <common.h> +#include <dm.h> +#include <mmc.h> +#include <part.h> +#include <part_efi.h> +#include <dm/test.h> +#include <test/ut.h>
+static int dm_test_part(struct unit_test_state *uts) +{
char str_disk_guid[UUID_STR_LEN + 1];struct blk_desc *mmc_dev_desc;struct disk_partition part_info;struct disk_partition parts[2] = {{.start = 48, /* GPT data takes up the first 34 blocks or so */.size = 1,.name = "test1",},{.start = 49,.size = 1,.name = "test2",},};ut_asserteq(1, blk_get_device_by_str("mmc", "1", &mmc_dev_desc));if (CONFIG_IS_ENABLED(RANDOM_UUID)) {gen_rand_uuid_str(parts[0].uuid, UUID_STR_FORMAT_STD);gen_rand_uuid_str(parts[1].uuid, UUID_STR_FORMAT_STD);gen_rand_uuid_str(str_disk_guid, UUID_STR_FORMAT_STD);}ut_assertok(gpt_restore(mmc_dev_desc, str_disk_guid, parts,ARRAY_SIZE(parts)));+#define test(expected, part_str, whole) \
Can this be a function instead of a macro?
Not one-to-one because ut-asserteq returns on error. This could be changed to
ut_asserteq(-ENODEV, test("", true));but I think a macro is the simplest option.
Well you are using ut_asserteq() in the macro so I don't see why the macro is better than what you have above, with the code in a function? That is what we normally do.
I suppose. It doesn't really matter too much IMO, but I will change it for v3.
--Sean
--Sean
ut_asserteq(expected, \part_get_info_by_dev_and_name_or_num("mmc", part_str, \&mmc_dev_desc, \&part_info, whole))test(-ENODEV, "", true);env_set("bootdevice", "0");test(0, "", true);env_set("bootdevice", "1");test(1, "", false);test(1, "-", false);env_set("bootdevice", "");test(-EPROTONOSUPPORT, "0", false);test(0, "0", true);test(0, ":0", true);test(0, ".0", true);test(0, ".0:0", true);test(-EINVAL, "#test1", true);test(1, "1", false);test(1, "1", true);test(-ENOENT, "1:0", false);test(0, "1:0", true);test(1, "1:1", false);test(2, "1:2", false);test(1, "1.0", false);test(0, "1.0:0", true);test(1, "1.0:1", false);test(2, "1.0:2", false);test(-EINVAL, "1#bogus", false);test(1, "1#test1", false);test(2, "1#test2", false);return 0;+}
+DM_TEST(dm_test_part, UT_TESTF_SCAN_PDATA | UT_TESTF_SCAN_FDT);
2.25.1
Regards, Simon
On Mon, Apr 12, 2021 at 06:53:07PM -0400, Sean Anderson wrote:
This is technically a library function, but we use MMCs for testing, so it is easier to do it with DM. At the moment, the only block devices in sandbox are MMCs (AFAIK) so we just test with those.
Signed-off-by: Sean Anderson sean.anderson@seco.com
Applied to u-boot/master, thanks!
On Mon, Apr 12, 2021 at 06:53:05PM -0400, Sean Anderson wrote:
This allows using dev#partlabel syntax.
Signed-off-by: Sean Anderson sean.anderson@seco.com
Applied to u-boot/master, thanks!
On 4/13/21 12:53 AM, Sean Anderson wrote:
This allows using dev#partlabel syntax.
Signed-off-by: Sean Anderson sean.anderson@seco.com
Since this patch was merged as commit 7194527b the command 'ls host' leads to a segmentation violation:
=> host bind 0 ../sandbox.img => setenv bootdevice 0:1 => ls host
Segmentation violation pc = 0x55a5b66b7ec1, pc_reloc = 0x55a5a66b7ec1
Writing sandbox state
Before the patch:
=> host bind 0 ../sandbox.img => setenv bootdevice 0:1 => ls host 148 boot.scr Test/
1 file(s), 1 dir(s)
Please, provide a fix for your patch and a unit test using 'setenv bootdevice' to avoid future mischief.
Best regards
Heinrich
participants (4)
-
Heinrich Schuchardt -
Sean Anderson -
Simon Glass -
Tom Rini