Re: [PATCH 1/1] fetch2: Create/replace symbolic links atomically

[Richard Purdie <richard.purdie@linuxfoundation.org>]

On Tue, 2017-03-28 at 14:30 +0200, Peter Kjellerstedt wrote:
> Under rare circumstances, creating symbolic links could fail because
> the link already exists. At first glance the code seemed to protect
> against this, but there was a small window where two separate tasks
> could decide that a symbolic link needed to be created and then the
> first task would create it and the second task would fail.
> 
> Signed-off-by: Peter Kjellerstedt <peter.kjellerstedt@axis.com>
> ---
>  bitbake/lib/bb/fetch2/__init__.py | 20 +++++++++++++-------
>  1 file changed, 13 insertions(+), 7 deletions(-)

As Ross says, this needs to go to the bitbake list. I'm also curious
why not watch for the "AlreadyExists" exception (whatever it really is)
and then simply pass over the error assuming the symlink points to the
same place? That saves messing with temporary files.

I also worry a little about the locking? Why have you two fetch
processes which are changing the same files but not covered by the same
lock file? :/

Cheers,

Richard

> diff --git a/bitbake/lib/bb/fetch2/__init__.py
> b/bitbake/lib/bb/fetch2/__init__.py
> index 464e66b98a..f891e34eab 100644
> --- a/bitbake/lib/bb/fetch2/__init__.py
> +++ b/bitbake/lib/bb/fetch2/__init__.py
> @@ -25,7 +25,7 @@ BitBake build tools.
>  #
>  # Based on functions from the base bb module, Copyright 2003 Holger
> Schurig
>  
> -import os, re
> +import os, re, tempfile
>  import signal
>  import logging
>  import urllib.request, urllib.parse, urllib.error
> @@ -946,6 +946,16 @@ def rename_bad_checksum(ud, suffix):
>      bb.warn("Renaming %s to %s" % (ud.localpath, new_localpath))
>      bb.utils.movefile(ud.localpath, new_localpath)
>  
> +def atomic_symlink(src, dst):
> +    """Create/replace a symbolic link atomically."""
> +
> +    tmpname = tempfile.mktemp(prefix=dst)
> +    os.symlink(src, tmpname)
> +    try:
> +        os.rename(tmpname, dst)
> +    except OSError:
> +        os.remove(tmpname)
> +        raise
>  
>  def try_mirror_url(fetch, origud, ud, ld, check = False):
>      # Return of None or a value means we're finished
> @@ -983,7 +993,7 @@ def try_mirror_url(fetch, origud, ud, ld, check =
> False):
>                  open(ud.donestamp, 'w').close()
>              dest = os.path.join(dldir,
> os.path.basename(ud.localpath))
>              if not os.path.exists(dest):
> -                os.symlink(ud.localpath, dest)
> +                atomic_symlink(ud.localpath, dest)
>              if not verify_donestamp(origud, ld) or
> origud.method.need_update(origud, ld):
>                  origud.method.download(origud, ld)
>                  if hasattr(origud.method,"build_mirror_data"):
> @@ -991,11 +1001,7 @@ def try_mirror_url(fetch, origud, ud, ld, check
> = False):
>              return origud.localpath
>          # Otherwise the result is a local file:// and we symlink to
> it
>          if not os.path.exists(origud.localpath):
> -            if os.path.islink(origud.localpath):
> -                # Broken symbolic link
> -                os.unlink(origud.localpath)
> -
> -            os.symlink(ud.localpath, origud.localpath)
> +            atomic_symlink(ud.localpath, origud.localpath)
>          update_stamp(origud, ld)
>          return ud.localpath
>  
> -- 
> 2.12.0
>