[PATCH v2] write_propval_string: Use a list of strings instead of "\0" in a string

[PATCH v2] write_propval_string: Use a list of strings instead of "\0" in a string

[Uwe Kleine-König]

From: Uwe Kleine-König <u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>

A string that contains '\0' can be written as a list of strings e.g.

	clock-names = "di0_pll\0di1_pll\0di0_sel\0di1_sel\0di2_sel\0di3_sel\0di0\0di1";

is equivalent to

	clock-names = "di0_pll", "di1_pll", "di0_sel", "di1_sel", "di2_sel", "di3_sel", "di0", "di1";

The latter is easier to read, to use this format instead.

Two test files are adapted accordingly to keep the test suite happy.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
---
Changes since (implicit) v1, sent with Message-Id:
20230426182405.572729-1-u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org:

 - Adapt the test suite

 tests/type-preservation.dt.yaml | 2 +-
 tests/type-preservation.dts     | 2 +-
 treesource.c                    | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/tests/type-preservation.dt.yaml b/tests/type-preservation.dt.yaml
index a0cc64cc4b69..e238d395aa02 100644
--- a/tests/type-preservation.dt.yaml
+++ b/tests/type-preservation.dt.yaml
@@ -12,7 +12,7 @@
     int16-matrix: [!u16 [0x1234, 0x5678], [0x90ab, 0xcdef]]
     int64: [!u64 [0x200000000]]
     int64-array: [!u64 [0x100000000, 0x0]]
-    a-string-with-nulls: ["foo\0bar", "baz"]
+    a-string-array: ["foo", "bar", "baz"]
     a-phandle: [[!phandle 0x1]]
     a-phandle-with-args: [[!phandle 0x1, 0x0, 0x1], [!phandle 0x1, 0x2, 0x3]]
     subsubnode:
diff --git a/tests/type-preservation.dts b/tests/type-preservation.dts
index 921ea21172d1..443424dc28ec 100644
--- a/tests/type-preservation.dts
+++ b/tests/type-preservation.dts
@@ -15,7 +15,7 @@
 		int16-matrix = /bits/ 16 <0x1234 0x5678>, <0x90ab 0xcdef>;
 		int64 = /bits/ 64 <0x200000000>;
 		int64-array = /bits/ 64 <0x100000000 0x00> int64_array_label_end:;
-		a-string-with-nulls = "foo\0bar", "baz";
+		a-string-array = "foo", "bar", "baz";
 		a-phandle = <&subsub1>;
 		a-phandle-with-args = <&subsub1 0x00 0x01>, <&subsub1 0x02 0x03>;
 
diff --git a/treesource.c b/treesource.c
index 33fedee82d58..de30188189fb 100644
--- a/treesource.c
+++ b/treesource.c
@@ -87,7 +87,7 @@ static void write_propval_string(FILE *f, const char *s, size_t len)
 			fprintf(f, "\\\"");
 			break;
 		case '\0':
-			fprintf(f, "\\0");
+			fprintf(f, "\", \"");
 			break;
 		default:
 			if (isprint((unsigned char)c))
-- 
2.39.2

Re: [PATCH v2] write_propval_string: Use a list of strings instead of "\0" in a string

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 3942 bytes --]

On Fri, Apr 28, 2023 at 01:32:17PM +0200, Uwe Kleine-König wrote:
> From: Uwe Kleine-König <u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> 
> A string that contains '\0' can be written as a list of strings e.g.
> 
> 	clock-names = "di0_pll\0di1_pll\0di0_sel\0di1_sel\0di2_sel\0di3_sel\0di0\0di1";
> 
> is equivalent to
> 
> 	clock-names = "di0_pll", "di1_pll", "di0_sel", "di1_sel", "di2_sel", "di3_sel", "di0", "di1";
> 
> The latter is easier to read, to use this format instead.
> 
> Two test files are adapted accordingly to keep the test suite happy.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> ---
> Changes since (implicit) v1, sent with Message-Id:
> 20230426182405.572729-1-u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org:
> 
>  - Adapt the test suite
> 
>  tests/type-preservation.dt.yaml | 2 +-
>  tests/type-preservation.dts     | 2 +-
>  treesource.c                    | 2 +-
>  3 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/tests/type-preservation.dt.yaml b/tests/type-preservation.dt.yaml
> index a0cc64cc4b69..e238d395aa02 100644
> --- a/tests/type-preservation.dt.yaml
> +++ b/tests/type-preservation.dt.yaml
> @@ -12,7 +12,7 @@
>      int16-matrix: [!u16 [0x1234, 0x5678], [0x90ab, 0xcdef]]
>      int64: [!u64 [0x200000000]]
>      int64-array: [!u64 [0x100000000, 0x0]]
> -    a-string-with-nulls: ["foo\0bar", "baz"]
> +    a-string-array: ["foo", "bar", "baz"]


