Re: [U-Boot] [RFC PATCH v2 4/7] env: Introduce "transient" and "system" access flags

2016-11-30 Thread Joe Hershberger 
On Wed, Nov 30, 2016 at 4:08 AM, Bernhard Nortmann
 wrote:
> Hi Joe!
>
> Thanks for chiming in, especially seeing that you have previously worked
> on something very similar.

Sure! Sorry I didn't review this sooner.

> Am 27.11.2016 um 19:53 schrieb Joe Hershberger:
>>
>> On Wed, Nov 16, 2016 at 4:29 AM, Bernhard Nortmann
>>  wrote:
>>>
>>> "transient" (='t') is like "any", but requests that a variable
>>> should not be exported (ENV_FLAGS_VARACCESS_PREVENT_EXPORT).
>>>
>>> "system" (='S') is meant for 'internal' variables that
>>
>> The flags are positional, so 's' is not in use. It seems it would be
>> cleaner to use a lower-case 's'.
>
>
> You're probably right. I think back then I deliberately picked 'S' to avoid
> potential confusion (e.g. users specifying "s" when they actuall mean/want
> "ss"), as trying to set flags to just "S" would result in an error message.
>
> For precisely that reason I'd actually prefer to find letter(s) that would
> not conflict with existing type or access flags, but 'v'olatile seemed
> ambiguous / too broad at that time.

Yes, that is a good point and was actually a consideration when I
first chose the letters that are used for the current 2 positions.
It's not a strict requirement, but it's less error-prone.

>>> [...]
>>
>> I'm not sure why you are adding "transient" or "volatile" to the
>> varaccess. It is an orthogonal property of a variable. This is obvious
>> from the fact that you need to add yet another to compose varaccess
>> with varlifetime (or something).
>>
>> I worked on something similar years ago, but never posted an RFC.
>>
>> http://lists.denx.de/pipermail/u-boot/2010-June/073027.html
>
>
> That's a very good point. This 'orthogonality' is what actually caused me
> to come up with that "transient" vs. "system" idea. I think I got misled
> by the way that U-Boot already combined various "access" bit flags, and it
> never occured to me to introduce another property with a third flag
> character.
> Actually "?av" (any-access, volatile) and  "?rv" (read-only, volatile) would
> represent my intent perfectly well.

Perfect. That's exactly what I was proposing in 2010 (with the
addition of auto-volatile).

> I also like your "auto-volatile" concept a lot (i.e. resetting the volatile
> nature on setenv by the user). To stay unambiguous ('a' = "any" access),
> maybe this could be tagged 't'emporary.

't'emporary is ok, but I'm thinking something like o'v'erride(able) is
more explicit about the behavior.

> We'd also need a default letter for
> the lifesspan flag, possibly 'n'ormal?

I had suggested 'p'recious, but 'n'ormal is ok too. WD wanted a
pointer type too, but I never got around to implementing it.
Presumably that would conflict with 'p'recious if we went with that.

Again, it's not that important that the letters used be unique across
positions, it's just less error prone. At the same time, using the
first letter of a meaning more often will be less error-prone also.

> The temporary (= auto-volatile) flag would also nicely save use from the
> need
> to have users fumble with "setenv .flags", and the quirks involved.

Exactly. It should be the uncommon case to need to touch the .flags.

> Implementing this means some refactoring / a major overhaul of this RFC
> series, but I think it could be well worth that. I'll definitely give it
> a try when I find some time.

Awesome, thanks.
-Joe
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [RFC PATCH v2 4/7] env: Introduce "transient" and "system" access flags

2016-11-30 Thread Bernhard Nortmann 

Hi Joe!

Thanks for chiming in, especially seeing that you have previously worked
on something very similar.

Am 27.11.2016 um 19:53 schrieb Joe Hershberger:

On Wed, Nov 16, 2016 at 4:29 AM, Bernhard Nortmann
 wrote:

