[PATCH 0/3] introduce command abi version

[PATCH 0/3] introduce command abi version

[Jean-Christophe PLAGNIOL-VILLARD]

Hi,

	This will allow to detect command incompability between different
	version of barebox

The following changes since commit a6f852d401b63e1de0efe73a11efd5403870b594:

  Merge branch 'pu/compiler-warnings' (2012-10-21 10:57:54 +0200)

are available in the git repository at:


  git://git.jcrosoft.org/barebox.git delivery/command_abi_version

for you to fetch changes up to 5ee2adb18c6e0e609e5633de4d4c6d9383b14173:

  environment: detect command_abi_version (2012-10-22 14:34:25 +0800)

----------------------------------------------------------------
Jean-Christophe PLAGNIOL-VILLARD (3):
      command: introduce abi version
      envfs: add command_abi_version support
      environment: detect command_abi_version

 commands/loadenv.c    |   11 ++++++++++-
 common/command.c      |    2 ++
 common/environment.c  |    7 ++++++-
 common/startup.c      |   42 ++++++++++++++++++++++++++++++++----------
 include/command.h     |    1 +
 include/command_abi.h |   12 ++++++++++++
 include/envfs.h       |    4 ++--
 include/environment.h |   13 ++++++++++++-
 scripts/bareboxenv.c  |    9 ++++++++-
 9 files changed, 85 insertions(+), 16 deletions(-)
 create mode 100644 include/command_abi.h

Best Regards,
J.

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 1/3] command: introduce abi version

[Jean-Christophe PLAGNIOL-VILLARD]

This will allow to detect incompatibility between the env abi and the current
barebox one

Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
---
 common/command.c      |    2 ++
 include/command.h     |    1 +
 include/command_abi.h |   12 ++++++++++++
 3 files changed, 15 insertions(+)
 create mode 100644 include/command_abi.h

diff --git a/common/command.c b/common/command.c
index 873b3ff..c80538c 100644
--- a/common/command.c
+++ b/common/command.c
@@ -154,6 +154,8 @@ static int init_command_list(void)
 {
 	struct command *cmdtp;
 
+	export_env_ull("command_abi_version", COMMAND_ABI_VERSION);
+
 	for (cmdtp = &__barebox_cmd_start;
 			cmdtp != &__barebox_cmd_end;
 			cmdtp++)
diff --git a/include/command.h b/include/command.h
index ffc722c..c49a014 100644
--- a/include/command.h
+++ b/include/command.h
@@ -25,6 +25,7 @@
 
 #include <linux/list.h>
 #include <linux/stringify.h>
+#include <command_abi.h>
 
 #ifndef NULL
 #define NULL	0
diff --git a/include/command_abi.h b/include/command_abi.h
new file mode 100644
index 0000000..264c003
--- /dev/null
+++ b/include/command_abi.h
@@ -0,0 +1,12 @@
+/*
+ * (C) Copyright 2012 Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
+ *
+ * Under GPL v2
+ */
+
+#ifndef __COMMAND_ABI_H__
+#define __COMMAND_ABI_H__
+
+#define COMMAND_ABI_VERSION	0
+
+#endif /* __COMMAND_ABI_H__ */
-- 
1.7.10.4


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 2/3] envfs: add command_abi_version support

[Jean-Christophe PLAGNIOL-VILLARD]

allow to store the command abi version
This will allow to detect incompatibility

Increase envfs minor to 1

Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
---
 commands/loadenv.c    |   11 ++++++++++-
 common/environment.c  |    7 ++++++-
 common/startup.c      |    4 ++--
 include/envfs.h       |    4 ++--
 include/environment.h |   13 ++++++++++++-
 scripts/bareboxenv.c  |    9 ++++++++-
 6 files changed, 40 insertions(+), 8 deletions(-)