Ah.  I was afraid of this.  So "fixing" the test highlights another
problem.  It's pretty clear to me that the whole point of this test is
to test the preservation of the internal \0, as distinct from the
separate strings later in there.  So, this is exercising an edge case
of the typing markers we now add.

Now.. personally, I've never been particularly convinced that the type
preservation stuff was a particularly good idea.  It will never be
perfect, and it can give a misleading impression that information is
preserved into the dtb which isn't.  But, it's pretty well established
now, and I assume people had reasons for wanting it.

So, your patch makes things nicer when going from dtb -> dts, but
breaks an established feature when going dts -> dts, or dts -> yaml.
So, I think we need to rethink.

Rather than changing how "strings" are emitted, I think you want to
change how we guess typing information when coming from dtb: instead
of just putting a single string marker on the property, we should put
one after every \0 and we should get the output you're after.

>      a-phandle: [[!phandle 0x1]]
>      a-phandle-with-args: [[!phandle 0x1, 0x0, 0x1], [!phandle 0x1, 0x2, 0x3]]
>      subsubnode:
> diff --git a/tests/type-preservation.dts b/tests/type-preservation.dts
> index 921ea21172d1..443424dc28ec 100644
> --- a/tests/type-preservation.dts
> +++ b/tests/type-preservation.dts
> @@ -15,7 +15,7 @@
>  		int16-matrix = /bits/ 16 <0x1234 0x5678>, <0x90ab 0xcdef>;
>  		int64 = /bits/ 64 <0x200000000>;
>  		int64-array = /bits/ 64 <0x100000000 0x00> int64_array_label_end:;
> -		a-string-with-nulls = "foo\0bar", "baz";
> +		a-string-array = "foo", "bar", "baz";
>  		a-phandle = <&subsub1>;
>  		a-phandle-with-args = <&subsub1 0x00 0x01>, <&subsub1 0x02 0x03>;
>  
> diff --git a/treesource.c b/treesource.c
> index 33fedee82d58..de30188189fb 100644
> --- a/treesource.c
> +++ b/treesource.c
> @@ -87,7 +87,7 @@ static void write_propval_string(FILE *f, const char *s, size_t len)
>  			fprintf(f, "\\\"");
>  			break;
>  		case '\0':
> -			fprintf(f, "\\0");
> +			fprintf(f, "\", \"");
>  			break;
>  		default:
>  			if (isprint((unsigned char)c))

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2] write_propval_string: Use a list of strings instead of "\0" in a string

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 3221 bytes --]