"transient" (='t') is like "any", but requests that a variable
should not be exported (ENV_FLAGS_VARACCESS_PREVENT_EXPORT).

"system" (='S') is meant for 'internal' variables that

The flags are positional, so 's' is not in use. It seems it would be
cleaner to use a lower-case 's'.


You're probably right. I think back then I deliberately picked 'S' to avoid
potential confusion (e.g. users specifying "s" when they actuall mean/want
"ss"), as trying to set flags to just "S" would result in an error message.

For precisely that reason I'd actually prefer to find letter(s) that would
not conflict with existing type or access flags, but 'v'olatile seemed
ambiguous / too broad at that time.


[...]

I'm not sure why you are adding "transient" or "volatile" to the
varaccess. It is an orthogonal property of a variable. This is obvious
from the fact that you need to add yet another to compose varaccess
with varlifetime (or something).

I worked on something similar years ago, but never posted an RFC.

http://lists.denx.de/pipermail/u-boot/2010-June/073027.html


That's a very good point. This 'orthogonality' is what actually caused me
to come up with that "transient" vs. "system" idea. I think I got misled
by the way that U-Boot already combined various "access" bit flags, and it
never occured to me to introduce another property with a third flag 
character.

Actually "?av" (any-access, volatile) and  "?rv" (read-only, volatile) would
represent my intent perfectly well.

I also like your "auto-volatile" concept a lot (i.e. resetting the volatile
nature on setenv by the user). To stay unambiguous ('a' = "any" access),
maybe this could be tagged 't'emporary. We'd also need a default letter for
the lifesspan flag, possibly 'n'ormal?

The temporary (= auto-volatile) flag would also nicely save use from the 
need

to have users fumble with "setenv .flags", and the quirks involved.

Implementing this means some refactoring / a major overhaul of this RFC
series, but I think it could be well worth that. I'll definitely give it
a try when I find some time.

Regards, B. Nortmann
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [RFC PATCH v2 4/7] env: Introduce "transient" and "system" access flags

2016-11-27 Thread Joe Hershberger 
On Wed, Nov 16, 2016 at 4:29 AM, Bernhard Nortmann
 wrote:
> "transient" (='t') is like "any", but requests that a variable
> should not be exported (ENV_FLAGS_VARACCESS_PREVENT_EXPORT).
>
> "system" (='S') is meant for 'internal' variables that

The flags are positional, so 's' is not in use. It seems it would be
cleaner to use a lower-case 's'.

