[PATCH] state/staging: Fix directory not deleted for race

[PATCH] state/staging: Fix directory not deleted for race

[Pgowda]

The race issue resulted in some empty directories not being deleted.
During staging_copyfile, it results in "file already exists error" on
creating link for directories during do_prepare_recipe_sysroot.
The following patch checks whether the directory and its sub directories
have no files in it and removes them.

The issue was observed on running rust-hello-world for glibc and musl
one after the other.
TCLIBC=glibc MACHINE=qemuarm bitbake rust-hello-world
TCLIBC=musl MACHINE=qemuarm bitbake rust-hello-world

On fixing the above issue, it was found that
arm-poky-linux-musleabi.json files are not being generated.
$/del/build/tmp/work/x86_64-linux/rust-cross-arm/1.54.0-r0/targets/
arm-poky-linux-musleabi.json': No such file or directory

It is generated and builds successfully when musl lib is built standalone.
However, when musl is built after glibc or viceversa results in the
latter json file not being generated.

Signed-off-by: Pgowda <pgowda.cve@gmail.com>
---
 meta/classes/staging.bbclass | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/meta/classes/staging.bbclass b/meta/classes/staging.bbclass
index af3397bab6..4b751af4d3 100644
--- a/meta/classes/staging.bbclass
+++ b/meta/classes/staging.bbclass
@@ -145,6 +145,16 @@ def staging_copyfile(c, target, dest, postinsts, seendirs):
     if os.path.islink(c):
         linkto = os.readlink(c)
         if os.path.lexists(dest):
+            # Check if directory is not deleted for race issue
+            if os.path.isdir(dest):
+                count = 0
+                for root, dirs, files in os.walk(dest):
+                    for Files in files:
+                        count += 1
+                if count == 0:
+                    import shutil
+                    shutil.rmtree(dest)
+                    os.symlink(linkto, dest)
             if not os.path.islink(dest):
                 raise OSError(errno.EEXIST, "Link %s already exists as a file" % dest, dest)
             if os.readlink(dest) == linkto:
-- 
2.31.1

Re: [PATCH] state/staging: Fix directory not deleted for race

[Richard Purdie]

On Wed, 2021-09-15 at 04:46 -0700, Pgowda wrote:
> The race issue resulted in some empty directories not being deleted.
> During staging_copyfile, it results in "file already exists error" on
> creating link for directories during do_prepare_recipe_sysroot.
> The following patch checks whether the directory and its sub directories
> have no files in it and removes them.
> 
> The issue was observed on running rust-hello-world for glibc and musl
> one after the other.
> TCLIBC=glibc MACHINE=qemuarm bitbake rust-hello-world
> TCLIBC=musl MACHINE=qemuarm bitbake rust-hello-world
> 
> On fixing the above issue, it was found that
> arm-poky-linux-musleabi.json files are not being generated.
> $/del/build/tmp/work/x86_64-linux/rust-cross-arm/1.54.0-r0/targets/
> arm-poky-linux-musleabi.json': No such file or directory
> 
> It is generated and builds successfully when musl lib is built standalone.
> However, when musl is built after glibc or viceversa results in the
> latter json file not being generated.
> 
> Signed-off-by: Pgowda <pgowda.cve@gmail.com>
> ---
>  meta/classes/staging.bbclass | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/meta/classes/staging.bbclass b/meta/classes/staging.bbclass
> index af3397bab6..4b751af4d3 100644
> --- a/meta/classes/staging.bbclass
> +++ b/meta/classes/staging.bbclass
> @@ -145,6 +145,16 @@ def staging_copyfile(c, target, dest, postinsts, seendirs):
>      if os.path.islink(c):
>          linkto = os.readlink(c)
>          if os.path.lexists(dest):
> +            # Check if directory is not deleted for race issue
> +            if os.path.isdir(dest):
> +                count = 0
> +                for root, dirs, files in os.walk(dest):
> +                    for Files in files:
> +                        count += 1
> +                if count == 0:
> +                    import shutil
> +                    shutil.rmtree(dest)
> +                    os.symlink(linkto, dest)
>              if not os.path.islink(dest):
>                  raise OSError(errno.EEXIST, "Link %s already exists as a file" % dest, dest)
>              if os.readlink(dest) == linkto:

This change looks like a good fix but it can be improved a little.

Firstly, rather than the count and os.walk, we can just use
"if not os.listdir(dest):".

Secondly, if we know the directory is empty and is a directory, we don' need
rmtree, we can just use os.rmdir() which should be much faster.

I'd also clarify the comment to something like:

