diff mbox series

[libubootenv] Add possibility of partion names to config file

Message ID 20210622142552.16044-1-poeschel@lemonage.de
State Changes Requested
Headers show
Series [libubootenv] Add possibility of partion names to config file | expand

Commit Message

Lars Poeschel June 22, 2021, 2:25 p.m. UTC
With this one can specify a mtd device by it's partion name.

If you have multiple mtd devices, the problem is that different kernels
or even different boots of the same kernel may yield a different order
of mtd devices. So setting something like

/dev/mtd1        0x00000000      0x00010000

in your /etc/fw_env.config may fail if the kernel decided, that the mtd
device where you have your u-boot environment in is now /dev/mtd4
The solution is write mtd partition names to /etc/fw_env.config instead
of the device names. Like this:

PARTNAME=bootenv        0x00000000      0x00010000

What this patch does is:
* looks for the 'PARTNAME=' string
* takes rest of the field as a partion name
* looks info /proc/mtd if it can find a partition with the specified
  name

If it does not find the 'PARTNAME=' string it does continue like before.

Signed-off-by: Lars Pöschel <poeschel@lemonage.de>
---
 src/uboot_env.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 49 insertions(+), 1 deletion(-)

Comments

Stefano Babic June 23, 2021, 7:12 a.m. UTC | #1
Hi Lars,

On 22.06.21 16:25, Lars Pöschel wrote:
> With this one can specify a mtd device by it's partion name.
> 
> If you have multiple mtd devices, the problem is that different kernels
> or even different boots of the same kernel may yield a different order
> of mtd devices. So setting something like
> 
> /dev/mtd1        0x00000000      0x00010000
> 
> in your /etc/fw_env.config may fail if the kernel decided, that the mtd
> device where you have your u-boot environment in is now /dev/mtd4
> The solution is write mtd partition names to /etc/fw_env.config instead
> of the device names. Like this:
> 
> PARTNAME=bootenv        0x00000000      0x00010000
> 
> What this patch does is:
> * looks for the 'PARTNAME=' string
> * takes rest of the field as a partion name
> * looks info /proc/mtd if it can find a partition with the specified
>    name
> 
> If it does not find the 'PARTNAME=' string it does continue like before.
> 

Before going into details with review, I raise a couple of topics. I 
know that early or later this has happened. Up now, libubootenv is 
claimed to be compatible with the old tools, and this spared me to write 
a full documentation for fw_env.config. It was just like "check in 
U-Boot". But when new features and new syntax are added, then we cannot 
rely anymore to the old one and documentation should be added. A way 
could be to add a fw_gen.config to project, describing options and 
syntax. We can start from the one from U-Boot, reporting what is 
difference and their syntax.

The second point is that a partition name is not bound to MTD subsystem. 
What about if environment is put on a eMMC (or generic, on a block 
device) partition ? This should work, too (it cannot in your patch).