> aren't supposed to be changed by the user. It corresponds
> to "transient" plus "read-only".
>
> Signed-off-by: Bernhard Nortmann 
>
> ---
>
> Changes in v2:
> - Fixed outdated "env_flags_varaccess_lock" to the correct
>   "env_flags_varaccess_system"
>
>  common/env_flags.c  | 11 +--
>  include/env_flags.h |  2 ++
>  2 files changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/common/env_flags.c b/common/env_flags.c
> index f39d952..2c30c7f 100644
> --- a/common/env_flags.c
> +++ b/common/env_flags.c
> @@ -28,7 +28,7 @@
>  #endif
>
>  static const char env_flags_vartype_rep[] = "sdxb" 
> ENV_FLAGS_NET_VARTYPE_REPS;
> -static const char env_flags_varaccess_rep[] = "aroc";
> +static const char env_flags_varaccess_rep[] = "aroctS";
>  static const int env_flags_varaccess_mask[] = {
> 0,
> ENV_FLAGS_VARACCESS_PREVENT_DELETE |
> @@ -37,7 +37,12 @@ static const int env_flags_varaccess_mask[] = {
> ENV_FLAGS_VARACCESS_PREVENT_DELETE |
> ENV_FLAGS_VARACCESS_PREVENT_OVERWR,
> ENV_FLAGS_VARACCESS_PREVENT_DELETE |
> -   ENV_FLAGS_VARACCESS_PREVENT_NONDEF_OVERWR};
> +   ENV_FLAGS_VARACCESS_PREVENT_NONDEF_OVERWR,
> +   ENV_FLAGS_VARACCESS_PREVENT_EXPORT,
> +   ENV_FLAGS_VARACCESS_PREVENT_DELETE |
> +   ENV_FLAGS_VARACCESS_PREVENT_CREATE |
> +   ENV_FLAGS_VARACCESS_PREVENT_OVERWR |
> +   ENV_FLAGS_VARACCESS_PREVENT_EXPORT};
>
>  #ifdef CONFIG_CMD_ENV_FLAGS
>  static const char * const env_flags_vartype_names[] = {
> @@ -55,6 +60,8 @@ static const char * const env_flags_varaccess_names[] = {
> "read-only",
> "write-once",
> "change-default",
> +   "transient",/* do not export/save */
> +   "system",   /* = "transient" plus "read-only" */

I'm not sure why you are adding "transient" or "volatile" to the
varaccess. It is an orthogonal property of a variable. This is obvious
from the fact that you need to add yet another to compose varaccess
with varlifetime (or something).

I worked on something similar years ago, but never posted an RFC.

http://lists.denx.de/pipermail/u-boot/2010-June/073027.html

>  };
>
>  /*
> diff --git a/include/env_flags.h b/include/env_flags.h
> index 7e2362a..9d66706 100644
> --- a/include/env_flags.h
> +++ b/include/env_flags.h
> @@ -25,6 +25,8 @@ enum env_flags_varaccess {
> env_flags_varaccess_readonly,
> env_flags_varaccess_writeonce,
> env_flags_varaccess_changedefault,
> +   env_flags_varaccess_transient,
> +   env_flags_varaccess_system,
> env_flags_varaccess_end
>  };
>
> --
> 2.7.3
>
> ___
> U-Boot mailing list
> U-Boot@lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [RFC PATCH v2 4/7] env: Introduce "transient" and "system" access flags

2016-11-27 Thread Simon Glass 
On 25 November 2016 at 03:48, Bernhard Nortmann
 wrote:
> Hi Simon,
>
> Am 23.11.2016 um 00:08 schrieb Simon Glass:
>>
>> Hi Bernhard,
>>
>> [...]
>> Well you could add a separate patch before this one which renames
>> everything. I don't think anyone else is working in this area.
>>
>> Regards,
>> Simon
>
>
> Doing so, I have arrived at the (additional) commit attached below.
> With that added to the series, do you think this has matured enough
> to promote it from "RFC" to an actual PATCH when submitting v3?

SGTM
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [RFC PATCH v2 4/7] env: Introduce "transient" and "system" access flags

2016-11-25 Thread Bernhard Nortmann 

Hi Simon,

Am 23.11.2016 um 00:08 schrieb Simon Glass:

Hi Bernhard,

[...]
Well you could add a separate patch before this one which renames
everything. I don't think anyone else is working in this area.

Regards,
Simon


Doing so, I have arrived at the (additional) commit attached below.
With that added to the series, do you think this has matured enough
to promote it from "RFC" to an actual PATCH when submitting v3?

Regards, B. Nortmann
From f0cae5d87bcf9366786367976a71b908ee9f2410 Mon Sep 17 00:00:00 2001
From: Bernhard Nortmann 
Date: Wed, 23 Nov 2016 10:50:16 +0100
Subject: [RFC PATCH v3 3/8] env: Rename env flag constants/enums to improve
 readability

Simon Glass suggested to cut down on the name length of these,
and to use an initializer syntax that makes it clearer which
values are associated with the various enums.

Signed-off-by: Bernhard Nortmann 
---

Changes in v3:
- Add new patch to rename env flag constants/enums

Changes in v2: None

 cmd/nvedit.c|  2 +-
 common/env_flags.c  | 87 +++--
 include/env_flags.h | 36 +++---
 3 files changed, 63 insertions(+), 62 deletions(-)

diff --git a/cmd/nvedit.c b/cmd/nvedit.c
index 9ca5cb5..84acedb 100644
--- a/cmd/nvedit.c
+++ b/cmd/nvedit.c
@@ -527,7 +527,7 @@ static int print_active_flags(ENTRY *entry)
return 0;
 
type = (enum env_flags_vartype)
-   (entry->flags & ENV_FLAGS_VARTYPE_BIN_MASK);
+   (entry->flags & ENV_FLAGS_TYPE_MASK);
access = env_flags_parse_varaccess_from_binflags(entry->flags);
printf("\t%-20s %-20s %-20s\n", entry->key,
env_flags_get_vartype_name(type),
diff --git a/common/env_flags.c b/common/env_flags.c
index 1087f4e..18cb5cf 100644
--- a/common/env_flags.c
+++ b/common/env_flags.c
@@ -22,39 +22,40 @@
 #endif
 
 #ifdef CONFIG_CMD_NET
-#define ENV_FLAGS_NET_VARTYPE_REPS "im"
+#define ENV_FLAGS_NET_VARTYPES "im"
 #else
-#define ENV_FLAGS_NET_VARTYPE_REPS ""
+#define ENV_FLAGS_NET_VARTYPES ""
 #endif
 
-static const char env_flags_vartype_rep[] = "sdxb" ENV_FLAGS_NET_VARTYPE_REPS;
+static const char env_flags_vartype_rep[] = "sdxb" ENV_FLAGS_NET_VARTYPES;
 static const char env_flags_varaccess_rep[] = "aroc";
 static const int env_flags_varaccess_mask[] = {
-   0,
-   ENV_FLAGS_VARACCESS_PREVENT_DELETE |
-   ENV_FLAGS_VARACCESS_PREVENT_CREATE |
-   ENV_FLAGS_VARACCESS_PREVENT_OVERWR,
-   ENV_FLAGS_VARACCESS_PREVENT_DELETE |
-   ENV_FLAGS_VARACCESS_PREVENT_OVERWR,
-   ENV_FLAGS_VARACCESS_PREVENT_DELETE |
-   ENV_FLAGS_VARACCESS_PREVENT_NONDEF_OVERWR};
+   [env_flags_access_any] =
+   0, /* no restrictions */
+   [env_flags_access_readonly] =
+   ENV_FLAGS_NO_DELETE | ENV_FLAGS_NO_CREATE | ENV_FLAGS_NO_OVERWR,
+   [env_flags_access_writeonce] =
+   ENV_FLAGS_NO_DELETE | ENV_FLAGS_NO_OVERWR,
+   [env_flags_access_changedefault] =
+   ENV_FLAGS_NO_DELETE | ENV_FLAGS_NO_NONDEF_OVERWR
+   };
 
 #ifdef CONFIG_CMD_ENV_FLAGS
 static const char * const env_flags_vartype_names[] = {
-   "string",
-   "decimal",
-   "hexadecimal",
-   "boolean",
+   [env_flags_type_string] = "string",
+   [env_flags_type_decimal] = "decimal",
+   [env_flags_type_hex] = "hexadecimal",
+   [env_flags_type_bool] = "boolean",
 #ifdef CONFIG_CMD_NET
-   "IP address",
-   "MAC address",
+   [env_flags_type_ipaddr] = "IP address",
+   [env_flags_type_macaddr] = "MAC address",
 #endif
 };
 static const char * const env_flags_varaccess_names[] = {
-   "any",
-   "read-only",
-   "write-once",
-   "change-default",
+   [env_flags_access_any] = "any",
+   [env_flags_access_readonly] = "read-only",
+   [env_flags_access_writeonce] = "write-once",
+   [env_flags_access_changedefault] = "change-default",
 };
 
 /*
@@ -64,7 +65,7 @@ void env_flags_print_vartypes(void)
 {
enum env_flags_vartype curtype = (enum env_flags_vartype)0;
 
-   while (curtype != env_flags_vartype_end) {
+   while (curtype != env_flags_type_end) {
printf("\t%c   -\t%s\n", env_flags_vartype_rep[curtype],
env_flags_vartype_names[curtype]);
curtype++;
@@ -78,7 +79,7 @@ void env_flags_print_varaccess(void)
 {
enum env_flags_varaccess curaccess = (enum env_flags_varaccess)0;
 
-   while (curaccess != env_flags_varaccess_end) {
+   while (curaccess != env_flags_access_end) {
printf("\t%c   -\t%s\n", env_flags_varaccess_rep[curaccess],
env_flags_varaccess_names[curaccess]);
curaccess++;
@@ -110,7 +111,7 @@ enum env_flags_vartype env_flags_parse_vartype(const char 
*flags)
char *type;
 
if (strlen(flags) <= ENV_FLAGS_VARTYPE_LOC)
-

Re: [U-Boot] [RFC PATCH v2 4/7] env: Introduce "transient" and "system" access flags

2016-11-22 Thread Simon Glass 
Hi Bernhard,

On 22 November 2016 at 11:54, Bernhard Nortmann
 wrote:
> Hi Simon!
>
> Am 19.11.2016 um 14:47 schrieb Simon Glass:
>>
>> Hi Bernhard,
>>
>> On 16 November 2016 at 03:29, Bernhard Nortmann
>>  wrote:
>>>
>>> "transient" (='t') is like "any", but requests that a variable
>>> should not be exported (ENV_FLAGS_VARACCESS_PREVENT_EXPORT).
>>>
>>> "system" (='S') is meant for 'internal' variables that
>>> aren't supposed to be changed by the user. It corresponds
>>> to "transient" plus "read-only".
>>>
>>> Signed-off-by: Bernhard Nortmann 
>>>
>>> ---
>>>
>>> Changes in v2:
>>> - Fixed outdated "env_flags_varaccess_lock" to the correct
>>>"env_flags_varaccess_system"
>>>
>>>   common/env_flags.c  | 11 +--
>>>   include/env_flags.h |  2 ++
>>>   2 files changed, 11 insertions(+), 2 deletions(-)
>>
>> Reviewed-by: Simon Glass 
>>
>> Please see below.
>>
>> [...]
>> Can these flags be shortened? This is not Java :-) Also it might be
>> helpful to use the
>>
>>[index] = value
>>
>> syntax so you can see which value it corresponds to?
>>
>> [...]
>> Regards,
>> Simon
>
>
>
> I like the [index] suggestion, which already gives a version that I find a
> lot easier to read:
>
>
> diff --git a/common/env_flags.c b/common/env_flags.c
> index f39d952..6dea70c 100644
> --- a/common/env_flags.c
> +++ b/common/env_flags.c
> @@ -28,16 +28,22 @@
>  #endif
>
>  static const char env_flags_vartype_rep[] = "sdxb"
> ENV_FLAGS_NET_VARTYPE_REPS;
> -static const char env_flags_varaccess_rep[] = "aroc";
> +static const char env_flags_varaccess_rep[] = "aroctS";
>  static const int env_flags_varaccess_mask[] = {
> -   0,
> -   ENV_FLAGS_VARACCESS_PREVENT_DELETE |
> -   ENV_FLAGS_VARACCESS_PREVENT_CREATE |
> -   ENV_FLAGS_VARACCESS_PREVENT_OVERWR,
> -   ENV_FLAGS_VARACCESS_PREVENT_DELETE |
> -   ENV_FLAGS_VARACCESS_PREVENT_OVERWR,
> -   ENV_FLAGS_VARACCESS_PREVENT_DELETE |
> -   ENV_FLAGS_VARACCESS_PREVENT_NONDEF_OVERWR};
> +   [0] =   0, /* no restrictions */
> +   [1] =   ENV_FLAGS_VARACCESS_PREVENT_DELETE
> +   | ENV_FLAGS_VARACCESS_PREVENT_CREATE
> +   | ENV_FLAGS_VARACCESS_PREVENT_OVERWR,
> +   [2] =   ENV_FLAGS_VARACCESS_PREVENT_DELETE
> +   | ENV_FLAGS_VARACCESS_PREVENT_OVERWR,
> +   [3] =   ENV_FLAGS_VARACCESS_PREVENT_DELETE
> +   | ENV_FLAGS_VARACCESS_PREVENT_NONDEF_OVERWR,
> +   [4] =   ENV_FLAGS_VARACCESS_PREVENT_EXPORT,
> +   [5] =   ENV_FLAGS_VARACCESS_PREVENT_DELETE
> +   | ENV_FLAGS_VARACCESS_PREVENT_CREATE
> +   | ENV_FLAGS_VARACCESS_PREVENT_OVERWR
> +   | ENV_FLAGS_VARACCESS_PREVENT_EXPORT
> +   };
>
>  #ifdef CONFIG_CMD_ENV_FLAGS
>  static const char * const env_flags_vartype_names[] = {
>
> As for the shortening of the various flags: The only one I'm introducing
> is ENV_FLAGS_VARACCESS_PREVENT_EXPORT (in patch 3/7), following along the
> spirit of existing ones - so I might not be the right person to bust them
> all?

Well you could add a separate patch before this one which renames
everything. I don't think anyone else is working in this area.

Regards,
Simon
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [RFC PATCH v2 4/7] env: Introduce "transient" and "system" access flags

2016-11-22 Thread Bernhard Nortmann 

Hi Simon!

Am 19.11.2016 um 14:47 schrieb Simon Glass:

Hi Bernhard,

On 16 November 2016 at 03:29, Bernhard Nortmann
 wrote:

"transient" (='t') is like "any", but requests that a variable
should not be exported (ENV_FLAGS_VARACCESS_PREVENT_EXPORT).

"system" (='S') is meant for 'internal' variables that
aren't supposed to be changed by the user. It corresponds
to "transient" plus "read-only".

Signed-off-by: Bernhard Nortmann 

---

Changes in v2:
- Fixed outdated "env_flags_varaccess_lock" to the correct
   "env_flags_varaccess_system"

  common/env_flags.c  | 11 +--
  include/env_flags.h |  2 ++
  2 files changed, 11 insertions(+), 2 deletions(-)

Reviewed-by: Simon Glass 

Please see below.

[...]
Can these flags be shortened? This is not Java :-) Also it might be
helpful to use the

   [index] = value