diff --git a/commands/loadenv.c b/commands/loadenv.c
index 5bf1740..c52ee4b 100644
--- a/commands/loadenv.c
+++ b/commands/loadenv.c
@@ -26,6 +26,9 @@
 
 static int do_loadenv(int argc, char *argv[])
 {
+	int ret;
+	uint16_t command_abi_version;
+
 	char *filename, *dirname;
 
 	if (argc < 3)
@@ -37,7 +40,13 @@ static int do_loadenv(int argc, char *argv[])
 	else
 		filename = argv[1];
 	printf("loading environment from %s\n", filename);
-	return envfs_load(filename, dirname);
+	ret = envfs_load(filename, dirname, &command_abi_version);
+
+	if (command_abi_version < COMMAND_ABI_VERSION)
+		pr_warn("environment command_abi_verison (%u) < current version (%u)\n",
+			command_abi_version, COMMAND_ABI_VERSION);
+
+	return ret;
 }
 
 BAREBOX_CMD_HELP_START(loadenv)
diff --git a/common/environment.c b/common/environment.c
index 69c4c0a..e0207e9 100644
--- a/common/environment.c
+++ b/common/environment.c
@@ -162,6 +162,7 @@ int envfs_save(char *filename, char *dirname)
 
 	super = (struct envfs_super *)buf;
 	super->magic = ENVFS_32(ENVFS_MAGIC);
+	super->command_abi_version = ENVFS_16(COMMAND_ABI_VERSION);
 	super->major = ENVFS_MAJOR;
 	super->minor = ENVFS_MINOR;
 	super->size = ENVFS_32(size);
@@ -206,7 +207,7 @@ EXPORT_SYMBOL(envfs_save);
  * Note: This function will also be used on the host! See note in the header
  * of this file.
  */
-int envfs_load(char *filename, char *dir)
+int envfs_load(char *filename, char *dir, uint16_t *command_abi_version)
 {
 	struct envfs_super super;
 	void *buf = NULL, *buf_free = NULL;
@@ -235,6 +236,7 @@ int envfs_load(char *filename, char *dir)
 		goto out;
 	}
 
+
 	if ( ENVFS_32(super.magic) != ENVFS_MAGIC) {
 		printf("envfs: wrong magic on %s\n", filename);
 		ret = -EIO;
@@ -248,6 +250,9 @@ int envfs_load(char *filename, char *dir)
 		goto out;
 	}
 
+	if (command_abi_version)
+		*command_abi_version = ENVFS_16(super.command_abi_version);
+
 	size = ENVFS_32(super.size);
 	buf = xmalloc(size);
 	buf_free = buf;
diff --git a/common/startup.c b/common/startup.c
index 78926c9..affb5a9 100644
--- a/common/startup.c
+++ b/common/startup.c
@@ -106,12 +106,12 @@ void start_barebox (void)
 	debug("initcalls done\n");
 
 #ifdef CONFIG_ENV_HANDLING
-	if (envfs_load(default_environment_path, "/env")) {
+	if (envfs_load(default_environment_path, "/env", NULL)) {
 #ifdef CONFIG_DEFAULT_ENVIRONMENT
 		printf("no valid environment found on %s. "
 			"Using default environment\n",
 			default_environment_path);
-		envfs_load("/dev/defaultenv", "/env");
+		envfs_load("/dev/defaultenv", "/env", NULL);
 #endif
 	}
 #endif
diff --git a/include/envfs.h b/include/envfs.h
index 3d14fcb..6503d22 100644
--- a/include/envfs.h
+++ b/include/envfs.h
@@ -6,7 +6,7 @@
 #endif
 
 #define ENVFS_MAJOR		1
-#define ENVFS_MINOR		0
+#define ENVFS_MINOR		1
 
 #define ENVFS_MAGIC		    0x798fba79	/* some random number */
 #define ENVFS_INODE_MAGIC	0x67a8c78d
@@ -40,7 +40,7 @@ struct envfs_super {
 	uint32_t size;			/* size of data */
 	uint8_t major;			/* major */
 	uint8_t minor;			/* minor */
-	uint16_t future;		/* reserved for future use */
+	uint16_t command_abi_version;	/* command abi version */
 	uint32_t flags;			/* feature flags */
 	uint32_t sb_crc;		/* crc for the superblock */
 };
diff --git a/include/environment.h b/include/environment.h
index 95e75e7..64de26f 100644
--- a/include/environment.h
+++ b/include/environment.h
@@ -74,8 +74,19 @@ int env_push_context(void);
 /* defaults to /dev/env0 */
 extern char *default_environment_path;
 
-int envfs_load(char *filename, char *dirname);
+#ifdef CONFIG_ENV_HANDLING
+int envfs_load(char *filename, char *dir, uint16_t *command_abi_version);
 int envfs_save(char *filename, char *dirname);
+#else
+static inline int envfs_load(char *filename, char *dir, uint16_t *command_abi_version)
+{
+	return -EINVAL;
+}
+static inline int envfs_save(char *filename, char *dirname)
+{
+	return -EINVAL;
+}
+#endif
 
 int export(const char *);
 
diff --git a/scripts/bareboxenv.c b/scripts/bareboxenv.c
index f44a1f8..6bd23e8 100644
--- a/scripts/bareboxenv.c
+++ b/scripts/bareboxenv.c
@@ -115,6 +115,7 @@ char *concat_subpath_file(const char *path, const char *f)
 }
 
 #include "../lib/recursive_action.c"