# Empty directories are not removed due to race issues but if we
# find a conflict, we can remove empty directories here.

Cheers,

Richard

Re: [OE-core] [PATCH] state/staging: Fix directory not deleted for race

[Richard Purdie]

On Thu, 2021-09-16 at 08:45 +0100, Richard Purdie via lists.openembedded.org
wrote:
> On Wed, 2021-09-15 at 04:46 -0700, Pgowda wrote:
> > The race issue resulted in some empty directories not being deleted.
> > During staging_copyfile, it results in "file already exists error" on
> > creating link for directories during do_prepare_recipe_sysroot.
> > The following patch checks whether the directory and its sub directories
> > have no files in it and removes them.
> > 
> > The issue was observed on running rust-hello-world for glibc and musl
> > one after the other.
> > TCLIBC=glibc MACHINE=qemuarm bitbake rust-hello-world
> > TCLIBC=musl MACHINE=qemuarm bitbake rust-hello-world
> > 
> > On fixing the above issue, it was found that
> > arm-poky-linux-musleabi.json files are not being generated.
> > $/del/build/tmp/work/x86_64-linux/rust-cross-arm/1.54.0-r0/targets/
> > arm-poky-linux-musleabi.json': No such file or directory
> > 
> > It is generated and builds successfully when musl lib is built standalone.
> > However, when musl is built after glibc or viceversa results in the
> > latter json file not being generated.
> > 
> > Signed-off-by: Pgowda <pgowda.cve@gmail.com>
> > ---
> >  meta/classes/staging.bbclass | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> > 
> > diff --git a/meta/classes/staging.bbclass b/meta/classes/staging.bbclass
> > index af3397bab6..4b751af4d3 100644
> > --- a/meta/classes/staging.bbclass
> > +++ b/meta/classes/staging.bbclass
> > @@ -145,6 +145,16 @@ def staging_copyfile(c, target, dest, postinsts, seendirs):
> >      if os.path.islink(c):
> >          linkto = os.readlink(c)
> >          if os.path.lexists(dest):
> > +            # Check if directory is not deleted for race issue
> > +            if os.path.isdir(dest):
> > +                count = 0
> > +                for root, dirs, files in os.walk(dest):
> > +                    for Files in files:
> > +                        count += 1
> > +                if count == 0:
> > +                    import shutil
> > +                    shutil.rmtree(dest)
> > +                    os.symlink(linkto, dest)
> >              if not os.path.islink(dest):
> >                  raise OSError(errno.EEXIST, "Link %s already exists as a file" % dest, dest)
> >              if os.readlink(dest) == linkto:
> 
> This change looks like a good fix but it can be improved a little.
> 
> Firstly, rather than the count and os.walk, we can just use
> "if not os.listdir(dest):".
> 
> Secondly, if we know the directory is empty and is a directory, we don' need
> rmtree, we can just use os.rmdir() which should be much faster.
> 
> I'd also clarify the comment to something like:
> 
> # Empty directories are not removed due to race issues but if we
> # find a conflict, we can remove empty directories here.

Looking further at this and your description, I think you might be saying there
are multiple levels of empty directories. If that is the case my simplification
may not work but I am more worried about why we have multiple levels of this
issue.

I would also like to understand why these files overlap and are symlinks in one
case but directories in another. Can you show some specific examples of the
files in question?

The missing json file sounds like there could be some file not being handled by
sstate but it is hard to know without looking at the code.

Cheers,

Richard

Re: [OE-core] [PATCH] state/staging: Fix directory not deleted for race

[Pgowda]

Hi Richard,

Thanks for reviewing the patch and your comments.

> Looking further at this and your description, I think you might be saying there
> are multiple levels of empty directories.

Yes. There are multiple level of directories as follows:-

/tmp/work/x86_64-linux/rust-cross-x86_64/1.54.0-r0/recipe-sysroot\
/usr/include/c++/11.2.0/x86_64-poky-linux/ext
/tmp/work/x86_64-linux/rust-cross-x86_64/1.54.0-r0/recipe-sysroot\
/usr/include/c++/11.2.0/x86_64-poky-linux/bits
/tmp/work/x86_64-linux/rust-cross-x86_64/1.54.0-r0/recipe-sysroot\
/usr/include/c++/11.2.0/x86_64-poky-linux

> I would also like to understand why these files overlap and are symlinks in one
> case but directories in another. Can you show some specific examples of the
> files in question?