> Signed-off-by: Lars Pöschel <poeschel@lemonage.de>
> ---
>   src/uboot_env.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++-
>   1 file changed, 49 insertions(+), 1 deletion(-)
> 
> diff --git a/src/uboot_env.c b/src/uboot_env.c
> index 30c39eb..8e3b549 100644
> --- a/src/uboot_env.c
> +++ b/src/uboot_env.c
> @@ -44,6 +44,8 @@
>   #define DEVICE_UBI_NAME 		"/dev/ubi"
>   #define SYS_UBI_VOLUME_COUNT		"/sys/class/ubi/ubi%d/volumes_count"
>   #define SYS_UBI_VOLUME_NAME		"/sys/class/ubi/ubi%d/ubi%d_%d/name"
> +#define PARTION_NAME_IDENTIFIER		"PARTNAME="
> +#define DEVICE_MTD_NAME_TEMPLATE	"/dev/mtd%d"
>   
>   #define	LIST_FOREACH_SAFE(var, head, field, tvar)			\
>   	for ((var) = LIST_FIRST((head));				\
> @@ -342,6 +344,52 @@ static int normalize_device_path(char *path, struct uboot_flash_env *dev)
>   	return 0;
>   }
>   
> +static int translate_device_path(char *path, struct uboot_flash_env *dev)
> +{
> +	char *partname;
> +	FILE *fp;
> +	size_t bufsize = 0;
> +	char *line = NULL;
> +	int ret = 0;
> +	int ndev = -1;
> +	char *tmp, *appostrophe;
> +	int retval = -ENOENT;
> +
> +	partname = strstr(path, PARTION_NAME_IDENTIFIER);

PARTION ===> PARTITION

> +	if (partname == NULL)
> +		return normalize_device_path(path, dev);
> +
> +	partname += strlen(PARTION_NAME_IDENTIFIER);
> +
> +	fp = fopen("/proc/mtd", "r");
> +	if (!fp)
> +		return -EBADF;
> +
> +	while (getline(&line, &bufsize, fp) != -1) {
> +		ret = sscanf(line, "mtd%i: %*s %*s \"%ms\"", &ndev, &tmp);
> +		if ((!tmp) || (ret < 2))
> +			continue;
> +
> +		appostrophe = index(tmp, '"');
> +		*appostrophe = '\0';
> +		ret = strcmp(partname, tmp);
> +		free(tmp);
> +		if (ret != 0)
> +			continue;
> +
> +		ret = snprintf(line, bufsize, DEVICE_MTD_NAME_TEMPLATE, ndev);
> +		if (ret < 0)
> +			continue;
> +
> +		retval = normalize_device_path(line, dev);
> +		break;
> +	}
> +
> +	fclose(fp);
> +	free(line);
> +	return retval;
> +}
> +
>   static int check_env_device(struct uboot_ctx *ctx, struct uboot_flash_env *dev)
>   {
>   	int fd, ret;
> @@ -1241,7 +1289,7 @@ int libuboot_read_config(struct uboot_ctx *ctx, const char *config)
>   			ctx->size = dev->envsize;
>   
>   		if (tmp) {
> -			if (normalize_device_path(tmp, dev) < 0) {
> +			if (translate_device_path(tmp, dev) < 0) {
>   				free(tmp);
>   				retval = -EINVAL;
>   				break;
> 

Best regards,
Stefano Babic
Lars Poeschel July 5, 2021, 7:02 a.m. UTC | #2
Hi Stefano,

Sorry for the late reply. I am a bit busy at the moment.

On Wed, Jun 23, 2021 at 09:12:34AM +0200, Stefano Babic wrote:
> Hi Lars,
> 
> On 22.06.21 16:25, Lars Pöschel wrote:
> > With this one can specify a mtd device by it's partion name.
> > 
> > If you have multiple mtd devices, the problem is that different kernels
> > or even different boots of the same kernel may yield a different order
> > of mtd devices. So setting something like
> > 
> > /dev/mtd1        0x00000000      0x00010000
> > 
> > in your /etc/fw_env.config may fail if the kernel decided, that the mtd
> > device where you have your u-boot environment in is now /dev/mtd4
> > The solution is write mtd partition names to /etc/fw_env.config instead
> > of the device names. Like this:
> > 
> > PARTNAME=bootenv        0x00000000      0x00010000
> > 
> > What this patch does is:
> > * looks for the 'PARTNAME=' string
> > * takes rest of the field as a partion name
> > * looks info /proc/mtd if it can find a partition with the specified
> >    name
> > 
> > If it does not find the 'PARTNAME=' string it does continue like before.
> > 
> 
> Before going into details with review, I raise a couple of topics. I know
> that early or later this has happened. Up now, libubootenv is claimed to be
> compatible with the old tools, and this spared me to write a full
> documentation for fw_env.config. It was just like "check in U-Boot". But
> when new features and new syntax are added, then we cannot rely anymore to
> the old one and documentation should be added. A way could be to add a
> fw_gen.config to project, describing options and syntax. We can start from
> the one from U-Boot, reporting what is difference and their syntax.

Ok, adding some documentation makes sense. I agree on this.

> The second point is that a partition name is not bound to MTD subsystem.
> What about if environment is put on a eMMC (or generic, on a block device)
> partition ? This should work, too (it cannot in your patch).

Although I think this could also handled in another patch later, let's
talk about it.
Do you have something in mind already how to do it ? How to get the
partion names of block devices from kernel ?

Thank you so far!

Regards,
Lars
Stefano Babic July 5, 2021, 10:37 a.m. UTC | #3
Hallo Lars,

On 05.07.21 09:02, Lars Poeschel wrote:
> Hi Stefano,
> 
> Sorry for the late reply. I am a bit busy at the moment.
> 
> On Wed, Jun 23, 2021 at 09:12:34AM +0200, Stefano Babic wrote:
>> Hi Lars,
>>
>> On 22.06.21 16:25, Lars Pöschel wrote:
>>> With this one can specify a mtd device by it's partion name.
>>>
>>> If you have multiple mtd devices, the problem is that different kernels
>>> or even different boots of the same kernel may yield a different order
>>> of mtd devices. So setting something like
>>>
>>> /dev/mtd1        0x00000000      0x00010000
>>>
>>> in your /etc/fw_env.config may fail if the kernel decided, that the mtd
>>> device where you have your u-boot environment in is now /dev/mtd4
>>> The solution is write mtd partition names to /etc/fw_env.config instead
>>> of the device names. Like this:
>>>
>>> PARTNAME=bootenv        0x00000000      0x00010000
>>>
>>> What this patch does is:
>>> * looks for the 'PARTNAME=' string
>>> * takes rest of the field as a partion name
>>> * looks info /proc/mtd if it can find a partition with the specified
>>>     name
>>>
>>> If it does not find the 'PARTNAME=' string it does continue like before.
>>>
>>
>> Before going into details with review, I raise a couple of topics. I know
>> that early or later this has happened. Up now, libubootenv is claimed to be
>> compatible with the old tools, and this spared me to write a full
>> documentation for fw_env.config. It was just like "check in U-Boot". But
>> when new features and new syntax are added, then we cannot rely anymore to
>> the old one and documentation should be added. A way could be to add a
>> fw_gen.config to project, describing options and syntax. We can start from
>> the one from U-Boot, reporting what is difference and their syntax.
> 
> Ok, adding some documentation makes sense. I agree on this.
> 
>> The second point is that a partition name is not bound to MTD subsystem.
>> What about if environment is put on a eMMC (or generic, on a block device)
>> partition ? This should work, too (it cannot in your patch).
> 
> Although I think this could also handled in another patch later, let's
> talk about it.

 From my experience (as maintainer for U-Boot, too) when it was agree to 
do in a follow up patch, nothing happens.

> Do you have something in mind already how to do it ? How to get the
> partion names of block devices from kernel ?

I will suggest you use libblkid, I think you can use 
blkid_dev_set_search() passing "LABEL" as type.

Best regards,
Stefano Babic
diff mbox series

Patch

diff --git a/src/uboot_env.c b/src/uboot_env.c
index 30c39eb..8e3b549 100644
--- a/src/uboot_env.c
+++ b/src/uboot_env.c
@@ -44,6 +44,8 @@ 
 #define DEVICE_UBI_NAME 		"/dev/ubi"
 #define SYS_UBI_VOLUME_COUNT		"/sys/class/ubi/ubi%d/volumes_count"
 #define SYS_UBI_VOLUME_NAME		"/sys/class/ubi/ubi%d/ubi%d_%d/name"
+#define PARTION_NAME_IDENTIFIER		"PARTNAME="
+#define DEVICE_MTD_NAME_TEMPLATE	"/dev/mtd%d"
 
 #define	LIST_FOREACH_SAFE(var, head, field, tvar)			\
 	for ((var) = LIST_FIRST((head));				\
@@ -342,6 +344,52 @@  static int normalize_device_path(char *path, struct uboot_flash_env *dev)
 	return 0;
 }
 
+static int translate_device_path(char *path, struct uboot_flash_env *dev)
+{
+	char *partname;
+	FILE *fp;
+	size_t bufsize = 0;
+	char *line = NULL;
+	int ret = 0;
+	int ndev = -1;
+	char *tmp, *appostrophe;
+	int retval = -ENOENT;
+
+	partname = strstr(path, PARTION_NAME_IDENTIFIER);
+	if (partname == NULL)
+		return normalize_device_path(path, dev);
+
+	partname += strlen(PARTION_NAME_IDENTIFIER);
+
+	fp = fopen("/proc/mtd", "r");
+	if (!fp)
+		return -EBADF;
+
+	while (getline(&line, &bufsize, fp) != -1) {
+		ret = sscanf(line, "mtd%i: %*s %*s \"%ms\"", &ndev, &tmp);
+		if ((!tmp) || (ret < 2))
+			continue;
+
+		appostrophe = index(tmp, '"');
+		*appostrophe = '\0';
+		ret = strcmp(partname, tmp);
+		free(tmp);
+		if (ret != 0)
+			continue;
+
+		ret = snprintf(line, bufsize, DEVICE_MTD_NAME_TEMPLATE, ndev);
+		if (ret < 0)
+			continue;
+
+		retval = normalize_device_path(line, dev);
+		break;
+	}
+
+	fclose(fp);
+	free(line);
+	return retval;
+}
+
 static int check_env_device(struct uboot_ctx *ctx, struct uboot_flash_env *dev)
 {
 	int fd, ret;
@@ -1241,7 +1289,7 @@  int libuboot_read_config(struct uboot_ctx *ctx, const char *config)
 			ctx->size = dev->envsize;
 
 		if (tmp) {
-			if (normalize_device_path(tmp, dev) < 0) {
+			if (translate_device_path(tmp, dev) < 0) {
 				free(tmp);
 				retval = -EINVAL;
 				break;