+#include "../include/command_abi.h"
 #include "../include/envfs.h"
 #include "../crypto/crc32.c"
 #include "../lib/make_directory.c"
@@ -189,14 +190,20 @@ int main(int argc, char *argv[])
 	}
 
 	if (load) {
+		uint16_t command_abi_version;
+
 		if (verbose)
 			printf("loading env from file %s to %s\n", filename, dirname);
-		envfs_load(filename, dirname);
+		envfs_load(filename, dirname, &command_abi_version);
+		if (verbose)
+			printf("with command_abi_version = %u\n", command_abi_version);
 	}
 	if (save) {
 		if (verbose)
 			printf("saving contents of %s to file %s\n", dirname, filename);
 		envfs_save(filename, dirname);
+		if (verbose)
+			printf("with command_abi_version = %u\n", COMMAND_ABI_VERSION);
 	}
 	exit(0);
 }
-- 
1.7.10.4


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 3/3] environment: detect command_abi_version

[Jean-Christophe PLAGNIOL-VILLARD]

Detect the environment command_abi_version and if not compatible swith to the
defaultenv. The user will still able to force the laod via loadenv command.

Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
---
 common/startup.c |   42 ++++++++++++++++++++++++++++++++----------
 1 file changed, 32 insertions(+), 10 deletions(-)

diff --git a/common/startup.c b/common/startup.c
index affb5a9..df6edfa 100644
--- a/common/startup.c
+++ b/common/startup.c
@@ -88,6 +88,36 @@ static int mount_root(void)
 fs_initcall(mount_root);
 #endif
 
+static void env_handling(void)
+{
+	uint16_t command_abi_version;
+
+	if (!IS_ENABLED(CONFIG_ENV_HANDLING))
+		return;
+
+	if (envfs_load(default_environment_path, "/env", &command_abi_version)) {
+		if (!IS_ENABLED(CONFIG_DEFAULT_ENVIRONMENT))
+			return;
+
+		printf("no valid environment found on %s\n",
+			default_environment_path);
+		goto load_defaultenv;
+	}
+
+	if (command_abi_version < COMMAND_ABI_VERSION) {
+		printf("environment found on %s have command_abi_version (%u) < current version (%u)\n",
+			default_environment_path,
+			command_abi_version, COMMAND_ABI_VERSION);
+		goto load_defaultenv;
+	}
+
+	return;
+
+load_defaultenv:
+	printf("Using default environment\n");
+	envfs_load("/dev/defaultenv", "/env", NULL);
+}
+
 void start_barebox (void)
 {
 	initcall_t *initcall;
@@ -105,16 +135,8 @@ void start_barebox (void)
 
 	debug("initcalls done\n");
 
-#ifdef CONFIG_ENV_HANDLING
-	if (envfs_load(default_environment_path, "/env", NULL)) {
-#ifdef CONFIG_DEFAULT_ENVIRONMENT
-		printf("no valid environment found on %s. "
-			"Using default environment\n",
-			default_environment_path);
-		envfs_load("/dev/defaultenv", "/env", NULL);
-#endif
-	}
-#endif
+	env_handling();
+
 #ifdef CONFIG_COMMAND_SUPPORT
 	printf("running /env/bin/init...\n");
 
-- 
1.7.10.4


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 0/3] introduce command abi version

[Sascha Hauer]

