[libvirt PATCH] scripts: Fix meson-install-symlink.py overwriting existing links

Erik Skultety posted 1 patch 3 years, 7 months ago
Test syntax-check failed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/6bb8c51baba1d90412e6e201f5e7d2b9967504cf.1596558409.git.eskultet@redhat.com
There is a newer version of this series
scripts/meson-install-symlink.py | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
[libvirt PATCH] scripts: Fix meson-install-symlink.py overwriting existing links
Posted by Erik Skultety 3 years, 7 months ago
By default, symlink re-creation fails if the link already exists, more
specifically in case of meson-install-symlink.py:

Traceback (most recent call last):
  File "/<path_to_libvirt_repo>/scripts/meson-install-symlink.py",
    line 15, in <module>
        os.symlink(target, link)
FileExistsError: File exists: '../default.xml' -> 'default.xml'

Unfortunately, Python can't mimic "ln -sf", so we have to fix this
differently - create a temporary name which is then going to be used
for the temporary link followed by a rename with the original link's
name.
Note that this solution is racy as mktemp() doesn't guarantee
atomicity in link creation, so theoretically another process could come
and create a file with the same name as the temporary link name, but
a proper solution would be longer and not as elegant.

Signed-off-by: Erik Skultety <eskultet@redhat.com>
---
 scripts/meson-install-symlink.py | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/scripts/meson-install-symlink.py b/scripts/meson-install-symlink.py
index e38507072d..630dfc079f 100644
--- a/scripts/meson-install-symlink.py
+++ b/scripts/meson-install-symlink.py
@@ -2,6 +2,7 @@
 
 import os
 import sys
+import tempfile
 
 destdir = os.environ.get('DESTDIR', os.sep)
 dirname = sys.argv[1]
@@ -12,4 +13,7 @@ workdir = os.path.join(destdir, dirname.strip(os.sep))
 
 os.makedirs(workdir, exist_ok=True)
 os.chdir(workdir)
-os.symlink(target, link)
+
+templink = tempfile.mktemp(dir=workdir)
+os.symlink(target, templink)
+os.replace(templink, link)
-- 
2.26.2

Re: [libvirt PATCH] scripts: Fix meson-install-symlink.py overwriting existing links
Posted by Martin Kletzander 3 years, 7 months ago
On Tue, Aug 04, 2020 at 06:27:08PM +0200, Erik Skultety wrote:
>By default, symlink re-creation fails if the link already exists, more
>specifically in case of meson-install-symlink.py:
>
>Traceback (most recent call last):
>  File "/<path_to_libvirt_repo>/scripts/meson-install-symlink.py",
>    line 15, in <module>
>        os.symlink(target, link)
>FileExistsError: File exists: '../default.xml' -> 'default.xml'
>
>Unfortunately, Python can't mimic "ln -sf", so we have to fix this
>differently - create a temporary name which is then going to be used
>for the temporary link followed by a rename with the original link's
>name.
>Note that this solution is racy as mktemp() doesn't guarantee
>atomicity in link creation, so theoretically another process could come
>and create a file with the same name as the temporary link name, but
>a proper solution would be longer and not as elegant.
>

Well, you could do subprocess.check_output(['ln', '-sf', ...]) (yuck), but it
does essentially the same thing anyway.

I don't think a race here would cause anything.  I would probably remove the
file and then create the symlink and if there is something/someone else
installing the same file than it is good that we error our because there are
bigger issues than that.  The whole installation process should not be
interfered with and you cannot make it atomic anyway.

Thoughts?

>Signed-off-by: Erik Skultety <eskultet@redhat.com>
>---
> scripts/meson-install-symlink.py | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
>diff --git a/scripts/meson-install-symlink.py b/scripts/meson-install-symlink.py
>index e38507072d..630dfc079f 100644
>--- a/scripts/meson-install-symlink.py
>+++ b/scripts/meson-install-symlink.py
>@@ -2,6 +2,7 @@
>
> import os
> import sys
>+import tempfile
>
> destdir = os.environ.get('DESTDIR', os.sep)
> dirname = sys.argv[1]
>@@ -12,4 +13,7 @@ workdir = os.path.join(destdir, dirname.strip(os.sep))
>
> os.makedirs(workdir, exist_ok=True)
> os.chdir(workdir)
>-os.symlink(target, link)
>+
>+templink = tempfile.mktemp(dir=workdir)
>+os.symlink(target, templink)
>+os.replace(templink, link)
>-- 
>2.26.2
>
Re: [libvirt PATCH] scripts: Fix meson-install-symlink.py overwriting existing links
Posted by Peter Krempa 3 years, 7 months ago
On Tue, Aug 04, 2020 at 19:48:15 +0200, Martin Kletzander wrote:
> On Tue, Aug 04, 2020 at 06:27:08PM +0200, Erik Skultety wrote:
> > By default, symlink re-creation fails if the link already exists, more
> > specifically in case of meson-install-symlink.py:
> > 
> > Traceback (most recent call last):
> >  File "/<path_to_libvirt_repo>/scripts/meson-install-symlink.py",
> >    line 15, in <module>
> >        os.symlink(target, link)
> > FileExistsError: File exists: '../default.xml' -> 'default.xml'
> > 
> > Unfortunately, Python can't mimic "ln -sf", so we have to fix this
> > differently - create a temporary name which is then going to be used
> > for the temporary link followed by a rename with the original link's
> > name.
> > Note that this solution is racy as mktemp() doesn't guarantee
> > atomicity in link creation, so theoretically another process could come
> > and create a file with the same name as the temporary link name, but
> > a proper solution would be longer and not as elegant.
> > 
> 
> Well, you could do subprocess.check_output(['ln', '-sf', ...]) (yuck), but it
> does essentially the same thing anyway.