It happens when we build rust-hello-world for glibc followed by musl
as follows:-
TCLIBC=glibc MACHINE=qemuarm bitbake rust-hello-world
TCLIBC=musl MACHINE=qemuarm bitbake rust-hello-world

The recipe do_prepare_recipe_sysroot tries to populate the sysroot file.
The glibc build had already generated the sysroot and copied properly.
When we build the musl later on, it tries to remove the above
directory and symlink
the newly built musl files. It was deleted properly before the race
issue was observed.
However, after the race issue; files are being deleted but the
directory and sub directories
are left undeleted which resulted in the error.

> The missing json file sounds like there could be some file not being handled by
> sstate but it is hard to know without looking at the code.

Thanks. I will try to debug the issue further.

Regards,
pgowda

Re: [OE-core] [PATCH] state/staging: Fix directory not deleted for race

[Randy MacLeod]

Add Khem (aka Mr Musl). ;-)

On 2021-09-16 4:28 a.m., Pgowda wrote:
> Hi Richard,
> 
> Thanks for reviewing the patch and your comments.
> 
>> Looking further at this and your description, I think you might be saying there
>> are multiple levels of empty directories.
> 
> Yes. There are multiple level of directories as follows:-
> 
> /tmp/work/x86_64-linux/rust-cross-x86_64/1.54.0-r0/recipe-sysroot\
> /usr/include/c++/11.2.0/x86_64-poky-linux/ext
> /tmp/work/x86_64-linux/rust-cross-x86_64/1.54.0-r0/recipe-sysroot\
> /usr/include/c++/11.2.0/x86_64-poky-linux/bits
> /tmp/work/x86_64-linux/rust-cross-x86_64/1.54.0-r0/recipe-sysroot\
> /usr/include/c++/11.2.0/x86_64-poky-linux
> 
>> I would also like to understand why these files overlap and are symlinks in one
>> case but directories in another. Can you show some specific examples of the
>> files in question?
> 
> It happens when we build rust-hello-world for glibc followed by musl
> as follows:-
> TCLIBC=glibc MACHINE=qemuarm bitbake rust-hello-world
> TCLIBC=musl MACHINE=qemuarm bitbake rust-hello-world
> 
> The recipe do_prepare_recipe_sysroot tries to populate the sysroot file.
> The glibc build had already generated the sysroot and copied properly.
> When we build the musl later on, it tries to remove the above
> directory and symlink
> the newly built musl files. It was deleted properly before the race
> issue was observed.
> However, after the race issue; files are being deleted but the
> directory and sub directories
> are left undeleted which resulted in the error.


All of that makes sense to me based on first reading it but I'm not
intimately familiar with toolchain sysroot setup.

> 
>> The missing json file sounds like there could be some file not being handled by
>> sstate but it is hard to know without looking at the code.
> 
> Thanks. I will try to debug the issue further.


So do you have additional information Naveen or
do you maintain that the fix presented on this thread is all that is needed.

../Randy