On Mon, Oct 22, 2012 at 06:03:41PM +0200, Jean-Christophe PLAGNIOL-VILLARD wrote:
> Hi,
> 
> 	This will allow to detect command incompability between different
> 	version of barebox

Generally a good move. I want to it some more time to apply it though
as this is a significant change.
Nevertheless some comments inline.

Sascha


-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 2/3] envfs: add command_abi_version support

[Sascha Hauer]

On Mon, Oct 22, 2012 at 06:07:27PM +0200, Jean-Christophe PLAGNIOL-VILLARD wrote:
> allow to store the command abi version
> This will allow to detect incompatibility
> 
> Increase envfs minor to 1
> 
> Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
> ---
>  commands/loadenv.c    |   11 ++++++++++-
>  common/environment.c  |    7 ++++++-
>  common/startup.c      |    4 ++--
>  include/envfs.h       |    4 ++--
>  include/environment.h |   13 ++++++++++++-
>  scripts/bareboxenv.c  |    9 ++++++++-
>  6 files changed, 40 insertions(+), 8 deletions(-)
> 
> diff --git a/commands/loadenv.c b/commands/loadenv.c
> index 5bf1740..c52ee4b 100644
> --- a/commands/loadenv.c
> +++ b/commands/loadenv.c
> @@ -26,6 +26,9 @@
>  
>  static int do_loadenv(int argc, char *argv[])
>  {
> +	int ret;
> +	uint16_t command_abi_version;
> +
>  	char *filename, *dirname;
>  
>  	if (argc < 3)
> @@ -37,7 +40,13 @@ static int do_loadenv(int argc, char *argv[])
>  	else
>  		filename = argv[1];
>  	printf("loading environment from %s\n", filename);
> -	return envfs_load(filename, dirname);
> +	ret = envfs_load(filename, dirname, &command_abi_version);
> +
> +	if (command_abi_version < COMMAND_ABI_VERSION)

'!=' instead of '<'? The incompatibility is often in both directions.

We may have to introduce min/max, I'm not sure on that.


> -int envfs_load(char *filename, char *dir)
> +int envfs_load(char *filename, char *dir, uint16_t *command_abi_version)
>  {
>  	struct envfs_super super;
>  	void *buf = NULL, *buf_free = NULL;
> @@ -235,6 +236,7 @@ int envfs_load(char *filename, char *dir)
>  		goto out;
>  	}
>  
> +
>  	if ( ENVFS_32(super.magic) != ENVFS_MAGIC) {
>  		printf("envfs: wrong magic on %s\n", filename);
>  		ret = -EIO;

Drop this hunk.

> diff --git a/scripts/bareboxenv.c b/scripts/bareboxenv.c
> index f44a1f8..6bd23e8 100644
> --- a/scripts/bareboxenv.c
> +++ b/scripts/bareboxenv.c
> @@ -115,6 +115,7 @@ char *concat_subpath_file(const char *path, const char *f)
>  }
>  
>  #include "../lib/recursive_action.c"
> +#include "../include/command_abi.h"
>  #include "../include/envfs.h"
>  #include "../crypto/crc32.c"
>  #include "../lib/make_directory.c"
> @@ -189,14 +190,20 @@ int main(int argc, char *argv[])
>  	}
>  
>  	if (load) {
> +		uint16_t command_abi_version;
> +
>  		if (verbose)
>  			printf("loading env from file %s to %s\n", filename, dirname);
> -		envfs_load(filename, dirname);
> +		envfs_load(filename, dirname, &command_abi_version);
> +		if (verbose)
> +			printf("with command_abi_version = %u\n", command_abi_version);
>  	}
>  	if (save) {
>  		if (verbose)
>  			printf("saving contents of %s to file %s\n", dirname, filename);
>  		envfs_save(filename, dirname);
> +		if (verbose)
> +			printf("with command_abi_version = %u\n", COMMAND_ABI_VERSION);
>  	}

For testing purposes or for generating an environment for another
barebox version it would be useful to be able to specify the ABI version
from the command line.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 2/3] envfs: add command_abi_version support

[Jean-Christophe PLAGNIOL-VILLARD]

