From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from aws-us-west-2-korg-lkml-1.web.codeaurora.org (localhost.localdomain [127.0.0.1]) by smtp.lore.kernel.org (Postfix) with ESMTP id 1DF1BC433F5 for ; Mon, 1 Nov 2021 11:21:59 +0000 (UTC) Subject: Re: [PATCH] sstate: Consider .lock suffix for when loading sstate file from mirror To: openembedded-core@lists.openembedded.org From: "Manuel Leonhardt" X-Originating-Location: Munich, Bavaria, DE (88.217.121.203) X-Originating-Platform: Mac Firefox 93 User-Agent: GROUPS.IO Web Poster MIME-Version: 1.0 Date: Mon, 01 Nov 2021 04:21:57 -0700 References: In-Reply-To: Message-ID: <27626.1635765717822776754@lists.openembedded.org> Content-Type: multipart/alternative; boundary="08Hd8pSDWyur8Bg4ICkv" List-Id: X-Webhook-Received: from li982-79.members.linode.com [45.33.32.79] by aws-us-west-2-korg-lkml-1.web.codeaurora.org with HTTPS for ; Mon, 01 Nov 2021 11:21:59 -0000 X-Groupsio-URL: https://lists.openembedded.org/g/openembedded-core/message/157703 --08Hd8pSDWyur8Bg4ICkv Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable On Mon, Nov 1, 2021 at 12:19 PM, Richard Purdie wrote: >=20 > On Mon, 2021-11-01 at 04:09 -0700, Manuel Leonhardt wrote: >=20 >> On Mon, Nov 1, 2021 at 11:52 AM, Richard Purdie wrote: >>=20 >>> On Mon, 2021-11-01 at 03:47 -0700, Manuel Leonhardt wrote: >>>=20 >>>> I submitted a patch to fix bb.utils.lockfile to at least break the loo= p >>>> and >>>> fail >>>> when a too long filename is passed with >>>> https://lists.openembedded.org/g/bitbake-devel/message/12850 >>>>=20 >>>> Nevertheless, you have a good point here: Fixing the code that constru= cts >>>> the >>>> filename of the lock files would be the better solution. For the sstat= e >>>> 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 hav= e >>>> one >>>> though on that: >>>>=20 >>>> From what I have seen, the filename of lockfiles is mostly constructed= by >>>> appending ".lock" without any further check. Truncating the filename o= f >>>> the >>>> lockfile inside bb.utils.lockfile could be confusing however, since th= e >>>> function >>>> would not use the filename that is was explicitly passed. Hence, I gue= ss >>>> bb.utils.lockfile should fail when a too long filename was passed, and= the >>>>=20 >>>> 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 yo= u >>>> agree? >>>> Or would you say, changing the filename inside bb.utils.lockfile is ok= , as >>>>=20 >>>> long >>>> as the function is working? >>>>=20 >>>> The drive-by fix I made with using limit instead of 254 when shortenin= g >>>> the >>>> filename is still correct and necessary, right? >>>=20 >>> I think we just let lockfile truncate the filename if necessary since t= his >>>=20 >>> could >>> affect other users of the funciton too. >>>=20 >>> 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 >>>=20 >>> confusing and I had to think twice about it). >>>=20 >>> Cheers, >>>=20 >>> Richard >>=20 >> Ok, I'll submit a new patch. >>=20 >> For the limit value, my guess was that when always using 254 the reserve= d >> characters for siginfo are not taken into account when siginfo=3DFalse. = For >> shorter filenames it's ensured that the siginfo file is always the sstat= e >> 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. >=20 > That is reasonable, the commit message just needs to say that is why it i= s > being > changed in that case. >=20 > Cheers, >=20 > Richard Thanks. You ware totally right, I missed that in the commit message. I'll s= ubmit a new patch for that as well. Best, Manuel --08Hd8pSDWyur8Bg4ICkv Content-Type: text/html; charset="utf-8" Content-Transfer-Encoding: quoted-printable 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 withhttps://lists.openembedded.org/g/bitbak= e-devel/message/12850

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

From what I have seen, the filena= me 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
wo= uld 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 itlimited;
that's bb.fetch2.FetchData.__init__ then if I'm not mistak= en. Would you
agree?
Or would you say, changing the filename insi= de 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 shor= tening
the
filename is still correct and necessary, right? I think we just let lockfile truncate the filename if necessary since this<= br />could
affect other users of the funciton too. 

Lo= oking at the code you changed with the limit value it isn't correct sinceextension 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 t= hat when always using 254 the reserved
characters for siginfo are not = taken into account when siginfo=3DFalse. For
shorter filenames it's en= sured 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 mir= ror server, e.g.
copy sstate files and ensure their respective .siginf= o 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 s= ubmit a new patch for that as well.

Best,

Manuel --08Hd8pSDWyur8Bg4ICkv--