In that case, we should throw away python and stick to the shell script
Pavel had before. This python is just a vanity thing to have less shell
scripts, but doesn't really make much sense in this case.

> I don't think a race here would cause anything.  I would probably remove the
> file and then create the symlink and if there is something/someone else
> installing the same file than it is good that we error our because there are
> bigger issues than that.  The whole installation process should not be
> interfered with and you cannot make it atomic anyway.
> 
> Thoughts?

I agree with deleting anything pre-existing first.

Re: [libvirt PATCH] scripts: Fix meson-install-symlink.py overwriting existing links
Posted by Erik Skultety 3 years, 7 months ago
On Tue, Aug 04, 2020 at 07:48:15PM +0200, Martin Kletzander wrote:
> On Tue, Aug 04, 2020 at 06:27:08PM +0200, Erik Skultety wrote:
> > By default, symlink re-creation fails if the link already exists, more
> > specifically in case of meson-install-symlink.py:
> > 
> > Traceback (most recent call last):
> >  File "/<path_to_libvirt_repo>/scripts/meson-install-symlink.py",
> >    line 15, in <module>
> >        os.symlink(target, link)
> > FileExistsError: File exists: '../default.xml' -> 'default.xml'
> > 
> > Unfortunately, Python can't mimic "ln -sf", so we have to fix this
> > differently - create a temporary name which is then going to be used
> > for the temporary link followed by a rename with the original link's
> > name.
> > Note that this solution is racy as mktemp() doesn't guarantee
> > atomicity in link creation, so theoretically another process could come
> > and create a file with the same name as the temporary link name, but
> > a proper solution would be longer and not as elegant.
> > 
> 
> Well, you could do subprocess.check_output(['ln', '-sf', ...]) (yuck), but it
> does essentially the same thing anyway.

Well, obviously I could issue a call to subprocess, but...yes, yuck.

> 
> I don't think a race here would cause anything.  I would probably remove the
> file and then create the symlink and if there is something/someone else
> installing the same file than it is good that we error our because there are
> bigger issues than that.  The whole installation process should not be
> interfered with and you cannot make it atomic anyway.

I agree with your thinking I just found it important to underline the fact that
the solution isn't 100% correct, but like you said, we should not care that
much during the installation phase, it would only point out a bigger issue.

Erik

Re: [libvirt PATCH] scripts: Fix meson-install-symlink.py overwriting existing links
Posted by Daniel P. Berrangé 3 years, 7 months ago
On Tue, Aug 04, 2020 at 06:27:08PM +0200, Erik Skultety wrote:
> By default, symlink re-creation fails if the link already exists, more
> specifically in case of meson-install-symlink.py:
> 
> Traceback (most recent call last):
>   File "/<path_to_libvirt_repo>/scripts/meson-install-symlink.py",
>     line 15, in <module>
>         os.symlink(target, link)
> FileExistsError: File exists: '../default.xml' -> 'default.xml'
> 
> Unfortunately, Python can't mimic "ln -sf", so we have to fix this
> differently - create a temporary name which is then going to be used
> for the temporary link followed by a rename with the original link's
> name.
> Note that this solution is racy as mktemp() doesn't guarantee
> atomicity in link creation, so theoretically another process could come
> and create a file with the same name as the temporary link name, but
> a proper solution would be longer and not as elegant.

You've just described exactly what python does for "ln -sf foo bar".
try stracing it and you'll see it do:

  symlinkat("foo", AT_FDCWD, "CubmxbFT")  = 0
  renameat(AT_FDCWD, "CubmxbFT", AT_FDCWD, "bar") = 0



Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [libvirt PATCH] scripts: Fix meson-install-symlink.py overwriting existing links
Posted by Erik Skultety 3 years, 7 months ago
On Wed, Aug 05, 2020 at 09:57:17AM +0100, Daniel P. Berrangé wrote:
> On Tue, Aug 04, 2020 at 06:27:08PM +0200, Erik Skultety wrote:
> > By default, symlink re-creation fails if the link already exists, more
> > specifically in case of meson-install-symlink.py:
> > 
> > Traceback (most recent call last):
> >   File "/<path_to_libvirt_repo>/scripts/meson-install-symlink.py",
> >     line 15, in <module>
> >         os.symlink(target, link)
> > FileExistsError: File exists: '../default.xml' -> 'default.xml'
> > 
> > Unfortunately, Python can't mimic "ln -sf", so we have to fix this
> > differently - create a temporary name which is then going to be used
> > for the temporary link followed by a rename with the original link's
> > name.
> > Note that this solution is racy as mktemp() doesn't guarantee
> > atomicity in link creation, so theoretically another process could come
> > and create a file with the same name as the temporary link name, but
> > a proper solution would be longer and not as elegant.
> 
> You've just described exactly what python does for "ln -sf foo bar".
> try stracing it and you'll see it do:
> 
>   symlinkat("foo", AT_FDCWD, "CubmxbFT")  = 0
>   renameat(AT_FDCWD, "CubmxbFT", AT_FDCWD, "bar") = 0

Oh, I didn't know that, in which case v1 is actually the correct fix here. I
sent v2 in the meantime where I remove the pre-existing link which is even more
compact, so I don't care which one we go for.

Erik