On 10:59 Tue 23 Oct     , Sascha Hauer wrote:
> On Mon, Oct 22, 2012 at 06:07:27PM +0200, Jean-Christophe PLAGNIOL-VILLARD wrote:
> > allow to store the command abi version
> > This will allow to detect incompatibility
> > 
> > Increase envfs minor to 1
> > 
> > Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
> > ---
> >  commands/loadenv.c    |   11 ++++++++++-
> >  common/environment.c  |    7 ++++++-
> >  common/startup.c      |    4 ++--
> >  include/envfs.h       |    4 ++--
> >  include/environment.h |   13 ++++++++++++-
> >  scripts/bareboxenv.c  |    9 ++++++++-
> >  6 files changed, 40 insertions(+), 8 deletions(-)
> > 
> > diff --git a/commands/loadenv.c b/commands/loadenv.c
> > index 5bf1740..c52ee4b 100644
> > --- a/commands/loadenv.c
> > +++ b/commands/loadenv.c
> > @@ -26,6 +26,9 @@
> >  
> >  static int do_loadenv(int argc, char *argv[])
> >  {
> > +	int ret;
> > +	uint16_t command_abi_version;
> > +
> >  	char *filename, *dirname;
> >  
> >  	if (argc < 3)
> > @@ -37,7 +40,13 @@ static int do_loadenv(int argc, char *argv[])
> >  	else
> >  		filename = argv[1];
> >  	printf("loading environment from %s\n", filename);
> > -	return envfs_load(filename, dirname);
> > +	ret = envfs_load(filename, dirname, &command_abi_version);
> > +
> > +	if (command_abi_version < COMMAND_ABI_VERSION)
> 
> '!=' instead of '<'? The incompatibility is often in both directions.
> 
> We may have to introduce min/max, I'm not sure on that.
no if your env is compatible with more recent but back compatible too

as you do this with the example of mount update I did recently

if [ $command_abi_version -lt 1 ]
then
	mount /dev/disk0.0 fat /mnt
else
	mount /dev/disk0.0 /mnt
fi
> 
> 
> > -int envfs_load(char *filename, char *dir)
> > +int envfs_load(char *filename, char *dir, uint16_t *command_abi_version)
> >  {
> >  	struct envfs_super super;
> >  	void *buf = NULL, *buf_free = NULL;
> > @@ -235,6 +236,7 @@ int envfs_load(char *filename, char *dir)
> >  		goto out;
> >  	}
> >  
> > +
> >  	if ( ENVFS_32(super.magic) != ENVFS_MAGIC) {
> >  		printf("envfs: wrong magic on %s\n", filename);
> >  		ret = -EIO;
> 
> Drop this hunk.
> 
> > diff --git a/scripts/bareboxenv.c b/scripts/bareboxenv.c
> > index f44a1f8..6bd23e8 100644
> > --- a/scripts/bareboxenv.c
> > +++ b/scripts/bareboxenv.c
> > @@ -115,6 +115,7 @@ char *concat_subpath_file(const char *path, const char *f)
> >  }
> >  
> >  #include "../lib/recursive_action.c"
> > +#include "../include/command_abi.h"
> >  #include "../include/envfs.h"
> >  #include "../crypto/crc32.c"
> >  #include "../lib/make_directory.c"
> > @@ -189,14 +190,20 @@ int main(int argc, char *argv[])
> >  	}
> >  
> >  	if (load) {
> > +		uint16_t command_abi_version;
> > +
> >  		if (verbose)
> >  			printf("loading env from file %s to %s\n", filename, dirname);
> > -		envfs_load(filename, dirname);
> > +		envfs_load(filename, dirname, &command_abi_version);
> > +		if (verbose)
> > +			printf("with command_abi_version = %u\n", command_abi_version);
> >  	}
> >  	if (save) {
> >  		if (verbose)
> >  			printf("saving contents of %s to file %s\n", dirname, filename);
> >  		envfs_save(filename, dirname);
> > +		if (verbose)
> > +			printf("with command_abi_version = %u\n", COMMAND_ABI_VERSION);
> >  	}
> 
> For testing purposes or for generating an environment for another
> barebox version it would be useful to be able to specify the ABI version
> from the command line.
yeah I was thinking of it too

as I was thinking to genereate an env compatible with multiple version
as we have the way to detect it

Best Regards,
J.
> 
> Sascha
> 
> -- 
> Pengutronix e.K.                           |                             |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 0/3] introduce command abi version