On Sat, Apr 29, 2023 at 04:29:50PM +1000, David Gibson wrote:
> On Fri, Apr 28, 2023 at 01:32:17PM +0200, Uwe Kleine-König wrote:
> > From: Uwe Kleine-König <u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> > 
> > A string that contains '\0' can be written as a list of strings e.g.
> > 
> > 	clock-names = "di0_pll\0di1_pll\0di0_sel\0di1_sel\0di2_sel\0di3_sel\0di0\0di1";
> > 
> > is equivalent to
> > 
> > 	clock-names = "di0_pll", "di1_pll", "di0_sel", "di1_sel", "di2_sel", "di3_sel", "di0", "di1";
> > 
> > The latter is easier to read, to use this format instead.
> > 
> > Two test files are adapted accordingly to keep the test suite happy.
> > 
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> > ---
> > Changes since (implicit) v1, sent with Message-Id:
> > 20230426182405.572729-1-u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org:
> > 
> >  - Adapt the test suite
> > 
> >  tests/type-preservation.dt.yaml | 2 +-
> >  tests/type-preservation.dts     | 2 +-
> >  treesource.c                    | 2 +-
> >  3 files changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/tests/type-preservation.dt.yaml b/tests/type-preservation.dt.yaml
> > index a0cc64cc4b69..e238d395aa02 100644
> > --- a/tests/type-preservation.dt.yaml
> > +++ b/tests/type-preservation.dt.yaml
> > @@ -12,7 +12,7 @@
> >      int16-matrix: [!u16 [0x1234, 0x5678], [0x90ab, 0xcdef]]
> >      int64: [!u64 [0x200000000]]
> >      int64-array: [!u64 [0x100000000, 0x0]]
> > -    a-string-with-nulls: ["foo\0bar", "baz"]
> > +    a-string-array: ["foo", "bar", "baz"]
> 
> 
> Ah.  I was afraid of this.  So "fixing" the test highlights another
> problem.  It's pretty clear to me that the whole point of this test is
> to test the preservation of the internal \0, as distinct from the
> separate strings later in there.  So, this is exercising an edge case
> of the typing markers we now add.
> 
> Now.. personally, I've never been particularly convinced that the type
> preservation stuff was a particularly good idea.  It will never be
> perfect, and it can give a misleading impression that information is
> preserved into the dtb which isn't.  But, it's pretty well established
> now, and I assume people had reasons for wanting it.
> 
> So, your patch makes things nicer when going from dtb -> dts, but
> breaks an established feature when going dts -> dts, or dts -> yaml.
> So, I think we need to rethink.
> 
> Rather than changing how "strings" are emitted, I think you want to
> change how we guess typing information when coming from dtb: instead
> of just putting a single string marker on the property, we should put
> one after every \0 and we should get the output you're after.

Yeah, sounds reasonable. I looked into that, and I don't see an easy way
to do that. I'll put that aside and concentrate on the other two bigger
changes (i.e. not overwriting phandles by an overlay and label/reference
restoring) first.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2] write_propval_string: Use a list of strings instead of "\0" in a string

[Rob Herring]

On Sat, Apr 29, 2023 at 1:52 AM David Gibson
<david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
>
> On Fri, Apr 28, 2023 at 01:32:17PM +0200, Uwe Kleine-König wrote:
> > From: Uwe Kleine-König <u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> >
> > A string that contains '\0' can be written as a list of strings e.g.
> >
> >       clock-names = "di0_pll\0di1_pll\0di0_sel\0di1_sel\0di2_sel\0di3_sel\0di0\0di1";
> >
> > is equivalent to
> >
> >       clock-names = "di0_pll", "di1_pll", "di0_sel", "di1_sel", "di2_sel", "di3_sel", "di0", "di1";
> >
> > The latter is easier to read, to use this format instead.
> >
> > Two test files are adapted accordingly to keep the test suite happy.
> >
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> > ---
> > Changes since (implicit) v1, sent with Message-Id:
> > 20230426182405.572729-1-u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org:
> >
> >  - Adapt the test suite
> >
> >  tests/type-preservation.dt.yaml | 2 +-
> >  tests/type-preservation.dts     | 2 +-
> >  treesource.c                    | 2 +-
> >  3 files changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/tests/type-preservation.dt.yaml b/tests/type-preservation.dt.yaml
> > index a0cc64cc4b69..e238d395aa02 100644
> > --- a/tests/type-preservation.dt.yaml
> > +++ b/tests/type-preservation.dt.yaml
> > @@ -12,7 +12,7 @@
> >      int16-matrix: [!u16 [0x1234, 0x5678], [0x90ab, 0xcdef]]
> >      int64: [!u64 [0x200000000]]
> >      int64-array: [!u64 [0x100000000, 0x0]]
> > -    a-string-with-nulls: ["foo\0bar", "baz"]
> > +    a-string-array: ["foo", "bar", "baz"]
>
>
> Ah.  I was afraid of this.  So "fixing" the test highlights another
> problem.  It's pretty clear to me that the whole point of this test is
> to test the preservation of the internal \0, as distinct from the
> separate strings later in there.  So, this is exercising an edge case
> of the typing markers we now add.
>
> Now.. personally, I've never been particularly convinced that the type
> preservation stuff was a particularly good idea.  It will never be
> perfect, and it can give a misleading impression that information is
> preserved into the dtb which isn't.  But, it's pretty well established
> now, and I assume people had reasons for wanting it.