> 
> Regards,
> pgowda
> 
> 
> 
> -=-=-=-=-=-=-=-=-=-=-=-
> Links: You receive all messages sent to this group.
> View/Reply Online (#156105): https://lists.openembedded.org/g/openembedded-core/message/156105
> Mute This Topic: https://lists.openembedded.org/mt/85625195/3616765
> Group Owner: openembedded-core+owner@lists.openembedded.org
> Unsubscribe: https://lists.openembedded.org/g/openembedded-core/unsub [randy.macleod@windriver.com]
> -=-=-=-=-=-=-=-=-=-=-=-
> 


-- 
# Randy MacLeod
# Wind River Linux

Re: [OE-core] [PATCH] state/staging: Fix directory not deleted for race

[Richard Purdie]

On Thu, 2021-09-16 at 13:58 +0530, pgowda cve wrote:
> Hi Richard,
> 
> Thanks for reviewing the patch and your comments.
> 
> > Looking further at this and your description, I think you might be saying there
> > are multiple levels of empty directories.
> 
> Yes. There are multiple level of directories as follows:-
> 
> /tmp/work/x86_64-linux/rust-cross-x86_64/1.54.0-r0/recipe-sysroot\
> /usr/include/c++/11.2.0/x86_64-poky-linux/ext
> /tmp/work/x86_64-linux/rust-cross-x86_64/1.54.0-r0/recipe-sysroot\
> /usr/include/c++/11.2.0/x86_64-poky-linux/bits
> /tmp/work/x86_64-linux/rust-cross-x86_64/1.54.0-r0/recipe-sysroot\
> /usr/include/c++/11.2.0/x86_64-poky-linux
> 
> > I would also like to understand why these files overlap and are symlinks in one
> > case but directories in another. Can you show some specific examples of the
> > files in question?
> 
> It happens when we build rust-hello-world for glibc followed by musl
> as follows:-
> TCLIBC=glibc MACHINE=qemuarm bitbake rust-hello-world
> TCLIBC=musl MACHINE=qemuarm bitbake rust-hello-world
> 
> The recipe do_prepare_recipe_sysroot tries to populate the sysroot file.
> The glibc build had already generated the sysroot and copied properly.
> When we build the musl later on, it tries to remove the above
> directory and symlink
> the newly built musl files. It was deleted properly before the race
> issue was observed.
> However, after the race issue; files are being deleted but the
> directory and sub directories
> are left undeleted which resulted in the error.
> 
> > The missing json file sounds like there could be some file not being handled by
> > sstate but it is hard to know without looking at the code.
> 
> Thanks. I will try to debug the issue further.

I did think about this a bit further. "cross" recipes are hard and I suspect in
this case we may need to account for musl/glibc in the recipe name or it's
workdir if it is going to be "picky" like this.

Cheers,

Richard

[PATCH v2] state/staging: Fix directory not deleted for race

[Pgowda]

Modify the SYSROOT PATH to fix race around issue when rust-hello-world
is run for two libc one after the other.

Signed-off-by: Pgowda <pgowda.cve@gmail.com>
---
 meta/recipes-devtools/rust/rust-cross.inc | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/meta/recipes-devtools/rust/rust-cross.inc b/meta/recipes-devtools/rust/rust-cross.inc
index bee7c9f12f..09d0078d17 100644
--- a/meta/recipes-devtools/rust/rust-cross.inc
+++ b/meta/recipes-devtools/rust/rust-cross.inc
@@ -32,7 +32,17 @@ DEPENDS += "virtual/${TARGET_PREFIX}gcc virtual/${TARGET_PREFIX}compilerlibs vir
 DEPENDS += "rust-native"
 
 PROVIDES = "virtual/${TARGET_PREFIX}rust"
-PN = "rust-cross-${TARGET_ARCH}"
+
+# Modify the sysroot PATH based on the libc used to fix race around issue.
+def get_libc(d):
+    tclibc = d.getVar('TCLIBC')
+    if tclibc == 'glibc':
+        pn = "rust-cross-${TARGET_ARCH}"
+    else:
+        pn = "rust-cross-${TARGET_ARCH}-${TCLIBC}"
+    return pn
+
+PN := "${@get_libc(d)}"
 
 # In the cross compilation case, rustc doesn't seem to get the rpath quite
 # right. It manages to include '../../lib/${TARGET_PREFIX}', but doesn't
-- 
2.31.1

Re: [OE-core] [PATCH] state/staging: Fix directory not deleted for race

[pgowda cve]

Hi Richard,

>> we may need to account for musl/glibc in the recipe name or it's
>> workdir if it is going to be "picky" like this.

Thanks for your suggestion on fixing this issue in a better way.

Please find the patch posted at the following link as per your suggestion and
let me know if it's fine.
https://lists.openembedded.org/g/openembedded-core/message/156513

Thanks,
pgowda

Re: [OE-core] [PATCH] state/staging: Fix directory not deleted for race

[pgowda cve]

Hi,

Gentle ping on this patch.

Thanks,
pgowda

On Fri, Oct 1, 2021 at 12:59 PM Pgowda via lists.openembedded.org
<pgowda.cve=gmail.com@lists.openembedded.org> wrote:
>
> Hi Richard,
>
> >> we may need to account for musl/glibc in the recipe name or it's
> >> workdir if it is going to be "picky" like this.
>
> Thanks for your suggestion on fixing this issue in a better way.
>
> Please find the patch posted at the following link as per your suggestion and
> let me know if it's fine.
> https://lists.openembedded.org/g/openembedded-core/message/156513
>
> Thanks,
> pgowda
>
> -=-=-=-=-=-=-=-=-=-=-=-
> Links: You receive all messages sent to this group.
> View/Reply Online (#156514): https://lists.openembedded.org/g/openembedded-core/message/156514
> Mute This Topic: https://lists.openembedded.org/mt/85625195/6402864
> Group Owner: openembedded-core+owner@lists.openembedded.org
> Unsubscribe: https://lists.openembedded.org/g/openembedded-core/unsub [pgowda.cve@gmail.com]
> -=-=-=-=-=-=-=-=-=-=-=-
>

Re: [PATCH v2] state/staging: Fix directory not deleted for race

[Richard Purdie]

On Fri, 2021-10-01 at 00:24 -0700, Pgowda wrote:
> Modify the SYSROOT PATH to fix race around issue when rust-hello-world
> is run for two libc one after the other.
> 
> Signed-off-by: Pgowda <pgowda.cve@gmail.com>
> ---
>  meta/recipes-devtools/rust/rust-cross.inc | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/meta/recipes-devtools/rust/rust-cross.inc b/meta/recipes-devtools/rust/rust-cross.inc
> index bee7c9f12f..09d0078d17 100644
> --- a/meta/recipes-devtools/rust/rust-cross.inc
> +++ b/meta/recipes-devtools/rust/rust-cross.inc
> @@ -32,7 +32,17 @@ DEPENDS += "virtual/${TARGET_PREFIX}gcc virtual/${TARGET_PREFIX}compilerlibs vir
>  DEPENDS += "rust-native"
>  
>  PROVIDES = "virtual/${TARGET_PREFIX}rust"
> -PN = "rust-cross-${TARGET_ARCH}"
> +
> +# Modify the sysroot PATH based on the libc used to fix race around issue.
> +def get_libc(d):
> +    tclibc = d.getVar('TCLIBC')
> +    if tclibc == 'glibc':
> +        pn = "rust-cross-${TARGET_ARCH}"
> +    else:
> +        pn = "rust-cross-${TARGET_ARCH}-${TCLIBC}"
> +    return pn
> +
> +PN := "${@get_libc(d)}"
>  

I'm wondering if we should just add TCLIBC to PN unconditionally? Do we need the
complexity of the conditional code?

Cheers,

Richard

Re: [PATCH v2] state/staging: Fix directory not deleted for race

[pgowda cve]

Hi Richard,

Thanks for the review and suggestion.

>> I'm wondering if we should just add TCLIBC to PN unconditionally?

I didn't want to add TCLIBC to the code when using default GLIBC.
Hence, I tried to keep the folder default for GLIBC.

However, if you suggest adding TCLIBC to PN unconditionally;
I will modify the patch and submit.

Thanks,
pgowda

Re: [PATCH v2] state/staging: Fix directory not deleted for race

[Richard Purdie]

On Mon, 2021-10-18 at 19:17 +0530, pgowda cve wrote:
> Hi Richard,
> 
> Thanks for the review and suggestion.
> 
> > > I'm wondering if we should just add TCLIBC to PN unconditionally?
> 
> I didn't want to add TCLIBC to the code when using default GLIBC.
> Hence, I tried to keep the folder default for GLIBC.
> 
> However, if you suggest adding TCLIBC to PN unconditionally;
> I will modify the patch and submit.

Lets try that please.

There is a related issue Martin reported where the recipe is really TUNE_ARCH
specific and not really TARGET_ARCH specific. One step at a time I guess though.

Cheers,

Richard

Re: [OE-core] [PATCH v2] state/staging: Fix directory not deleted for race

[Martin Jansa]

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

On Mon, Oct 18, 2021 at 4:52 PM Richard Purdie <
richard.purdie@linuxfoundation.org> wrote:

> On Mon, 2021-10-18 at 19:17 +0530, pgowda cve wrote:
> > Hi Richard,
> >
> > Thanks for the review and suggestion.
> >
> > > > I'm wondering if we should just add TCLIBC to PN unconditionally?
> >
> > I didn't want to add TCLIBC to the code when using default GLIBC.
> > Hence, I tried to keep the folder default for GLIBC.
> >
> > However, if you suggest adding TCLIBC to PN unconditionally;
> > I will modify the patch and submit.
>
> Lets try that please.
>
> There is a related issue Martin reported where the recipe is really
> TUNE_ARCH
> specific and not really TARGET_ARCH specific. One step at a time I guess
> though.
>

I think you were referring to:
https://lists.openembedded.org/g/openembedded-core/message/156516
and it's currently even MACHINE_ARCH due to poking in MACHINEOVERRIDES
variable, but in theory it should be possible to re-write the logic to be
only TUNE_ARCH specific.

Cheers,

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

Re: [PATCH v2] state/staging: Fix directory not deleted for race

[pgowda cve]

Hi Richard,

Thanks for the confirmation.

>> Let's try that please.

Please find the patch posted at the following link as per your suggestion.
https://lists.openembedded.org/g/openembedded-core/message/157103

>> There is a related issue Martin reported where the recipe is really TUNE_ARCH
>> specific and not really TARGET_ARCH specific.

I will check this issue.


Thanks,
pgowda