[Jean-Christophe PLAGNIOL-VILLARD]

On 10:51 Tue 23 Oct     , Sascha Hauer wrote:
> On Mon, Oct 22, 2012 at 06:03:41PM +0200, Jean-Christophe PLAGNIOL-VILLARD wrote:
> > Hi,
> > 
> > 	This will allow to detect command incompability between different
> > 	version of barebox
> 
> Generally a good move. I want to it some more time to apply it though
> as this is a significant change.
> Nevertheless some comments inline.
I'm going to resend the boot sequence and want to bump the command ABI

I was think to use a major minor to report minor command udpate such as nand
test which is not critical as mount which need major bump

one a issue I get is how to describe env compatible with multiple version of
barebox and choose if env is more recent than the current barebox do not
udpate it

Best Regards,
J.

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 2/3] envfs: add command_abi_version support

[Sascha Hauer]

On Tue, Oct 23, 2012 at 12:20:59PM +0200, Jean-Christophe PLAGNIOL-VILLARD wrote:
> > 
> > We may have to introduce min/max, I'm not sure on that.
> no if your env is compatible with more recent but back compatible too
> 
> as you do this with the example of mount update I did recently
> 
> if [ $command_abi_version -lt 1 ]
> then
> 	mount /dev/disk0.0 fat /mnt
> else
> 	mount /dev/disk0.0 /mnt
> fi

That is going to be *very* painful.

Sascha


-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 2/3] envfs: add command_abi_version support

[Jean-Christophe PLAGNIOL-VILLARD]

On 14:56 Tue 23 Oct     , Sascha Hauer wrote:
> On Tue, Oct 23, 2012 at 12:20:59PM +0200, Jean-Christophe PLAGNIOL-VILLARD wrote:
> > > 
> > > We may have to introduce min/max, I'm not sure on that.
> > no if your env is compatible with more recent but back compatible too
> > 
> > as you do this with the example of mount update I did recently
> > 
> > if [ $command_abi_version -lt 1 ]
> > then
> > 	mount /dev/disk0.0 fat /mnt
> > else
> > 	mount /dev/disk0.0 /mnt
> > fi
> 
> That is going to be *very* painful.
no the idea is to allow it for external script or if we want to load an env
for multiple version of barebox

remote udpate
or boot script store on a usb key or mmc and that can be used on different
version of barebox because in production we use different version of barebox
in the life time of the product

env generate by barebox are for the current version

Best Regards,
J.

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 1/3] command: introduce abi version

[Sascha Hauer]

On Mon, Oct 22, 2012 at 06:07:26PM +0200, Jean-Christophe PLAGNIOL-VILLARD wrote:
> This will allow to detect incompatibility between the env abi and the current
> barebox one
> 
> Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
> ---
>  common/command.c      |    2 ++
>  include/command.h     |    1 +
>  include/command_abi.h |   12 ++++++++++++
>  3 files changed, 15 insertions(+)
>  create mode 100644 include/command_abi.h
> 
> diff --git a/common/command.c b/common/command.c
> index 873b3ff..c80538c 100644
> --- a/common/command.c
> +++ b/common/command.c
> @@ -154,6 +154,8 @@ static int init_command_list(void)
>  {
>  	struct command *cmdtp;
>  
> +	export_env_ull("command_abi_version", COMMAND_ABI_VERSION);
> +
>  	for (cmdtp = &__barebox_cmd_start;
>  			cmdtp != &__barebox_cmd_end;
>  			cmdtp++)
> diff --git a/include/command.h b/include/command.h
> index ffc722c..c49a014 100644
> --- a/include/command.h
> +++ b/include/command.h
> @@ -25,6 +25,7 @@
>  
>  #include <linux/list.h>
>  #include <linux/stringify.h>
> +#include <command_abi.h>
>  
>  #ifndef NULL
>  #define NULL	0
> diff --git a/include/command_abi.h b/include/command_abi.h
> new file mode 100644
> index 0000000..264c003
> --- /dev/null
> +++ b/include/command_abi.h
> @@ -0,0 +1,12 @@
> +/*
> + * (C) Copyright 2012 Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
> + *
> + * Under GPL v2
> + */
> +
> +#ifndef __COMMAND_ABI_H__
> +#define __COMMAND_ABI_H__
> +
> +#define COMMAND_ABI_VERSION	0

Please describe what this is. Something like:

/*
 * This tracks incompatible changes to the barebox command interface.
 * This number is increased when changes are introduced which will cause
 * an older environment to no longer work. This could be:
 *
 * - changes in commandline options to commands
 * - renames of commands
 * - rename of device files
 *
 * If you change this value, add a explanation of the actual change to
 * Documentation/command-abi-changes.txt
 */

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 1/3] command: introduce abi version