syntax so you can see which value it corresponds to?

[...]
Regards,
Simon



I like the [index] suggestion, which already gives a version that I find 
a lot easier to read:



diff --git a/common/env_flags.c b/common/env_flags.c
index f39d952..6dea70c 100644
--- a/common/env_flags.c
+++ b/common/env_flags.c
@@ -28,16 +28,22 @@
 #endif

 static const char env_flags_vartype_rep[] = "sdxb" 
ENV_FLAGS_NET_VARTYPE_REPS;

-static const char env_flags_varaccess_rep[] = "aroc";
+static const char env_flags_varaccess_rep[] = "aroctS";
 static const int env_flags_varaccess_mask[] = {
-   0,
-   ENV_FLAGS_VARACCESS_PREVENT_DELETE |
-   ENV_FLAGS_VARACCESS_PREVENT_CREATE |
-   ENV_FLAGS_VARACCESS_PREVENT_OVERWR,
-   ENV_FLAGS_VARACCESS_PREVENT_DELETE |
-   ENV_FLAGS_VARACCESS_PREVENT_OVERWR,
-   ENV_FLAGS_VARACCESS_PREVENT_DELETE |
-   ENV_FLAGS_VARACCESS_PREVENT_NONDEF_OVERWR};
+   [0] =   0, /* no restrictions */
+   [1] =   ENV_FLAGS_VARACCESS_PREVENT_DELETE
+   | ENV_FLAGS_VARACCESS_PREVENT_CREATE
+   | ENV_FLAGS_VARACCESS_PREVENT_OVERWR,
+   [2] =   ENV_FLAGS_VARACCESS_PREVENT_DELETE
+   | ENV_FLAGS_VARACCESS_PREVENT_OVERWR,
+   [3] =   ENV_FLAGS_VARACCESS_PREVENT_DELETE
+   | ENV_FLAGS_VARACCESS_PREVENT_NONDEF_OVERWR,
+   [4] =   ENV_FLAGS_VARACCESS_PREVENT_EXPORT,
+   [5] =   ENV_FLAGS_VARACCESS_PREVENT_DELETE
+   | ENV_FLAGS_VARACCESS_PREVENT_CREATE
+   | ENV_FLAGS_VARACCESS_PREVENT_OVERWR
+   | ENV_FLAGS_VARACCESS_PREVENT_EXPORT
+   };

 #ifdef CONFIG_CMD_ENV_FLAGS
 static const char * const env_flags_vartype_names[] = {

As for the shortening of the various flags: The only one I'm introducing
is ENV_FLAGS_VARACCESS_PREVENT_EXPORT (in patch 3/7), following along the
spirit of existing ones - so I might not be the right person to bust them
all?

Regards, B. Nortmann

___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [RFC PATCH v2 4/7] env: Introduce "transient" and "system" access flags

2016-11-19 Thread Simon Glass 
Hi Bernhard,

On 16 November 2016 at 03:29, Bernhard Nortmann
 wrote:
> "transient" (='t') is like "any", but requests that a variable
> should not be exported (ENV_FLAGS_VARACCESS_PREVENT_EXPORT).
>
> "system" (='S') is meant for 'internal' variables that
> aren't supposed to be changed by the user. It corresponds
> to "transient" plus "read-only".
>
> Signed-off-by: Bernhard Nortmann 
>
> ---
>
> Changes in v2:
> - Fixed outdated "env_flags_varaccess_lock" to the correct
>   "env_flags_varaccess_system"
>
>  common/env_flags.c  | 11 +--
>  include/env_flags.h |  2 ++
>  2 files changed, 11 insertions(+), 2 deletions(-)

Reviewed-by: Simon Glass 

Please see below.

>
> diff --git a/common/env_flags.c b/common/env_flags.c
> index f39d952..2c30c7f 100644
> --- a/common/env_flags.c
> +++ b/common/env_flags.c
> @@ -28,7 +28,7 @@
>  #endif
>
>  static const char env_flags_vartype_rep[] = "sdxb" 
> ENV_FLAGS_NET_VARTYPE_REPS;
> -static const char env_flags_varaccess_rep[] = "aroc";
> +static const char env_flags_varaccess_rep[] = "aroctS";
>  static const int env_flags_varaccess_mask[] = {
> 0,
> ENV_FLAGS_VARACCESS_PREVENT_DELETE |
> @@ -37,7 +37,12 @@ static const int env_flags_varaccess_mask[] = {
> ENV_FLAGS_VARACCESS_PREVENT_DELETE |
> ENV_FLAGS_VARACCESS_PREVENT_OVERWR,
> ENV_FLAGS_VARACCESS_PREVENT_DELETE |
> -   ENV_FLAGS_VARACCESS_PREVENT_NONDEF_OVERWR};
> +   ENV_FLAGS_VARACCESS_PREVENT_NONDEF_OVERWR,
> +   ENV_FLAGS_VARACCESS_PREVENT_EXPORT,
> +   ENV_FLAGS_VARACCESS_PREVENT_DELETE |

Can these flags be shortened? This is not Java :-) Also it might be
helpful to use the

  [index] = value

syntax so you can see which value it corresponds to?

> +   ENV_FLAGS_VARACCESS_PREVENT_CREATE |
> +   ENV_FLAGS_VARACCESS_PREVENT_OVERWR |
> +   ENV_FLAGS_VARACCESS_PREVENT_EXPORT};
>
>  #ifdef CONFIG_CMD_ENV_FLAGS
>  static const char * const env_flags_vartype_names[] = {
> @@ -55,6 +60,8 @@ static const char * const env_flags_varaccess_names[] = {
> "read-only",
> "write-once",
> "change-default",
> +   "transient",/* do not export/save */
> +   "system",   /* = "transient" plus "read-only" */
>  };
>
>  /*
> diff --git a/include/env_flags.h b/include/env_flags.h
> index 7e2362a..9d66706 100644
> --- a/include/env_flags.h
> +++ b/include/env_flags.h
> @@ -25,6 +25,8 @@ enum env_flags_varaccess {
> env_flags_varaccess_readonly,
> env_flags_varaccess_writeonce,
> env_flags_varaccess_changedefault,
> +   env_flags_varaccess_transient,
> +   env_flags_varaccess_system,
> env_flags_varaccess_end
>  };
>
> --
> 2.7.3
>

Regards,
Simon
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

[U-Boot] [RFC PATCH v2 4/7] env: Introduce "transient" and "system" access flags

2016-11-16 Thread Bernhard Nortmann 
"transient" (='t') is like "any", but requests that a variable
should not be exported (ENV_FLAGS_VARACCESS_PREVENT_EXPORT).

"system" (='S') is meant for 'internal' variables that
aren't supposed to be changed by the user. It corresponds
to "transient" plus "read-only".

Signed-off-by: Bernhard Nortmann 

---

Changes in v2:
- Fixed outdated "env_flags_varaccess_lock" to the correct
  "env_flags_varaccess_system"

 common/env_flags.c  | 11 +--
 include/env_flags.h |  2 ++
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/common/env_flags.c b/common/env_flags.c
index f39d952..2c30c7f 100644
--- a/common/env_flags.c
+++ b/common/env_flags.c
@@ -28,7 +28,7 @@
 #endif
 
 static const char env_flags_vartype_rep[] = "sdxb" ENV_FLAGS_NET_VARTYPE_REPS;
-static const char env_flags_varaccess_rep[] = "aroc";
+static const char env_flags_varaccess_rep[] = "aroctS";
 static const int env_flags_varaccess_mask[] = {
0,
ENV_FLAGS_VARACCESS_PREVENT_DELETE |
@@ -37,7 +37,12 @@ static const int env_flags_varaccess_mask[] = {
ENV_FLAGS_VARACCESS_PREVENT_DELETE |
ENV_FLAGS_VARACCESS_PREVENT_OVERWR,
ENV_FLAGS_VARACCESS_PREVENT_DELETE |
-   ENV_FLAGS_VARACCESS_PREVENT_NONDEF_OVERWR};
+   ENV_FLAGS_VARACCESS_PREVENT_NONDEF_OVERWR,
+   ENV_FLAGS_VARACCESS_PREVENT_EXPORT,
+   ENV_FLAGS_VARACCESS_PREVENT_DELETE |
+   ENV_FLAGS_VARACCESS_PREVENT_CREATE |
+   ENV_FLAGS_VARACCESS_PREVENT_OVERWR |
+   ENV_FLAGS_VARACCESS_PREVENT_EXPORT};
 
 #ifdef CONFIG_CMD_ENV_FLAGS
 static const char * const env_flags_vartype_names[] = {
@@ -55,6 +60,8 @@ static const char * const env_flags_varaccess_names[] = {
"read-only",
"write-once",
"change-default",
+   "transient",/* do not export/save */
+   "system",   /* = "transient" plus "read-only" */
 };
 
 /*
diff --git a/include/env_flags.h b/include/env_flags.h
index 7e2362a..9d66706 100644
--- a/include/env_flags.h
+++ b/include/env_flags.h
@@ -25,6 +25,8 @@ enum env_flags_varaccess {
env_flags_varaccess_readonly,
env_flags_varaccess_writeonce,
env_flags_varaccess_changedefault,
+   env_flags_varaccess_transient,
+   env_flags_varaccess_system,
env_flags_varaccess_end
 };
 
-- 
2.7.3

___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot