* [PATCH] data_smart: Fix inactive overide accidental variable value corruption
@ 2021-08-02 15:42 Richard Purdie
2021-08-02 15:46 ` [bitbake-devel] " Konrad Weihmann
2021-08-03 8:39 ` Peter Kjellerstedt
0 siblings, 2 replies; 6+ messages in thread
From: Richard Purdie @ 2021-08-02 15:42 UTC (permalink / raw)
To: bitbake-devel
Setting something like:
BAR:append:unusedoverride
should cause BAR to be None, not "" which was what the datastore was
returning. This caused problems when mixing variables like:
RDEPENDS:${PN}:inactiveoverride
RDEPENDS:${BPN}
since key expansion would report key overlap when there was none. This
is a bug in the datastore. Fix it and add a test too.
[YOCTO #14088]
Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>
---
lib/bb/data_smart.py | 8 ++++----
lib/bb/tests/data.py | 5 +++++
2 files changed, 9 insertions(+), 4 deletions(-)
diff --git a/lib/bb/data_smart.py b/lib/bb/data_smart.py
index 43e9e78555..65528c6ae6 100644
--- a/lib/bb/data_smart.py
+++ b/lib/bb/data_smart.py
@@ -750,8 +750,6 @@ class DataSmart(MutableMapping):
if flag == "_content" and local_var is not None and ":append" in local_var and not parsing:
- if not value:
- value = ""
self.need_overrides()
for (r, o) in local_var[":append"]:
match = True
@@ -760,11 +758,11 @@ class DataSmart(MutableMapping):
if not o2 in self.overrides:
match = False
if match:
+ if value is None:
+ value = ""
value = value + r
if flag == "_content" and local_var is not None and ":prepend" in local_var and not parsing:
- if not value:
- value = ""
self.need_overrides()
for (r, o) in local_var[":prepend"]:
@@ -774,6 +772,8 @@ class DataSmart(MutableMapping):
if not o2 in self.overrides:
match = False
if match:
+ if value is None:
+ value = ""
value = r + value
parser = None
diff --git a/lib/bb/tests/data.py b/lib/bb/tests/data.py
index 7f1d3ffbbc..e667c7c7d3 100644
--- a/lib/bb/tests/data.py
+++ b/lib/bb/tests/data.py
@@ -405,6 +405,11 @@ class TestOverrides(unittest.TestCase):
bb.data.expandKeys(self.d)
self.assertEqual(self.d.getVar("VERSION"), "2")
+ def test_append_and_unused_override(self):
+ # Had a bug where an unused override append could return "" instead of None
+ self.d.setVar("BAR:append:unusedoverride", "testvalue2")
+ self.assertEqual(self.d.getVar("BAR"), None)
+
class TestKeyExpansion(unittest.TestCase):
def setUp(self):
self.d = bb.data.init()
--
2.30.2
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [bitbake-devel] [PATCH] data_smart: Fix inactive overide accidental variable value corruption
2021-08-02 15:42 [PATCH] data_smart: Fix inactive overide accidental variable value corruption Richard Purdie
@ 2021-08-02 15:46 ` Konrad Weihmann
2021-08-02 15:53 ` Richard Purdie
2021-08-03 8:39 ` Peter Kjellerstedt
1 sibling, 1 reply; 6+ messages in thread
From: Konrad Weihmann @ 2021-08-02 15:46 UTC (permalink / raw)
To: Richard Purdie, bitbake-devel
On 02.08.21 17:42, Richard Purdie wrote:
> Setting something like:
>
> BAR:append:unusedoverride
>
> should cause BAR to be None, not "" which was what the datastore was
> returning. This caused problems when mixing variables like:
>
> RDEPENDS:${PN}:inactiveoverride
> RDEPENDS:${BPN}
>
> since key expansion would report key overlap when there was none. This
> is a bug in the datastore. Fix it and add a test too.
>
> [YOCTO #14088]
>
> Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>
> ---
> lib/bb/data_smart.py | 8 ++++----
> lib/bb/tests/data.py | 5 +++++
> 2 files changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/lib/bb/data_smart.py b/lib/bb/data_smart.py
> index 43e9e78555..65528c6ae6 100644
> --- a/lib/bb/data_smart.py
> +++ b/lib/bb/data_smart.py
> @@ -750,8 +750,6 @@ class DataSmart(MutableMapping):
>
>
> if flag == "_content" and local_var is not None and ":append" in local_var and not parsing:
> - if not value:
> - value = ""
> self.need_overrides()
> for (r, o) in local_var[":append"]:
> match = True
> @@ -760,11 +758,11 @@ class DataSmart(MutableMapping):
> if not o2 in self.overrides:
> match = False
> if match:
> + if value is None:
> + value = ""
> value = value + r
>
> if flag == "_content" and local_var is not None and ":prepend" in local_var and not parsing:
> - if not value:
> - value = ""
> self.need_overrides()
> for (r, o) in local_var[":prepend"]:
>
> @@ -774,6 +772,8 @@ class DataSmart(MutableMapping):
> if not o2 in self.overrides:
> match = False
> if match:
> + if value is None:
> + value = ""
> value = r + value
>
> parser = None
> diff --git a/lib/bb/tests/data.py b/lib/bb/tests/data.py
> index 7f1d3ffbbc..e667c7c7d3 100644
> --- a/lib/bb/tests/data.py
> +++ b/lib/bb/tests/data.py
> @@ -405,6 +405,11 @@ class TestOverrides(unittest.TestCase):
> bb.data.expandKeys(self.d)
> self.assertEqual(self.d.getVar("VERSION"), "2")
>
> + def test_append_and_unused_override(self):
> + # Had a bug where an unused override append could return "" instead of None
> + self.d.setVar("BAR:append:unusedoverride", "testvalue2")
> + self.assertEqual(self.d.getVar("BAR"), None)
Maybe I'm not seeing it but to me the test doesn't match the changes made.
If the code change turns None into "", how can the assert ever work here?
I would have expected assertNotEqual or similar here
> +
> class TestKeyExpansion(unittest.TestCase):
> def setUp(self):
> self.d = bb.data.init()
>
>
>
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [bitbake-devel] [PATCH] data_smart: Fix inactive overide accidental variable value corruption
2021-08-02 15:46 ` [bitbake-devel] " Konrad Weihmann
@ 2021-08-02 15:53 ` Richard Purdie
2021-08-02 15:55 ` Konrad Weihmann
0 siblings, 1 reply; 6+ messages in thread
From: Richard Purdie @ 2021-08-02 15:53 UTC (permalink / raw)
To: Konrad Weihmann, bitbake-devel
On Mon, 2021-08-02 at 17:46 +0200, Konrad Weihmann wrote:
>
> On 02.08.21 17:42, Richard Purdie wrote:
> > Setting something like:
> >
> > BAR:append:unusedoverride
> >
> > should cause BAR to be None, not "" which was what the datastore was
> > returning. This caused problems when mixing variables like:
> >
> > RDEPENDS:${PN}:inactiveoverride
> > RDEPENDS:${BPN}
> >
> > since key expansion would report key overlap when there was none. This
> > is a bug in the datastore. Fix it and add a test too.
> >
> > [YOCTO #14088]
> >
> > Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>
> > ---
> > lib/bb/data_smart.py | 8 ++++----
> > lib/bb/tests/data.py | 5 +++++
> > 2 files changed, 9 insertions(+), 4 deletions(-)
> >
> > diff --git a/lib/bb/data_smart.py b/lib/bb/data_smart.py
> > index 43e9e78555..65528c6ae6 100644
> > --- a/lib/bb/data_smart.py
> > +++ b/lib/bb/data_smart.py
> > @@ -750,8 +750,6 @@ class DataSmart(MutableMapping):
> >
> >
> >
> >
> > if flag == "_content" and local_var is not None and ":append" in local_var and not parsing:
> > - if not value:
> > - value = ""
> > self.need_overrides()
> > for (r, o) in local_var[":append"]:
> > match = True
> > @@ -760,11 +758,11 @@ class DataSmart(MutableMapping):
> > if not o2 in self.overrides:
> > match = False
> > if match:
> > + if value is None:
> > + value = ""
> > value = value + r
> >
> >
> > if flag == "_content" and local_var is not None and ":prepend" in local_var and not parsing:
> > - if not value:
> > - value = ""
> > self.need_overrides()
> > for (r, o) in local_var[":prepend"]:
> >
> >
> > @@ -774,6 +772,8 @@ class DataSmart(MutableMapping):
> > if not o2 in self.overrides:
> > match = False
> > if match:
> > + if value is None:
> > + value = ""
> > value = r + value
> >
> >
> > parser = None
> > diff --git a/lib/bb/tests/data.py b/lib/bb/tests/data.py
> > index 7f1d3ffbbc..e667c7c7d3 100644
> > --- a/lib/bb/tests/data.py
> > +++ b/lib/bb/tests/data.py
> > @@ -405,6 +405,11 @@ class TestOverrides(unittest.TestCase):
> > bb.data.expandKeys(self.d)
> > self.assertEqual(self.d.getVar("VERSION"), "2")
> >
> >
> > + def test_append_and_unused_override(self):
> > + # Had a bug where an unused override append could return "" instead of None
> > + self.d.setVar("BAR:append:unusedoverride", "testvalue2")
> > + self.assertEqual(self.d.getVar("BAR"), None)
>
> Maybe I'm not seeing it but to me the test doesn't match the changes made.
> If the code change turns None into "", how can the assert ever work here?
> I would have expected assertNotEqual or similar here
The code change turns what was a return value of "" into a return value of None. I
did run the test before and after the change and it seems to be correct?
Put another way, sstting "BAR:append:unusedoverride" shouldn't change BAR from
None to "" unless the override is active.
Cheers,
Richard
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [bitbake-devel] [PATCH] data_smart: Fix inactive overide accidental variable value corruption
2021-08-02 15:53 ` Richard Purdie
@ 2021-08-02 15:55 ` Konrad Weihmann
0 siblings, 0 replies; 6+ messages in thread
From: Konrad Weihmann @ 2021-08-02 15:55 UTC (permalink / raw)
To: Richard Purdie, bitbake-devel
On 02.08.21 17:53, Richard Purdie wrote:
> On Mon, 2021-08-02 at 17:46 +0200, Konrad Weihmann wrote:
>>
>> On 02.08.21 17:42, Richard Purdie wrote:
>>> Setting something like:
>>>
>>> BAR:append:unusedoverride
>>>
>>> should cause BAR to be None, not "" which was what the datastore was
>>> returning. This caused problems when mixing variables like:
>>>
>>> RDEPENDS:${PN}:inactiveoverride
>>> RDEPENDS:${BPN}
>>>
>>> since key expansion would report key overlap when there was none. This
>>> is a bug in the datastore. Fix it and add a test too.
>>>
>>> [YOCTO #14088]
>>>
>>> Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>
>>> ---
>>> lib/bb/data_smart.py | 8 ++++----
>>> lib/bb/tests/data.py | 5 +++++
>>> 2 files changed, 9 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/lib/bb/data_smart.py b/lib/bb/data_smart.py
>>> index 43e9e78555..65528c6ae6 100644
>>> --- a/lib/bb/data_smart.py
>>> +++ b/lib/bb/data_smart.py
>>> @@ -750,8 +750,6 @@ class DataSmart(MutableMapping):
>>>
>>>
>>>
>>>
>>> if flag == "_content" and local_var is not None and ":append" in local_var and not parsing:
>>> - if not value:
>>> - value = ""
>>> self.need_overrides()
>>> for (r, o) in local_var[":append"]:
>>> match = True
>>> @@ -760,11 +758,11 @@ class DataSmart(MutableMapping):
>>> if not o2 in self.overrides:
>>> match = False
>>> if match:
>>> + if value is None:
>>> + value = ""
>>> value = value + r
>>>
>>>
>>> if flag == "_content" and local_var is not None and ":prepend" in local_var and not parsing:
>>> - if not value:
>>> - value = ""
>>> self.need_overrides()
>>> for (r, o) in local_var[":prepend"]:
>>>
>>>
>>> @@ -774,6 +772,8 @@ class DataSmart(MutableMapping):
>>> if not o2 in self.overrides:
>>> match = False
>>> if match:
>>> + if value is None:
>>> + value = ""
>>> value = r + value
>>>
>>>
>>> parser = None
>>> diff --git a/lib/bb/tests/data.py b/lib/bb/tests/data.py
>>> index 7f1d3ffbbc..e667c7c7d3 100644
>>> --- a/lib/bb/tests/data.py
>>> +++ b/lib/bb/tests/data.py
>>> @@ -405,6 +405,11 @@ class TestOverrides(unittest.TestCase):
>>> bb.data.expandKeys(self.d)
>>> self.assertEqual(self.d.getVar("VERSION"), "2")
>>>
>>>
>>> + def test_append_and_unused_override(self):
>>> + # Had a bug where an unused override append could return "" instead of None
>>> + self.d.setVar("BAR:append:unusedoverride", "testvalue2")
>>> + self.assertEqual(self.d.getVar("BAR"), None)
>>
>> Maybe I'm not seeing it but to me the test doesn't match the changes made.
>> If the code change turns None into "", how can the assert ever work here?
>> I would have expected assertNotEqual or similar here
>
> The code change turns what was a return value of "" into a return value of None. I
> did run the test before and after the change and it seems to be correct?
>
> Put another way, sstting "BAR:append:unusedoverride" shouldn't change BAR from
> None to "" unless the override is active.
Yeah, it took a moment too long in my head to compute this change :-)
Sorry for the noise.
Patch is correct IMHO, although I'm not sure about the fallout of this
change, as any kind of string operation resulting from a d.getVar might
get us into trouble here.
>
> Cheers,
>
> Richard
>
>
>
>
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [bitbake-devel] [PATCH] data_smart: Fix inactive overide accidental variable value corruption
2021-08-02 15:42 [PATCH] data_smart: Fix inactive overide accidental variable value corruption Richard Purdie
2021-08-02 15:46 ` [bitbake-devel] " Konrad Weihmann
@ 2021-08-03 8:39 ` Peter Kjellerstedt
2021-08-03 9:13 ` Richard Purdie
1 sibling, 1 reply; 6+ messages in thread
From: Peter Kjellerstedt @ 2021-08-03 8:39 UTC (permalink / raw)
To: Richard Purdie, bitbake-devel@lists.openembedded.org
> -----Original Message-----
> From: bitbake-devel@lists.openembedded.org <bitbake-
> devel@lists.openembedded.org> On Behalf Of Richard Purdie
> Sent: den 2 augusti 2021 17:42
> To: bitbake-devel@lists.openembedded.org
> Subject: [bitbake-devel] [PATCH] data_smart: Fix inactive overide
> accidental variable value corruption
>
> Setting something like:
>
> BAR:append:unusedoverride
>
> should cause BAR to be None, not "" which was what the datastore was
> returning. This caused problems when mixing variables like:
>
> RDEPENDS:${PN}:inactiveoverride
> RDEPENDS:${BPN}
>
> since key expansion would report key overlap when there was none. This
> is a bug in the datastore. Fix it and add a test too.
>
> [YOCTO #14088]
>
> Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>
> ---
> lib/bb/data_smart.py | 8 ++++----
> lib/bb/tests/data.py | 5 +++++
> 2 files changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/lib/bb/data_smart.py b/lib/bb/data_smart.py
> index 43e9e78555..65528c6ae6 100644
> --- a/lib/bb/data_smart.py
> +++ b/lib/bb/data_smart.py
> @@ -750,8 +750,6 @@ class DataSmart(MutableMapping):
>
>
> if flag == "_content" and local_var is not None and ":append" in local_var and not parsing:
> - if not value:
> - value = ""
> self.need_overrides()
> for (r, o) in local_var[":append"]:
> match = True
> @@ -760,11 +758,11 @@ class DataSmart(MutableMapping):
> if not o2 in self.overrides:
> match = False
> if match:
> + if value is None:
> + value = ""
> value = value + r
Minor detail, but at least I find this more readable:
value = (value or "") + r
>
> if flag == "_content" and local_var is not None and ":prepend" in local_var and not parsing:
> - if not value:
> - value = ""
> self.need_overrides()
> for (r, o) in local_var[":prepend"]:
>
> @@ -774,6 +772,8 @@ class DataSmart(MutableMapping):
> if not o2 in self.overrides:
> match = False
> if match:
> + if value is None:
> + value = ""
> value = r + value
>
> parser = None
> diff --git a/lib/bb/tests/data.py b/lib/bb/tests/data.py
> index 7f1d3ffbbc..e667c7c7d3 100644
> --- a/lib/bb/tests/data.py
> +++ b/lib/bb/tests/data.py
> @@ -405,6 +405,11 @@ class TestOverrides(unittest.TestCase):
> bb.data.expandKeys(self.d)
> self.assertEqual(self.d.getVar("VERSION"), "2")
>
> + def test_append_and_unused_override(self):
> + # Had a bug where an unused override append could return "" instead of None
> + self.d.setVar("BAR:append:unusedoverride", "testvalue2")
> + self.assertEqual(self.d.getVar("BAR"), None)
> +
> class TestKeyExpansion(unittest.TestCase):
> def setUp(self):
> self.d = bb.data.init()
> --
> 2.30.2
//Peter
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [bitbake-devel] [PATCH] data_smart: Fix inactive overide accidental variable value corruption
2021-08-03 8:39 ` Peter Kjellerstedt
@ 2021-08-03 9:13 ` Richard Purdie
0 siblings, 0 replies; 6+ messages in thread
From: Richard Purdie @ 2021-08-03 9:13 UTC (permalink / raw)
To: Peter Kjellerstedt, bitbake-devel@lists.openembedded.org
On Tue, 2021-08-03 at 08:39 +0000, Peter Kjellerstedt wrote:
> > -----Original Message-----
> > From: bitbake-devel@lists.openembedded.org <bitbake-
> > devel@lists.openembedded.org> On Behalf Of Richard Purdie
> > Sent: den 2 augusti 2021 17:42
> > To: bitbake-devel@lists.openembedded.org
> > Subject: [bitbake-devel] [PATCH] data_smart: Fix inactive overide
> > accidental variable value corruption
> >
> > Setting something like:
> >
> > BAR:append:unusedoverride
> >
> > should cause BAR to be None, not "" which was what the datastore was
> > returning. This caused problems when mixing variables like:
> >
> > RDEPENDS:${PN}:inactiveoverride
> > RDEPENDS:${BPN}
> >
> > since key expansion would report key overlap when there was none. This
> > is a bug in the datastore. Fix it and add a test too.
> >
> > [YOCTO #14088]
> >
> > Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>
> > ---
> > lib/bb/data_smart.py | 8 ++++----
> > lib/bb/tests/data.py | 5 +++++
> > 2 files changed, 9 insertions(+), 4 deletions(-)
> >
> > diff --git a/lib/bb/data_smart.py b/lib/bb/data_smart.py
> > index 43e9e78555..65528c6ae6 100644
> > --- a/lib/bb/data_smart.py
> > +++ b/lib/bb/data_smart.py
> > @@ -750,8 +750,6 @@ class DataSmart(MutableMapping):
> >
> >
> > if flag == "_content" and local_var is not None and ":append" in local_var and not parsing:
> > - if not value:
> > - value = ""
> > self.need_overrides()
> > for (r, o) in local_var[":append"]:
> > match = True
> > @@ -760,11 +758,11 @@ class DataSmart(MutableMapping):
> > if not o2 in self.overrides:
> > match = False
> > if match:
> > + if value is None:
> > + value = ""
> > value = value + r
>
> Minor detail, but at least I find this more readable:
That isn't quite the same thing though as for example what if value was
False?
You'd hope that we didn't have such situations but I coded it that way so
it would cause an error if it occurred, being explicit about the case we
need to handle.
Cheers,
Richard
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-08-03 9:13 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-08-02 15:42 [PATCH] data_smart: Fix inactive overide accidental variable value corruption Richard Purdie
2021-08-02 15:46 ` [bitbake-devel] " Konrad Weihmann
2021-08-02 15:53 ` Richard Purdie
2021-08-02 15:55 ` Konrad Weihmann
2021-08-03 8:39 ` Peter Kjellerstedt
2021-08-03 9:13 ` Richard Purdie
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.