[Johannes Stezenbach]

On Mon, Oct 29, 2012 at 09:50:55AM +0100, Sascha Hauer wrote:
> On Mon, Oct 22, 2012 at 06:07:26PM +0200, Jean-Christophe PLAGNIOL-VILLARD wrote:
> > This will allow to detect incompatibility between the env abi and the current
> > barebox one
...
> > +
> > +#define COMMAND_ABI_VERSION	0
> 
> Please describe what this is. Something like:
> 
> /*
>  * This tracks incompatible changes to the barebox command interface.
>  * This number is increased when changes are introduced which will cause
>  * an older environment to no longer work. This could be:
>  *
>  * - changes in commandline options to commands
>  * - renames of commands
>  * - rename of device files
>  *
>  * If you change this value, add a explanation of the actual change to
>  * Documentation/command-abi-changes.txt
>  */

I wonder how this will work in practice.  If I use a simple
/env/bin/init script and someone makes changes to a command
which isn't used by my /env/bin/init, will it still cause
my environment to be detected as incompatible?

Maybe it would be a good idea to give the user control
over when to change COMMAND_ABI_VERSION by putting it into menuconfig?

Either way I guess it means the default environment's init script
would need to implement automatic update to not lose important  settings.

Bottom line: It's much better to not make incompatible ABI changes _ever_.


Johannes

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 1/3] command: introduce abi version

[Sascha Hauer]

On Mon, Oct 29, 2012 at 10:56:04AM +0100, Johannes Stezenbach wrote:
> On Mon, Oct 29, 2012 at 09:50:55AM +0100, Sascha Hauer wrote:
> > On Mon, Oct 22, 2012 at 06:07:26PM +0200, Jean-Christophe PLAGNIOL-VILLARD wrote:
> > > This will allow to detect incompatibility between the env abi and the current
> > > barebox one
> ...
> > > +
> > > +#define COMMAND_ABI_VERSION	0
> > 
> > Please describe what this is. Something like:
> > 
> > /*
> >  * This tracks incompatible changes to the barebox command interface.
> >  * This number is increased when changes are introduced which will cause
> >  * an older environment to no longer work. This could be:
> >  *
> >  * - changes in commandline options to commands
> >  * - renames of commands
> >  * - rename of device files
> >  *
> >  * If you change this value, add a explanation of the actual change to
> >  * Documentation/command-abi-changes.txt
> >  */
> 
> I wonder how this will work in practice.  If I use a simple
> /env/bin/init script and someone makes changes to a command
> which isn't used by my /env/bin/init, will it still cause
> my environment to be detected as incompatible?
> 
> Maybe it would be a good idea to give the user control
> over when to change COMMAND_ABI_VERSION by putting it into menuconfig?
> 
> Either way I guess it means the default environment's init script
> would need to implement automatic update to not lose important  settings.

I see the same problems and I am not really happy with introducing an
ABI version.
Also, introducing an ABI version could motivate people to break the ABI just
because it's simple to do so and we have a standard way of doing it.

> 
> Bottom line: It's much better to not make incompatible ABI changes _ever_.

Agreed. In case of the change to the mount command this could mean to
preserve the old ABI at least for compatibility. In case of my USB/MMC
disk example this could mean to add links.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 1/3] command: introduce abi version

[Jean-Christophe PLAGNIOL-VILLARD]