I don't really think so. It was a side effect of adding the yaml
support AFAICT. Before commit 32b9c6130762 ("Preserve datatype markers
when emitting dts format"), any '\0' was not maintained.

Speaking of the YAML output, at least for dtschema, it is no longer
being used with using dtb format directly for over a year now. Support
for yaml encoding in dtschema was removed completely in the January
release. Perhaps in dtc it should be deprecated for some time if not
just outright removed. I'm not aware of anyone else using it.

Rob

Re: [PATCH v2] write_propval_string: Use a list of strings instead of "\0" in a string

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 3566 bytes --]

On Mon, May 01, 2023 at 01:43:22PM -0500, Rob Herring wrote:
> On Sat, Apr 29, 2023 at 1:52 AM David Gibson
> <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> >
> > On Fri, Apr 28, 2023 at 01:32:17PM +0200, Uwe Kleine-König wrote:
> > > From: Uwe Kleine-König <u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> > >
> > > A string that contains '\0' can be written as a list of strings e.g.
> > >
> > >       clock-names = "di0_pll\0di1_pll\0di0_sel\0di1_sel\0di2_sel\0di3_sel\0di0\0di1";
> > >
> > > is equivalent to
> > >
> > >       clock-names = "di0_pll", "di1_pll", "di0_sel", "di1_sel", "di2_sel", "di3_sel", "di0", "di1";
> > >
> > > The latter is easier to read, to use this format instead.
> > >
> > > Two test files are adapted accordingly to keep the test suite happy.
> > >
> > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> > > ---
> > > Changes since (implicit) v1, sent with Message-Id:
> > > 20230426182405.572729-1-u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org:
> > >
> > >  - Adapt the test suite
> > >
> > >  tests/type-preservation.dt.yaml | 2 +-
> > >  tests/type-preservation.dts     | 2 +-
> > >  treesource.c                    | 2 +-
> > >  3 files changed, 3 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/tests/type-preservation.dt.yaml b/tests/type-preservation.dt.yaml
> > > index a0cc64cc4b69..e238d395aa02 100644
> > > --- a/tests/type-preservation.dt.yaml
> > > +++ b/tests/type-preservation.dt.yaml
> > > @@ -12,7 +12,7 @@
> > >      int16-matrix: [!u16 [0x1234, 0x5678], [0x90ab, 0xcdef]]
> > >      int64: [!u64 [0x200000000]]
> > >      int64-array: [!u64 [0x100000000, 0x0]]
> > > -    a-string-with-nulls: ["foo\0bar", "baz"]
> > > +    a-string-array: ["foo", "bar", "baz"]
> >
> >
> > Ah.  I was afraid of this.  So "fixing" the test highlights another
> > problem.  It's pretty clear to me that the whole point of this test is
> > to test the preservation of the internal \0, as distinct from the
> > separate strings later in there.  So, this is exercising an edge case
> > of the typing markers we now add.
> >
> > Now.. personally, I've never been particularly convinced that the type
> > preservation stuff was a particularly good idea.  It will never be
> > perfect, and it can give a misleading impression that information is
> > preserved into the dtb which isn't.  But, it's pretty well established
> > now, and I assume people had reasons for wanting it.
> 
> I don't really think so. It was a side effect of adding the yaml
> support AFAICT. Before commit 32b9c6130762 ("Preserve datatype markers
> when emitting dts format"), any '\0' was not maintained.

Right, which also introduced this test, presumbly to verify those
semantics.

> Speaking of the YAML output, at least for dtschema, it is no longer
> being used with using dtb format directly for over a year now. Support
> for yaml encoding in dtschema was removed completely in the January
> release. Perhaps in dtc it should be deprecated for some time if not
> just outright removed. I'm not aware of anyone else using it.

Oh, that's good to hear.  I've long thought the yaml output was a bad
idea, because it makes people think the dtb encodes type information
which it doesn't and can't.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]