[PATCH] sstate: Consider .lock suffix for when loading sstate file from mirror

[PATCH] sstate: Consider .lock suffix for when loading sstate file from mirror

[Manuel Leonhardt]

[-- Attachment #1.1: Type: text/plain, Size: 2735 bytes --]

When using an sstate mirror over HTTP/S, a lockfile is created during
download. Previously, this resulted in an OSError (ENAMETOOLONG) in the
bb.utils.lockfile function because the lockfile has the filename of the
sstate file plus ".lock". Since this case is also not handled properly
in bb.utils.lockfile, this caused populate tasks to hang indefinitely.

Also, previously when generating a shorter filename, the reserved
characters for .siginfo and now .lock were not considered. This is now
fixed by using limit instead of 254, which is the maximum length.

Signed-off-by: Manuel Leonhardt <mleonhardt@arri.de>
---
 meta/classes/sstate.bbclass | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/meta/classes/sstate.bbclass b/meta/classes/sstate.bbclass
index 6e4eb09f8e..b6a8951f8a 100644
--- a/meta/classes/sstate.bbclass
+++ b/meta/classes/sstate.bbclass
@@ -14,6 +14,8 @@ def generate_sstatefn(spec, hash, taskname, siginfo, d):
     if siginfo:
         limit = 254
         extension = ".tar.zst.siginfo"
+    # 5 chars reserved for .lock suffix when downloading from sstate mirror
+    limit -= 5
     if not hash:
         hash = "INVALID"
     fn = spec + hash + "_" + taskname + extension
@@ -22,7 +24,7 @@ def generate_sstatefn(spec, hash, taskname, siginfo, d):
         components = spec.split(":")
         # Fields 0,5,6 are mandatory, 1 is most useful, 2,3,4 are just for information
         # 7 is for the separators
-        avail = (254 - len(hash + "_" + taskname + extension) - len(components[0]) - len(components[1]) - len(components[5]) - len(components[6]) - 7) // 3
+        avail = (limit - len(hash + "_" + taskname + extension) - len(components[0]) - len(components[1]) - len(components[5]) - len(components[6]) - 7) // 3
         components[2] = components[2][:avail]
         components[3] = components[3][:avail]
         components[4] = components[4][:avail]
--
2.33.1


_______________________________________________________
Manuel
Leonhardt
Softwareentwickler
​
ARRI
Arnold & Richter Cine Technik GmbH & Co. Betriebs KG
Herbert-Bayer-Str. 10,
80807
München
www.arri.com

+49 89 3809-1719
MLeonhardt@arri.de

​
Get all the latest information from www.arri.com, Facebook, Twitter, Instagram and YouTube.

Arnold & Richter Cine Technik GmbH & Co. Betriebs KG
Sitz: München - Registergericht: Amtsgericht München - Handelsregisternummer: HRA 57918
Persönlich haftender Gesellschafter: Arnold & Richter Cine Technik GmbH
Sitz: München - Registergericht: Amtsgericht München - Handelsregisternummer: HRB 54477
Geschäftsführer: Dr. Michael Neuhäuser; Stephan Schenk; Walter Trauninger; Markus Zeiler

​

[-- Attachment #1.2: Type: text/html, Size: 11469 bytes --]

[-- Attachment #2: image555154.png --]
[-- Type: image/png, Size: 528 bytes --]

[-- Attachment #3: image650886.png --]
[-- Type: image/png, Size: 824 bytes --]

Re: [PATCH] sstate: Consider .lock suffix for when loading sstate file from mirror

[Manuel Leonhardt]

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

See also https://lists.openembedded.org/g/bitbake-devel/topic/86724965 , where bb.utils.lockfile is fixed to raise when the filename of a lockfile is too long.

[-- Attachment #2: Type: text/html, Size: 268 bytes --]

Re: [OE-core] [PATCH] sstate: Consider .lock suffix for when loading sstate file from mirror

[Richard Purdie]

On Sun, 2021-10-31 at 21:08 +0100, Manuel Leonhardt wrote:
> When using an sstate mirror over HTTP/S, a lockfile is created during
> download. Previously, this resulted in an OSError (ENAMETOOLONG) in the
> bb.utils.lockfile function because the lockfile has the filename of the
> sstate file plus ".lock". Since this case is also not handled properly
> in bb.utils.lockfile, this caused populate tasks to hang indefinitely.
> 
> Also, previously when generating a shorter filename, the reserved
> characters for .siginfo and now .lock were not considered. This is now
> fixed by using limit instead of 254, which is the maximum length.
> 
> Signed-off-by: Manuel Leonhardt <mleonhardt@arri.de>
> ---
>  meta/classes/sstate.bbclass | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)

I think it may be better to fix the lock function not to use large filenames?
The worst case is two locks overlap with the truncated names but that isn't a
bad fallback situation...

Cheers,

Richard

Re: [PATCH] sstate: Consider .lock suffix for when loading sstate file from mirror

[Manuel Leonhardt]

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

I submitted a patch to fix bb.utils.lockfile to at least break the loop and fail when a too long filename is passed with https://lists.openembedded.org/g/bitbake-devel/message/12850

Nevertheless, you have a good point here: Fixing the code that constructs the filename of the lock files would be the better solution. For the sstate files only lockfiles of .siginfo files can overlap, because the filename of the actual sstate file is already 8 characters shorter than 255 characters. I have one though on that:

From what I have seen, the filename of lockfiles is mostly constructed by appending ".lock" without any further check. Truncating the filename of the lockfile inside bb.utils.lockfile could be confusing however, since the function would not use the filename that is was explicitly passed. Hence, I guess bb.utils.lockfile should fail when a too long filename was passed, and the function building the lockfile's filename should ensure to keep it limited; that's bb.fetch2.FetchData.__init__ then if I'm not mistaken. Would you agree? Or would you say, changing the filename inside bb.utils.lockfile is ok, as long as the function is working?

The drive-by fix I made with using limit instead of 254 when shortening the filename is still correct and necessary, right?

Best,

Manuel

May I suggest two solutions here:

a) Let bb.utils.lockfile

[-- Attachment #2: Type: text/html, Size: 1539 bytes --]

Re: [OE-core] [PATCH] sstate: Consider .lock suffix for when loading sstate file from mirror

[Richard Purdie]

On Mon, 2021-11-01 at 03:47 -0700, Manuel Leonhardt wrote:
> I submitted a patch to fix bb.utils.lockfile to at least break the loop and
> fail
> when a too long filename is passed with
> https://lists.openembedded.org/g/bitbake-devel/message/12850
> 
> Nevertheless, you have a good point here: Fixing the code that constructs the
> filename of the lock files would be the better solution. For the sstate files
> only lockfiles of .siginfo files can overlap, because the filename of the
> actual
> sstate file is already 8 characters shorter than 255 characters. I have one
> though on that:
> 
> From what I have seen, the filename of lockfiles is mostly constructed by
> appending ".lock" without any further check. Truncating the filename of the
> lockfile inside bb.utils.lockfile could be confusing however, since the
> function
> would not use the filename that is was explicitly passed. Hence, I guess
> bb.utils.lockfile should fail when a too long filename was passed, and the
> function building the lockfile's filename should ensure to keep it limited;
> that's bb.fetch2.FetchData.__init__ then if I'm not mistaken. Would you agree?
> Or would you say, changing the filename inside bb.utils.lockfile is ok, as
> long
> as the function is working?
> 
> The drive-by fix I made with using limit instead of 254 when shortening the
> filename is still correct and necessary, right?

I think we just let lockfile truncate the filename if necessary since this could
affect other users of the funciton too. 

Looking at the code you changed with the limit value it isn't correct since
extension is already accounted for in avail (although I agree that code is
confusing and I had to think twice about it).

Cheers,

Richard

Re: [PATCH] sstate: Consider .lock suffix for when loading sstate file from mirror

[Manuel Leonhardt]

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

On Mon, Nov 1, 2021 at 11:52 AM, Richard Purdie wrote:

> 
> On Mon, 2021-11-01 at 03:47 -0700, Manuel Leonhardt wrote:
> 
>> I submitted a patch to fix bb.utils.lockfile to at least break the loop
>> and
>> fail
>> when a too long filename is passed with
>> https://lists.openembedded.org/g/bitbake-devel/message/12850
>> 
>> Nevertheless, you have a good point here: Fixing the code that constructs
>> the
>> filename of the lock files would be the better solution. For the sstate
>> files
>> only lockfiles of .siginfo files can overlap, because the filename of the
>> actual
>> sstate file is already 8 characters shorter than 255 characters. I have
>> one
>> though on that:
>> 
>> From what I have seen, the filename of lockfiles is mostly constructed by
>> appending ".lock" without any further check. Truncating the filename of
>> the
>> lockfile inside bb.utils.lockfile could be confusing however, since the
>> function
>> would not use the filename that is was explicitly passed. Hence, I guess
>> bb.utils.lockfile should fail when a too long filename was passed, and the
>> 
>> function building the lockfile's filename should ensure to keep it
>> limited;
>> that's bb.fetch2.FetchData.__init__ then if I'm not mistaken. Would you
>> agree?
>> Or would you say, changing the filename inside bb.utils.lockfile is ok, as
>> 
>> long
>> as the function is working?
>> 
>> The drive-by fix I made with using limit instead of 254 when shortening
>> the
>> filename is still correct and necessary, right?
> 
> I think we just let lockfile truncate the filename if necessary since this
> could
> affect other users of the funciton too.
> 
> Looking at the code you changed with the limit value it isn't correct
> since
> extension is already accounted for in avail (although I agree that code is
> 
> confusing and I had to think twice about it).
> 
> Cheers,
> 
> Richard

Ok, I'll submit a new patch.

For the limit value, my guess was that when always using 254 the reserved characters for siginfo are not taken into account when siginfo=False. For shorter filenames it's ensured that the siginfo file is always the sstate file's name plus ".siginfo". With longer filenames it's possible that the basename of both files is different. This comes unhandy when working with the files outside of bitbake, e.g. running maintenance tasks on the sstate mirror server, e.g. copy sstate files and ensure their respective .siginfo file is also copied.

[-- Attachment #2: Type: text/html, Size: 2661 bytes --]

Re: [OE-core] [PATCH] sstate: Consider .lock suffix for when loading sstate file from mirror

[Richard Purdie]

On Mon, 2021-11-01 at 04:09 -0700, Manuel Leonhardt wrote:
> On Mon, Nov 1, 2021 at 11:52 AM, Richard Purdie wrote:
> > On Mon, 2021-11-01 at 03:47 -0700, Manuel Leonhardt wrote:
> > > I submitted a patch to fix bb.utils.lockfile to at least break the loop
> > > and
> > > fail
> > > when a too long filename is passed with
> > > https://lists.openembedded.org/g/bitbake-devel/message/12850
> > > 
> > > Nevertheless, you have a good point here: Fixing the code that constructs
> > > the
> > > filename of the lock files would be the better solution. For the sstate
> > > files
> > > only lockfiles of .siginfo files can overlap, because the filename of the
> > > actual
> > > sstate file is already 8 characters shorter than 255 characters. I have
> > > one
> > > though on that:
> > > 
> > > From what I have seen, the filename of lockfiles is mostly constructed by
> > > appending ".lock" without any further check. Truncating the filename of
> > > the
> > > lockfile inside bb.utils.lockfile could be confusing however, since the
> > > function
> > > would not use the filename that is was explicitly passed. Hence, I guess
> > > bb.utils.lockfile should fail when a too long filename was passed, and the
> > > function building the lockfile's filename should ensure to keep it
> > > limited;
> > > that's bb.fetch2.FetchData.__init__ then if I'm not mistaken. Would you
> > > agree?
> > > Or would you say, changing the filename inside bb.utils.lockfile is ok, as
> > > long
> > > as the function is working?
> > > 
> > > The drive-by fix I made with using limit instead of 254 when shortening
> > > the
> > > filename is still correct and necessary, right?
> > I think we just let lockfile truncate the filename if necessary since this
> > could
> > affect other users of the funciton too. 
> > 
> > Looking at the code you changed with the limit value it isn't correct since
> > extension is already accounted for in avail (although I agree that code is
> > confusing and I had to think twice about it).
> > 
> > Cheers,
> > 
> > Richard
> Ok, I'll submit a new patch.
> 
> For the limit value, my guess was that when always using 254 the reserved
> characters for siginfo are not taken into account when siginfo=False. For
> shorter filenames it's ensured that the siginfo file is always the sstate
> file's
> name plus ".siginfo". With longer filenames it's possible that the basename of
> both files is different. This comes unhandy when working with the files
> outside
> of bitbake, e.g. running maintenance tasks on the sstate mirror server, e.g.
> copy sstate files and ensure their respective .siginfo file is also copied.

That is reasonable, the commit message just needs to say that is why it is being
changed in that case.

Cheers,

Richard

Re: [PATCH] sstate: Consider .lock suffix for when loading sstate file from mirror

[Manuel Leonhardt]

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

On Mon, Nov 1, 2021 at 12:19 PM, Richard Purdie wrote:

> 
> On Mon, 2021-11-01 at 04:09 -0700, Manuel Leonhardt wrote:
> 
>> On Mon, Nov 1, 2021 at 11:52 AM, Richard Purdie wrote:
>> 
>>> On Mon, 2021-11-01 at 03:47 -0700, Manuel Leonhardt wrote:
>>> 
>>>> I submitted a patch to fix bb.utils.lockfile to at least break the loop
>>>> and
>>>> fail
>>>> when a too long filename is passed with
>>>> https://lists.openembedded.org/g/bitbake-devel/message/12850
>>>> 
>>>> Nevertheless, you have a good point here: Fixing the code that constructs
>>>> the
>>>> filename of the lock files would be the better solution. For the sstate
>>>> files
>>>> only lockfiles of .siginfo files can overlap, because the filename of the
>>>> actual
>>>> sstate file is already 8 characters shorter than 255 characters. I have
>>>> one
>>>> though on that:
>>>> 
>>>> From what I have seen, the filename of lockfiles is mostly constructed by
>>>> appending ".lock" without any further check. Truncating the filename of
>>>> the
>>>> lockfile inside bb.utils.lockfile could be confusing however, since the
>>>> function
>>>> would not use the filename that is was explicitly passed. Hence, I guess
>>>> bb.utils.lockfile should fail when a too long filename was passed, and the
>>>> 
>>>> function building the lockfile's filename should ensure to keep it
>>>> limited;
>>>> that's bb.fetch2.FetchData.__init__ then if I'm not mistaken. Would you
>>>> agree?
>>>> Or would you say, changing the filename inside bb.utils.lockfile is ok, as
>>>> 
>>>> long
>>>> as the function is working?
>>>> 
>>>> The drive-by fix I made with using limit instead of 254 when shortening
>>>> the
>>>> filename is still correct and necessary, right?
>>> 
>>> I think we just let lockfile truncate the filename if necessary since this
>>> 
>>> could
>>> affect other users of the funciton too.
>>> 
>>> Looking at the code you changed with the limit value it isn't correct
>>> since
>>> extension is already accounted for in avail (although I agree that code is
>>> 
>>> confusing and I had to think twice about it).
>>> 
>>> Cheers,
>>> 
>>> Richard
>> 
>> Ok, I'll submit a new patch.
>> 
>> For the limit value, my guess was that when always using 254 the reserved
>> characters for siginfo are not taken into account when siginfo=False. For
>> shorter filenames it's ensured that the siginfo file is always the sstate
>> file's
>> name plus ".siginfo". With longer filenames it's possible that the
>> basename of
>> both files is different. This comes unhandy when working with the files
>> outside
>> of bitbake, e.g. running maintenance tasks on the sstate mirror server,
>> e.g.
>> copy sstate files and ensure their respective .siginfo file is also
>> copied.
> 
> That is reasonable, the commit message just needs to say that is why it is
> being
> changed in that case.
> 
> Cheers,
> 
> Richard

Thanks. You ware totally right, I missed that in the commit message. I'll submit a new patch for that as well.

Best,

Manuel

[-- Attachment #2: Type: text/html, Size: 3218 bytes --]