On 10:56 Mon 29 Oct     , Johannes Stezenbach wrote:
> On Mon, Oct 29, 2012 at 09:50:55AM +0100, Sascha Hauer wrote:
> > On Mon, Oct 22, 2012 at 06:07:26PM +0200, Jean-Christophe PLAGNIOL-VILLARD wrote:
> > > This will allow to detect incompatibility between the env abi and the current
> > > barebox one
> ...
> > > +
> > > +#define COMMAND_ABI_VERSION	0
> > 
> > Please describe what this is. Something like:
> > 
> > /*
> >  * This tracks incompatible changes to the barebox command interface.
> >  * This number is increased when changes are introduced which will cause
> >  * an older environment to no longer work. This could be:
> >  *
> >  * - changes in commandline options to commands
> >  * - renames of commands
> >  * - rename of device files
> >  *
> >  * If you change this value, add a explanation of the actual change to
> >  * Documentation/command-abi-changes.txt
> >  */
> 
> I wonder how this will work in practice.  If I use a simple
> /env/bin/init script and someone makes changes to a command
> which isn't used by my /env/bin/init, will it still cause
> my environment to be detected as incompatible?
> 
> Maybe it would be a good idea to give the user control
> over when to change COMMAND_ABI_VERSION by putting it into menuconfig?
> 
> Either way I guess it means the default environment's init script
> would need to implement automatic update to not lose important  settings.
here you loose nothing we just load defaulenv then you load the old one if you
one the code does not save the defaultenv automatlically
> 
> Bottom line: It's much better to not make incompatible ABI changes _ever_.
I disagree here sometimes we do need as the ABI is really not convinian or
POSIX or like this linux one

Best Regrds.
J.
> 
> 
> Johannes

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 1/3] command: introduce abi version

[Johannes Stezenbach]

On Mon, Oct 29, 2012 at 05:05:13PM +0100, Jean-Christophe PLAGNIOL-VILLARD wrote:
> On 10:56 Mon 29 Oct     , Johannes Stezenbach wrote:
> > 
> > I wonder how this will work in practice.  If I use a simple
> > /env/bin/init script and someone makes changes to a command
> > which isn't used by my /env/bin/init, will it still cause
> > my environment to be detected as incompatible?
> > 
> > Maybe it would be a good idea to give the user control
> > over when to change COMMAND_ABI_VERSION by putting it into menuconfig?
> > 
> > Either way I guess it means the default environment's init script
> > would need to implement automatic update to not lose important  settings.
> here you loose nothing we just load defaulenv then you load the old one if you
> one the code does not save the defaultenv automatlically

Well, the whole point of having an environment is that the
user can save a customized version.  So by definition
falling back to the default means you are going to annoy seme users
(even though it's usually not difficult to restore the customizations).

> > Bottom line: It's much better to not make incompatible ABI changes _ever_.
> I disagree here sometimes we do need as the ABI is really not convinian or
> POSIX or like this linux one

Well, if an ABI or API is fundamentally broken it might be
a good idea to fix it.  But it should be well justified.
And COMMAND_ABI_VERSION seems to make the issue worse and not
better, by forcing an incompatibility where there actually
might be none.

Johannes

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 1/3] command: introduce abi version

[Jean-Christophe PLAGNIOL-VILLARD]

On 18:00 Mon 29 Oct     , Johannes Stezenbach wrote:
> On Mon, Oct 29, 2012 at 05:05:13PM +0100, Jean-Christophe PLAGNIOL-VILLARD wrote:
> > On 10:56 Mon 29 Oct     , Johannes Stezenbach wrote:
> > > 
> > > I wonder how this will work in practice.  If I use a simple
> > > /env/bin/init script and someone makes changes to a command
> > > which isn't used by my /env/bin/init, will it still cause
> > > my environment to be detected as incompatible?
> > > 
> > > Maybe it would be a good idea to give the user control
> > > over when to change COMMAND_ABI_VERSION by putting it into menuconfig?
> > > 
> > > Either way I guess it means the default environment's init script
> > > would need to implement automatic update to not lose important  settings.
> > here you loose nothing we just load defaulenv then you load the old one if you
> > one the code does not save the defaultenv automatlically
> 
> Well, the whole point of having an environment is that the
> user can save a customized version.  So by definition
> falling back to the default means you are going to annoy seme users
> (even though it's usually not difficult to restore the customizations).
here the issue iin the env we have 2 part the default and the config

so we should speperate them so we can update the rootfs and keep the config
ABI

Best Regards,
J.

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox