[U-Boot] [PATCH v2 0/3] common: cmd_part: start and size sub-commands introduction

[U-Boot] [PATCH v2 0/3] common: cmd_part: start and size sub-commands introduction

[Paul Kocialkowski]

Changes since v1:
* Fixed build errors related to snprintf

[U-Boot] [PATCH v2 1/3] common: cmd_part: Proper alignment

[Paul Kocialkowski]

This fixes a misaligned declaration.

Signed-off-by: Paul Kocialkowski <contact@paulk.fr>
---
 common/cmd_part.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/common/cmd_part.c b/common/cmd_part.c
index 8483c12..4bdbf90 100644
--- a/common/cmd_part.c
+++ b/common/cmd_part.c
@@ -88,7 +88,7 @@ static int do_part_list(int argc, char * const argv[])
 	if (var != NULL) {
 		int p;
 		char str[512] = { '\0', };
-	  disk_partition_t info;
+		disk_partition_t info;
 
 		for (p = 1; p < 128; p++) {
 			char t[5];
-- 
1.9.1

[U-Boot] [PATCH v2 2/3] common: cmd_part: start and size sub-commands introduction

[Paul Kocialkowski]

This introduces the part start and part size sub-commands. The purpose of these
is to store the start block and size of a partition in a variable, given the
device and partition number.

This allows reading raw data that fits a single partition more easily.
For instance, this could be used to figure out the start block and size of a
kernel partition when a partition table is present, given the partition number.

Signed-off-by: Paul Kocialkowski <contact@paulk.fr>
---
 common/cmd_part.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 65 insertions(+), 1 deletion(-)

diff --git a/common/cmd_part.c b/common/cmd_part.c
index 4bdbf90..7212ba6 100644
--- a/common/cmd_part.c
+++ b/common/cmd_part.c
@@ -112,6 +112,62 @@ static int do_part_list(int argc, char * const argv[])
 	return 0;
 }
 
+static int do_part_start(int argc, char * const argv[])
+{
+	block_dev_desc_t *desc;
+	disk_partition_t info;
+	char buf[512] = { 0 };
+	int part;
+	int err;
+	int ret;
+
+	if (argc < 4)
+		return CMD_RET_USAGE;
+
+	part = simple_strtoul(argv[2], NULL, 0);
+
+	ret = get_device(argv[0], argv[1], &desc);
+	if (ret < 0)
+		return 1;
+
+	err = get_partition_info(desc, part, &info);
+	if (err)
+		return 1;
+
+	snprintf(buf, sizeof(buf), "0x%lx", info.start);
+	setenv(argv[3], buf);
+
+	return 0;
+}
+
+static int do_part_size(int argc, char * const argv[])
+{
+	block_dev_desc_t *desc;
+	disk_partition_t info;
+	char buf[512] = { 0 };
+	int part;
+	int err;
+	int ret;
+
+	if (argc < 4)
+		return CMD_RET_USAGE;
+
+	part = simple_strtoul(argv[2], NULL, 0);
+
+	ret = get_device(argv[0], argv[1], &desc);
+	if (ret < 0)
+		return 1;
+
+	err = get_partition_info(desc, part, &info);
+	if (err)
+		return 1;
+
+	snprintf(buf, sizeof(buf), "0x%lx", info.size);
+	setenv(argv[3], buf);
+
+	return 0;
+}
+
 static int do_part(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 {
 	if (argc < 2)
@@ -121,6 +177,10 @@ static int do_part(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 		return do_part_uuid(argc - 2, argv + 2);
 	else if (!strcmp(argv[1], "list"))
 		return do_part_list(argc - 2, argv + 2);
+	else if (!strcmp(argv[1], "start"))
+		return do_part_start(argc - 2, argv + 2);
+	else if (!strcmp(argv[1], "size"))
+		return do_part_size(argc - 2, argv + 2);
 
 	return CMD_RET_USAGE;
 }
@@ -136,5 +196,9 @@ U_BOOT_CMD(
 	"    - print a device's partition table\n"
 	"part list <interface> <dev> [flags] <varname>\n"
 	"    - set environment variable to the list of partitions\n"
-	"      flags can be -bootable (list only bootable partitions)"
+	"      flags can be -bootable (list only bootable partitions)\n"
+	"part start <interface> <dev> <part> <varname>\n"
+	"    - set environment variable to the start of the partition (in blocks)\n"
+	"part size <interface> <dev> <part> <varname>\n"
+	"    - set environment variable to the size of the partition (in blocks)"
 );
-- 
1.9.1

[U-Boot] [PATCH v2 2/3] common: cmd_part: start and size sub-commands introduction

[Stephen Warren]

On 06/13/2015 02:38 AM, Paul Kocialkowski wrote:
> This introduces the part start and part size sub-commands. The purpose of these
> is to store the start block and size of a partition in a variable, given the
> device and partition number.
>
> This allows reading raw data that fits a single partition more easily.
> For instance, this could be used to figure out the start block and size of a
> kernel partition when a partition table is present, given the partition number.

Patches 1 and 2,
Acked-by: Stephen Warren <swarren@nvidia.com>

One minor comment here though:

> diff --git a/common/cmd_part.c b/common/cmd_part.c

> +static int do_part_start(int argc, char * const argv[])

> +	if (argc < 4)
> +		return CMD_RET_USAGE;
...
> +	snprintf(buf, sizeof(buf), "0x%lx", info.start);
> +	setenv(argv[3], buf);

It would be nice if the variable name parameter to this command (and 
part size) were optional, just like it is in "part uuid". This would 
make it simpler to run the command interactively while experimenting.

[U-Boot] [PATCH v2 2/3] common: cmd_part: start and size sub-commands introduction

[Tom Rini]

On Mon, Jun 15, 2015 at 08:59:47AM -0600, Stephen Warren wrote:
> On 06/13/2015 02:38 AM, Paul Kocialkowski wrote:
> >This introduces the part start and part size sub-commands. The purpose of these
> >is to store the start block and size of a partition in a variable, given the
> >device and partition number.
> >
> >This allows reading raw data that fits a single partition more easily.
> >For instance, this could be used to figure out the start block and size of a
> >kernel partition when a partition table is present, given the partition number.
> 
> Patches 1 and 2,
> Acked-by: Stephen Warren <swarren@nvidia.com>
> 
> One minor comment here though:
> 
> >diff --git a/common/cmd_part.c b/common/cmd_part.c
> 
> >+static int do_part_start(int argc, char * const argv[])
> 
> >+	if (argc < 4)
> >+		return CMD_RET_USAGE;
> ...
> >+	snprintf(buf, sizeof(buf), "0x%lx", info.start);
> >+	setenv(argv[3], buf);
> 
> It would be nice if the variable name parameter to this command (and
> part size) were optional, just like it is in "part uuid". This would
> make it simpler to run the command interactively while
> experimenting.

Yes.  Also numbers are hex by default in U-Boot so we don't need the 0x
here.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20150615/1fe38832/attachment.sig>

[U-Boot] [PATCH v2 2/3] common: cmd_part: start and size sub-commands introduction

[Paul Kocialkowski]

Le lundi 15 juin 2015 ? 12:02 -0400, Tom Rini a ?crit :
> On Mon, Jun 15, 2015 at 08:59:47AM -0600, Stephen Warren wrote:
> > On 06/13/2015 02:38 AM, Paul Kocialkowski wrote:
> > >This introduces the part start and part size sub-commands. The purpose of these
> > >is to store the start block and size of a partition in a variable, given the
> > >device and partition number.
> > >
> > >This allows reading raw data that fits a single partition more easily.
> > >For instance, this could be used to figure out the start block and size of a
> > >kernel partition when a partition table is present, given the partition number.
> > 
> > Patches 1 and 2,
> > Acked-by: Stephen Warren <swarren@nvidia.com>
> > 
> > One minor comment here though:
> > 
> > >diff --git a/common/cmd_part.c b/common/cmd_part.c
> > 
> > >+static int do_part_start(int argc, char * const argv[])
> > 
> > >+	if (argc < 4)
> > >+		return CMD_RET_USAGE;
> > ...
> > >+	snprintf(buf, sizeof(buf), "0x%lx", info.start);
> > >+	setenv(argv[3], buf);
> > 
> > It would be nice if the variable name parameter to this command (and
> > part size) were optional, just like it is in "part uuid". This would
> > make it simpler to run the command interactively while
> > experimenting.
> 
> Yes.  Also numbers are hex by default in U-Boot so we don't need the 0x
> here.

Thanks for the reminder. v3 doesn't include the 0x.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20150615/f0a1a61b/attachment.sig>

[U-Boot] [PATCH v2 2/3] common: cmd_part: start and size sub-commands introduction

[Paul Kocialkowski]

Le lundi 15 juin 2015 ? 08:59 -0600, Stephen Warren a ?crit :
> On 06/13/2015 02:38 AM, Paul Kocialkowski wrote:
> > This introduces the part start and part size sub-commands. The purpose of these
> > is to store the start block and size of a partition in a variable, given the
> > device and partition number.
> >
> > This allows reading raw data that fits a single partition more easily.
> > For instance, this could be used to figure out the start block and size of a
> > kernel partition when a partition table is present, given the partition number.
> 
> Patches 1 and 2,
> Acked-by: Stephen Warren <swarren@nvidia.com>
> 
> One minor comment here though:
> 
> > diff --git a/common/cmd_part.c b/common/cmd_part.c
> 
> > +static int do_part_start(int argc, char * const argv[])
> 
> > +	if (argc < 4)
> > +		return CMD_RET_USAGE;
> ...
> > +	snprintf(buf, sizeof(buf), "0x%lx", info.start);
> > +	setenv(argv[3], buf);
> 
> It would be nice if the variable name parameter to this command (and 
> part size) were optional, just like it is in "part uuid". This would 
> make it simpler to run the command interactively while experimenting.

Sure, that wouldn't hurt!

Thanks for the review.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20150615/db4289fa/attachment.sig>

[U-Boot] [PATCH v2 3/3] common: cmd_part: Error prints on failures

[Paul Kocialkowski]

When a failure occurs when selecting the device or partition, the user should be
notified through an error print.

Signed-off-by: Paul Kocialkowski <contact@paulk.fr>
---
 common/cmd_part.c | 24 ++++++++++++++++++------
 1 file changed, 18 insertions(+), 6 deletions(-)

diff --git a/common/cmd_part.c b/common/cmd_part.c
index 7212ba6..ff9bc07 100644
--- a/common/cmd_part.c
+++ b/common/cmd_part.c
@@ -38,8 +38,10 @@ static int do_part_uuid(int argc, char * const argv[])
 		return CMD_RET_USAGE;
 
 	part = get_device_and_partition(argv[0], argv[1], &dev_desc, &info, 0);
-	if (part < 0)
+	if (part < 0) {
+		error("Invalid device and/or partition\n");
 		return 1;
+	}
 
 	if (argc > 2)
 		setenv(argv[2], info.uuid);
@@ -82,8 +84,10 @@ static int do_part_list(int argc, char * const argv[])
 	}
 
 	ret = get_device(argv[0], argv[1], &desc);
-	if (ret < 0)
+	if (ret < 0) {
+		error("Invalid device\n");
 		return 1;
+	}
 
 	if (var != NULL) {
 		int p;
@@ -127,12 +131,16 @@ static int do_part_start(int argc, char * const argv[])
 	part = simple_strtoul(argv[2], NULL, 0);
 
 	ret = get_device(argv[0], argv[1], &desc);
-	if (ret < 0)
+	if (ret < 0) {
+		error("Invalid device\n");
 		return 1;
+	}
 
 	err = get_partition_info(desc, part, &info);
-	if (err)
+	if (err) {
+		error("Invalid partition number\n");
 		return 1;
+	}
 
 	snprintf(buf, sizeof(buf), "0x%lx", info.start);
 	setenv(argv[3], buf);
@@ -155,12 +163,16 @@ static int do_part_size(int argc, char * const argv[])
 	part = simple_strtoul(argv[2], NULL, 0);
 
 	ret = get_device(argv[0], argv[1], &desc);
-	if (ret < 0)
+	if (ret < 0) {
+		error("Invalid device\n");
 		return 1;
+	}
 
 	err = get_partition_info(desc, part, &info);
-	if (err)
+	if (err) {
+		error("Invalid partition number\n");
 		return 1;
+	}
 
 	snprintf(buf, sizeof(buf), "0x%lx", info.size);
 	setenv(argv[3], buf);
-- 
1.9.1

[U-Boot] [PATCH v2 3/3] common: cmd_part: Error prints on failures

[Stephen Warren]

On 06/13/2015 02:38 AM, Paul Kocialkowski wrote:
> When a failure occurs when selecting the device or partition, the user should be
> notified through an error print.

> diff --git a/common/cmd_part.c b/common/cmd_part.c

> @@ -38,8 +38,10 @@ static int do_part_uuid(int argc, char * const argv[])
>   		return CMD_RET_USAGE;
>
>   	part = get_device_and_partition(argv[0], argv[1], &dev_desc, &info, 0);
> -	if (part < 0)
> +	if (part < 0) {
> +		error("Invalid device and/or partition\n");

A very quick look at the implementation of get_device_and_partition() 
(and all the other relevant functions for this patch) implies the 
implementation already prints an error message. If you found a case 
where that isn't true, I think those functions should be fixed, not all 
their callers.

[U-Boot] [PATCH v2 3/3] common: cmd_part: Error prints on failures

[Paul Kocialkowski]

Le lundi 15 juin 2015 ? 09:00 -0600, Stephen Warren a ?crit :
> On 06/13/2015 02:38 AM, Paul Kocialkowski wrote:
> > When a failure occurs when selecting the device or partition, the user should be
> > notified through an error print.
> 
> > diff --git a/common/cmd_part.c b/common/cmd_part.c
> 
> > @@ -38,8 +38,10 @@ static int do_part_uuid(int argc, char * const argv[])
> >   		return CMD_RET_USAGE;
> >
> >   	part = get_device_and_partition(argv[0], argv[1], &dev_desc, &info, 0);
> > -	if (part < 0)
> > +	if (part < 0) {
> > +		error("Invalid device and/or partition\n");
> 
> A very quick look at the implementation of get_device_and_partition() 
> (and all the other relevant functions for this patch) implies the 
> implementation already prints an error message. If you found a case 
> where that isn't true, I think those functions should be fixed, not all 
> their callers.

You're right, I'll just drop that patch from the series.

Thanks!

-- 
Paul Kocialkowski, Replicant developer

Replicant is a fully free Android distribution running on several
devices, a free software mobile operating system putting the emphasis on
freedom and privacy/security.

Website: http://www.replicant.us/
Blog: http://blog.replicant.us/
Wiki/tracker/forums: http://redmine.replicant.us/
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20150615/ead80138/attachment.sig>