[Buildroot] [PATCH 0/5] Cleanups in download helpers (branch yem/check-downloads)

[Buildroot] [PATCH 0/5] Cleanups in download helpers (branch yem/check-downloads)

[Yann E. MORIN]

Hello All!

This series is a follow-up to the previous download series, and brings in:
  - a fix for the bzr helper
  - fixes a potential race in parallel downloads
  - simplifies the localfiles helper
  - post-pones creating temp files/dirs only at the point they are needed
  - cleanups and rationalises the naming of temp files/dirs

Regards,
Yann E. MORIN.


The following changes since commit ea0f52fc3fca5e5ba3ea5cd46033df2f53e9b5a5:

  pkg-infra: do the package install before installing init files (2014-07-06 22:25:24 +0200)

are available in the git repository at:

  git://ymorin.is-a-geek.org/buildroot yem/check-downloads

for you to fetch changes up to 68fe3038dcc98bd803b0fe01a3d26cf95be97d4b:

  support/download: rationalise naming and use of the temporary files (2014-07-06 23:15:36 +0200)

----------------------------------------------------------------
Yann E. MORIN (5):
      support/download: fix the bzr helper
      support/download: properly use temp files
      support/download: simplify the local-files helper
      support/download: only create final temp file when needed
      support/download: rationalise naming and use of the temporary files

 Config.in               |  4 ----
 Config.in.legacy        | 11 +++++++++++
 package/pkg-download.mk |  1 -
 support/download/bzr    |  9 ++++-----
 support/download/cp     |  6 ++++--
 support/download/cvs    | 21 ++++++++++++---------
 support/download/git    | 17 ++++++++---------
 support/download/hg     | 11 +++--------
 support/download/scp    |  8 +++++---
 support/download/svn    | 14 +++++---------
 support/download/wget   |  7 +++----
 11 files changed, 55 insertions(+), 54 deletions(-)

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

[Buildroot] [PATCH 1/5] support/download: fix the bzr helper

[Yann E. MORIN]

bzr uses the name of the extension of the output file to known what
output format to use: tar, tgz, tar.bz2... If no extension is
recognised, bzr will output to a directory.

Since we use 'mktemp .XXXXXX' to generate temporary files, it obviously
never ends with a recognised extension. Thus, bzr expects the output to
be a directory, and fails since it is a file.

Fix that by forcing the output format.

Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
---
 support/download/bzr | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/support/download/bzr b/support/download/bzr
index 3f52ee9..19d837d 100755
--- a/support/download/bzr
+++ b/support/download/bzr
@@ -26,7 +26,7 @@ tmp_output="$( mktemp "${output}.XXXXXX" )"
 # - finally, we atomically rename to the final file
 
 ret=1
-if ${BZR} export "${tmp_dl}" "${repo}" -r "${rev}"; then
+if ${BZR} export --format=tgz "${tmp_dl}" "${repo}" -r "${rev}"; then
     if mv "${tmp_dl}" "${tmp_output}"; then
         mv "${tmp_output}" "${output}"
         ret=0
-- 
1.9.1

[Buildroot] [PATCH 2/5] support/download: properly use temp files

[Yann E. MORIN]

When using 'mv' to move files to temp files, the existing temp file is
forcibly removed by 'mv', and a new file is created.

Although this should have little impact to us, there is still a race
conditions in case two parallel downloads would step on each other's
toes, and it goes like that:

    Process 1                       Process 2
    mktemp tmp_output
    mv tmp_dl tmp_output
        removes tmp_output
                                    mktemp tmp_ouput
        writes to tmp_output
                                    mv tmp_dl tmp_output
                                        removes tmp_output
                                        writes to tmp_output
    mv tmp_output output
                                    mv tmp_output output

In this case, mktemp has the opportunity to create the same tmp_output
temporary file, and we trash the content from one with the content
from the other, and the last to do the final 'mv' breaks horibly.

Instead, use 'cat', which guarantees that tmp_output is not removed
before writing to it.

This complexifies a bit the local-files (cp) helper, but better safe
than sorry.

(Note: this is a purely theoretical issue, I did not observe it yet, but
it is slated to happen one day or the other, and will be very hard to
debug then.)

Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
---
 support/download/bzr  | 2 +-
 support/download/cp   | 9 ++++++---
 support/download/cvs  | 2 +-
 support/download/git  | 4 ++--
 support/download/hg   | 2 +-
 support/download/scp  | 2 +-
 support/download/svn  | 2 +-
 support/download/wget | 2 +-
 8 files changed, 14 insertions(+), 11 deletions(-)

diff --git a/support/download/bzr b/support/download/bzr
index 19d837d..f86fa82 100755
--- a/support/download/bzr
+++ b/support/download/bzr
@@ -27,7 +27,7 @@ tmp_output="$( mktemp "${output}.XXXXXX" )"
 
 ret=1
 if ${BZR} export --format=tgz "${tmp_dl}" "${repo}" -r "${rev}"; then
-    if mv "${tmp_dl}" "${tmp_output}"; then
+    if cat "${tmp_dl}" >"${tmp_output}"; then
         mv "${tmp_output}" "${output}"
         ret=0
     fi
diff --git a/support/download/cp b/support/download/cp
index 8f6bc06..e73159b 100755
--- a/support/download/cp
+++ b/support/download/cp
@@ -13,12 +13,15 @@ set -e
 source="${1}"
 output="${2}"
 
+tmp_dl="$( mktemp "${BUILD_DIR}/.XXXXXX" )"
 tmp_output="$( mktemp "${output}.XXXXXX" )"
 
 ret=1
-if ${LOCALFILES} "${source}" "${tmp_output}"; then
-    mv "${tmp_output}" "${output}"
-    ret=0
+if ${LOCALFILES} "${source}" "${tmp_dl}"; then
+    if cat "${tmp_dl}" >"${tmp_output}"; then
+        mv "${tmp_output}" "${output}"
+        ret=0
+    fi
 fi
 
 # Cleanup
diff --git a/support/download/cvs b/support/download/cvs
index 9aeed79..a8ab080 100755
--- a/support/download/cvs
+++ b/support/download/cvs
@@ -36,7 +36,7 @@ rm -rf "${repodir}"
 ret=1
 if ${CVS} -z3 -d":pserver:anonymous@${repo}" \
            co -d "${repodir}" -r ":${rev}" -P "${rawname}"; then
-    if tar czf "${tmp_output}" "${repodir}"; then
+    if tar czf - "${repodir}" >"${tmp_output}"; then
         mv "${tmp_output}" "${output}"
         ret=0
     fi
diff --git a/support/download/git b/support/download/git
index badb491..b0031e5 100755
--- a/support/download/git
+++ b/support/download/git
@@ -43,8 +43,8 @@ fi
 
 ret=1
 pushd "${repodir}" >/dev/null
-if ${GIT} archive --prefix="${basename}/" -o "${tmp_tar}" \
-                  --format=tar "${cset}"; then
+if ${GIT} archive --prefix="${basename}/" --format=tar "${cset}" \
+                  >"${tmp_tar}"; then
     if gzip -c "${tmp_tar}" >"${tmp_output}"; then
         mv "${tmp_output}" "${output}"
         ret=0
diff --git a/support/download/hg b/support/download/hg
index 6e9e26b..8b36db9 100755
--- a/support/download/hg
+++ b/support/download/hg
@@ -35,7 +35,7 @@ ret=1
 if ${HG} clone --noupdate --rev "${cset}" "${repo}" "${repodir}"; then
     if ${HG} archive --repository "${repodir}" --type tgz \
                      --prefix "${basename}" --rev "${cset}" \
-                     "${tmp_output}"; then
+                     - >"${tmp_output}"; then
         mv "${tmp_output}" "${output}"
         ret=0
     fi
diff --git a/support/download/scp b/support/download/scp
index d3aad43..e16ece5 100755
--- a/support/download/scp
+++ b/support/download/scp
@@ -17,7 +17,7 @@ tmp_output="$( mktemp "${output}.XXXXXX" )"
 
 ret=1
 if ${SCP} "${url}" "${tmp_dl}"; then
-    if mv "${tmp_dl}" "${tmp_output}"; then
+    if cat "${tmp_dl}" >"${tmp_output}"; then
         mv "${tmp_output}" "${output}"
         ret=0
     fi
diff --git a/support/download/svn b/support/download/svn
index 39cbbcb..259d3ed 100755
--- a/support/download/svn
+++ b/support/download/svn
@@ -33,7 +33,7 @@ rm -rf "${repodir}"
 
 ret=1
 if ${SVN} export "${repo}@${rev}" "${repodir}"; then
-    if tar czf "${tmp_output}" "${repodir}"; then
+    if tar czf - "${repodir}" >"${tmp_output}"; then
         mv "${tmp_output}" "${output}"
         ret=0
     fi
diff --git a/support/download/wget b/support/download/wget
index cbeca3b..0f71108 100755
--- a/support/download/wget
+++ b/support/download/wget
@@ -25,7 +25,7 @@ tmp_output="$( mktemp "${output}.XXXXXX" )"
 
 ret=1
 if ${WGET} -O "${tmp_dl}" "${url}"; then
-    if mv "${tmp_dl}" "${tmp_output}"; then
+    if cat "${tmp_dl}" >"${tmp_output}"; then
         mv "${tmp_output}" "${output}"
         ret=0
     fi
-- 
1.9.1

[Buildroot] [PATCH 3/5] support/download: simplify the local-files helper

[Yann E. MORIN]

Currently, the local-files download helper behaves like all the other
download helpers, by first copying into the BUILD_DIR, then to
BR2_DL_DIR, and then rename the final file.

This does two copies, for the sake of using the LOCALFILES command.

Just get rid of the intermediate copy to BUILD_DIR. Instead, directly
copy to the final temp file and rename that.

This does a single copy, but we lose the file access mode, so we just
reinstate them (in case it's a self-extracting executable used in a
br2-external package, for example.)

Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>

---
One may argue that we could just do a symlink dest -> source. But
if source is on a network mount, it may vanish any time (eg. when
undocking your laptop for example ;-) ). So we really need to do a
copy.
---
 Config.in               |  4 ----
 Config.in.legacy        | 11 +++++++++++
 package/pkg-download.mk |  1 -
 support/download/cp     | 13 ++++++-------
 4 files changed, 17 insertions(+), 12 deletions(-)

diff --git a/Config.in b/Config.in
index 50968fb..9e81d77 100644
--- a/Config.in
+++ b/Config.in
@@ -71,10 +71,6 @@ config BR2_CVS
 	string "CVS command"
 	default "cvs"
 
-config BR2_LOCALFILES
-	string "Local files retrieval command"
-	default "cp"
-
 config BR2_SCP
 	string "Secure copy (scp) command"
 	default "scp"
diff --git a/Config.in.legacy b/Config.in.legacy
index a2c7846..aa49806 100644
--- a/Config.in.legacy
+++ b/Config.in.legacy
@@ -101,6 +101,17 @@ endif
 ###############################################################################
 comment "Legacy options removed in 2014.08"
 
+config BR2_LOCALFILES
+	string "Local files retrieval command has been removed"
+	help
+	  The option to specify how to copy local files has been removed.
+	  It is now handled by a helper script in support/download/cp.
+
+config BR2_LOCALFILES_WRAP
+	bool
+	default y if BR2_LOCALFILES != ""
+	select BR2_LEGACY
+
 config BR2_KERNEL_HEADERS_3_8
 	bool "kernel headers version 3.8.x are no longer supported"
 	select BR2_KERNEL_HEADERS_3_9
diff --git a/package/pkg-download.mk b/package/pkg-download.mk
index 7f208d5..fb52ae0 100644
--- a/package/pkg-download.mk
+++ b/package/pkg-download.mk
@@ -16,7 +16,6 @@ export GIT := $(call qstrip,$(BR2_GIT))
 export HG := $(call qstrip,$(BR2_HG)) $(QUIET)
 export SCP := $(call qstrip,$(BR2_SCP)) $(QUIET)
 SSH := $(call qstrip,$(BR2_SSH)) $(QUIET)
-export LOCALFILES := $(call qstrip,$(BR2_LOCALFILES))
 
 # Default spider mode is 'DOWNLOAD'. Other possible values are 'SOURCE_CHECK'
 # used by the _source-check target and 'SHOW_EXTERNAL_DEPS', used by the
diff --git a/support/download/cp b/support/download/cp
index e73159b..e1e7337 100755
--- a/support/download/cp
+++ b/support/download/cp
@@ -8,20 +8,19 @@ set -e
 #   $1: source file
 #   $2: output file
 # And this environment:
-#   LOCALFILES: the cp command to call
+#   (nothing special)
 
 source="${1}"
 output="${2}"
 
-tmp_dl="$( mktemp "${BUILD_DIR}/.XXXXXX" )"
 tmp_output="$( mktemp "${output}.XXXXXX" )"
 
 ret=1
-if ${LOCALFILES} "${source}" "${tmp_dl}"; then
-    if cat "${tmp_dl}" >"${tmp_output}"; then
-        mv "${tmp_output}" "${output}"
-        ret=0
-    fi
+if cat "${source}" >"${tmp_output}"; then
+    mode="$( stat -c '%a' "${source}" )"
+    chmod "${mode}" "${tmp_output}"
+    mv "${tmp_output}" "${output}"
+    ret=0
 fi
 
 # Cleanup
-- 
1.9.1

[Buildroot] [PATCH 4/5] support/download: only create final temp file when needed

[Yann E. MORIN]

Create the temp file in the final location only when it is needed.

This avoids the nasty experience of seeing lots of temp files in
BR2_DL_DIR, that would linger around in case the downloads fails.

Add a comment on why we don;t clean-up after git.

Reported-by: Peter Korsgaard <jacmet@uclibc.org>
Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
---
 support/download/bzr  | 2 +-
 support/download/cvs  | 2 +-
 support/download/git  | 6 ++++--
 support/download/hg   | 2 +-
 support/download/scp  | 2 +-
 support/download/svn  | 2 +-
 support/download/wget | 2 +-
 7 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/support/download/bzr b/support/download/bzr
index f86fa82..a2cb440 100755
--- a/support/download/bzr
+++ b/support/download/bzr
@@ -17,7 +17,6 @@ rev="${2}"
 output="${3}"
 
 tmp_dl="$( mktemp "${BUILD_DIR}/.XXXXXX" )"
-tmp_output="$( mktemp "${output}.XXXXXX" )"
 
 # Play tic-tac-toe with temp files
 # - first, we download to a trashable location (the build-dir)
@@ -27,6 +26,7 @@ tmp_output="$( mktemp "${output}.XXXXXX" )"
 
 ret=1
 if ${BZR} export --format=tgz "${tmp_dl}" "${repo}" -r "${rev}"; then
+    tmp_output="$( mktemp "${output}.XXXXXX" )"
     if cat "${tmp_dl}" >"${tmp_output}"; then
         mv "${tmp_output}" "${output}"
         ret=0
diff --git a/support/download/cvs b/support/download/cvs
index a8ab080..22863d8 100755
--- a/support/download/cvs
+++ b/support/download/cvs
@@ -21,7 +21,6 @@ basename="${4}"
 output="${5}"
 
 repodir="${basename}.tmp-cvs-checkout"
-tmp_output="$( mktemp "${output}.XXXXXX" )"
 
 cd "${BUILD_DIR}"
 # Remove leftovers from a previous failed run
@@ -36,6 +35,7 @@ rm -rf "${repodir}"
 ret=1
 if ${CVS} -z3 -d":pserver:anonymous@${repo}" \
            co -d "${repodir}" -r ":${rev}" -P "${rawname}"; then
+    tmp_output="$( mktemp "${output}.XXXXXX" )"
     if tar czf - "${repodir}" >"${tmp_output}"; then
         mv "${tmp_output}" "${output}"
         ret=0
diff --git a/support/download/git b/support/download/git
index b0031e5..d3fcdaf 100755
--- a/support/download/git
+++ b/support/download/git
@@ -19,8 +19,6 @@ basename="${3}"
 output="${4}"
 
 repodir="${basename}.tmp-git-checkout"
-tmp_tar="$( mktemp "${BUILD_DIR}/.XXXXXX" )"
-tmp_output="$( mktemp "${output}.XXXXXX" )"
 
 # Play tic-tac-toe with temp files
 # - first, we download to a trashable location (the build-dir)
@@ -33,6 +31,8 @@ cd "${BUILD_DIR}"
 # Remove leftovers from a previous failed run
 rm -rf "${repodir}"
 
+# Upon failure, git cleans behind itself, so no need to catch failures here.
+# The only case when git would not clean up, is if it gets killed with SIGKILL.
 if [ -n "$(${GIT} ls-remote "${repo}" "${cset}" 2>&1)" ]; then
     printf "Doing shallow clone\n"
     ${GIT} clone --depth 1 -b "${cset}" --bare "${repo}" "${repodir}"
@@ -43,8 +43,10 @@ fi
 
 ret=1
 pushd "${repodir}" >/dev/null
+tmp_tar="$( mktemp "${BUILD_DIR}/.XXXXXX" )"
 if ${GIT} archive --prefix="${basename}/" --format=tar "${cset}" \
                   >"${tmp_tar}"; then
+    tmp_output="$( mktemp "${output}.XXXXXX" )"
     if gzip -c "${tmp_tar}" >"${tmp_output}"; then
         mv "${tmp_output}" "${output}"
         ret=0
diff --git a/support/download/hg b/support/download/hg
index 8b36db9..7e5c9eb 100755
--- a/support/download/hg
+++ b/support/download/hg
@@ -19,7 +19,6 @@ basename="${3}"
 output="${4}"
 
 repodir="${basename}.tmp-hg-checkout"
-tmp_output="$( mktemp "${output}.XXXXXX" )"
 
 cd "${BUILD_DIR}"
 # Remove leftovers from a previous failed run
@@ -33,6 +32,7 @@ rm -rf "${repodir}"
 
 ret=1
 if ${HG} clone --noupdate --rev "${cset}" "${repo}" "${repodir}"; then
+    tmp_output="$( mktemp "${output}.XXXXXX" )"
     if ${HG} archive --repository "${repodir}" --type tgz \
                      --prefix "${basename}" --rev "${cset}" \
                      - >"${tmp_output}"; then
diff --git a/support/download/scp b/support/download/scp
index e16ece5..1cc18de 100755
--- a/support/download/scp
+++ b/support/download/scp
@@ -13,10 +13,10 @@ set -e
 url="${1}"
 output="${2}"
 tmp_dl="$( mktemp "${BUILD_DIR}/.XXXXXX" )"
-tmp_output="$( mktemp "${output}.XXXXXX" )"
 
 ret=1
 if ${SCP} "${url}" "${tmp_dl}"; then
+    tmp_output="$( mktemp "${output}.XXXXXX" )"
     if cat "${tmp_dl}" >"${tmp_output}"; then
         mv "${tmp_output}" "${output}"
         ret=0
diff --git a/support/download/svn b/support/download/svn
index 259d3ed..232d887 100755
--- a/support/download/svn
+++ b/support/download/svn
@@ -19,7 +19,6 @@ basename="${3}"
 output="${4}"
 
 repodir="${basename}.tmp-svn-checkout"
-tmp_output="$( mktemp "${output}.XXXXXX" )"
 
 cd "${BUILD_DIR}"
 # Remove leftovers from a previous failed run
@@ -33,6 +32,7 @@ rm -rf "${repodir}"
 
 ret=1
 if ${SVN} export "${repo}@${rev}" "${repodir}"; then
+    tmp_output="$( mktemp "${output}.XXXXXX" )"
     if tar czf - "${repodir}" >"${tmp_output}"; then
         mv "${tmp_output}" "${output}"
         ret=0
diff --git a/support/download/wget b/support/download/wget
index 0f71108..e961d71 100755
--- a/support/download/wget
+++ b/support/download/wget
@@ -15,7 +15,6 @@ url="${1}"
 output="${2}"
 
 tmp_dl="$( mktemp "${BUILD_DIR}/.XXXXXX" )"
-tmp_output="$( mktemp "${output}.XXXXXX" )"
 
 # Play tic-tac-toe with temp files
 # - first, we download to a trashable location (the build-dir)
@@ -25,6 +24,7 @@ tmp_output="$( mktemp "${output}.XXXXXX" )"
 
 ret=1
 if ${WGET} -O "${tmp_dl}" "${url}"; then
+    tmp_output="$( mktemp "${output}.XXXXXX" )"
     if cat "${tmp_dl}" >"${tmp_output}"; then
         mv "${tmp_output}" "${output}"
         ret=0
-- 
1.9.1

[Buildroot] [PATCH 5/5] support/download: rationalise naming and use of the temporary files

[Yann E. MORIN]

Currently, we have temporary files to store 'downloaded' files or
repositories, and they all are created using 'mktemp .XXXXXX'.
Also, temporary repositories are named in a inconsistent manner.

This poses a problem in case something goes wrong: it is not possible
to easily see what temp file corresponds to what attempted download.

Make all this a bit more homogeneous:
  - name the temporary files and directories after the final file,
    with this mktemp pattern:   ${BUILD_DIR}/.${output##*/}.XXXXXX

This ensures it is easy to correlate a temp file or dir to the
associated download attempt.

Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>

---
Notes:
  - I was not able to test those helpers, because we have no package
    using them: cp, cvs, scp;
  - I tested bzr with our sole package using bazaar, so corner cases
    might spring to our attention at unexpected times;
  - the cvs helper uses a little trick, due to the fact that cvs is
    so ancient it has never learned the ability to generate archives
    on its own.

Tested with this defconfig, which exercises all possible helpers:
    BR2_x86_i686=y
    BR2_TOOLCHAIN_EXTERNAL=y            # wget
    BR2_PACKAGE_DVB_APPS=y              # hg
    BR2_PACKAGE_DTV_SCAN_TABLES=y       # git
    BR2_PACKAGE_FIS=y                   # svn
    BR2_PACKAGE_PYTHON=y                # wget, needed for pynfc:
    BR2_PACKAGE_PYTHON_NFC=y            # bzr
---
 support/download/bzr  |  3 +--
 support/download/cvs  | 19 +++++++++++--------
 support/download/git  | 11 ++++-------
 support/download/hg   |  7 +------
 support/download/scp  |  4 +++-
 support/download/svn  | 10 +++-------
 support/download/wget |  3 +--
 7 files changed, 24 insertions(+), 33 deletions(-)

diff --git a/support/download/bzr b/support/download/bzr
index a2cb440..ff01bff 100755
--- a/support/download/bzr
+++ b/support/download/bzr
@@ -16,8 +16,6 @@ repo="${1}"
 rev="${2}"
 output="${3}"
 
-tmp_dl="$( mktemp "${BUILD_DIR}/.XXXXXX" )"
-
 # Play tic-tac-toe with temp files
 # - first, we download to a trashable location (the build-dir)
 # - the we move to a temp file in the final location, so it is
@@ -25,6 +23,7 @@ tmp_dl="$( mktemp "${BUILD_DIR}/.XXXXXX" )"
 # - finally, we atomically rename to the final file
 
 ret=1
+tmp_dl="$( mktemp "${BUILD_DIR}/.${output##*/}.XXXXXX" )"
 if ${BZR} export --format=tgz "${tmp_dl}" "${repo}" -r "${rev}"; then
     tmp_output="$( mktemp "${output}.XXXXXX" )"
     if cat "${tmp_dl}" >"${tmp_output}"; then
diff --git a/support/download/cvs b/support/download/cvs
index 22863d8..57beba7 100755
--- a/support/download/cvs
+++ b/support/download/cvs
@@ -20,28 +20,31 @@ rawname="${3}"
 basename="${4}"
 output="${5}"
 
-repodir="${basename}.tmp-cvs-checkout"
-
-cd "${BUILD_DIR}"
-# Remove leftovers from a previous failed run
-rm -rf "${repodir}"
-
 # Play tic-tac-toe with temp files
 # - first, we download to a trashable location (the build-dir)
 # - then we create a temporary tarball in the final location, so it is
 #   on the same filesystem as the final file
 # - finally, we atomically rename to the final file
 
+# Since cvs by itself is not capable of generating archives, we use
+# a little trick of tar, to transform the filenames as they are being
+# added to the archive, by replacing the leadin '.' with the basename
+# of the package. Note: basename is just that, a basename, so it does
+# not contain any '/', so we need not escape them for the transform
+# expression.
+
 ret=1
+repodir="$( mktemp -d "${BUILD_DIR}/.${output##*/}.XXXXXX" )"
 if ${CVS} -z3 -d":pserver:anonymous@${repo}" \
            co -d "${repodir}" -r ":${rev}" -P "${rawname}"; then
     tmp_output="$( mktemp "${output}.XXXXXX" )"
-    if tar czf - "${repodir}" >"${tmp_output}"; then
+    if tar czf - -C "${repodir}" --transform "s/^\./${basename}/;" . \
+           >"${tmp_output}"; then
         mv "${tmp_output}" "${output}"
         ret=0
     fi
 fi
 
 # Cleanup
-rm -rf "${repodir}" "${tmp_output}"
+rm -rf "${tmpdir}" "${tmp_output}"
 exit ${ret}
diff --git a/support/download/git b/support/download/git
index d3fcdaf..011c055 100755
--- a/support/download/git
+++ b/support/download/git
@@ -18,8 +18,6 @@ cset="${2}"
 basename="${3}"
 output="${4}"
 
-repodir="${basename}.tmp-git-checkout"
-
 # Play tic-tac-toe with temp files
 # - first, we download to a trashable location (the build-dir)
 # - then we create the uncomporessed tarball in tht same trashable location
@@ -27,12 +25,11 @@ repodir="${basename}.tmp-git-checkout"
 #   it is on the same filesystem as the final file
 # - finally, we atomically rename to the final file
 
+# Upon failure, git cleans behind itself, so no need to catch failures
+# here. The only case when git would not clean up, is if it gets killed
+# with SIGKILL, which is user-initiated, so we let the user handle that.
 cd "${BUILD_DIR}"
-# Remove leftovers from a previous failed run
-rm -rf "${repodir}"
-
-# Upon failure, git cleans behind itself, so no need to catch failures here.
-# The only case when git would not clean up, is if it gets killed with SIGKILL.
+repodir="$( mktemp -d ".${output##*/}.XXXXXX" )"
 if [ -n "$(${GIT} ls-remote "${repo}" "${cset}" 2>&1)" ]; then
     printf "Doing shallow clone\n"
     ${GIT} clone --depth 1 -b "${cset}" --bare "${repo}" "${repodir}"
diff --git a/support/download/hg b/support/download/hg
index 7e5c9eb..104d034 100755
--- a/support/download/hg
+++ b/support/download/hg
@@ -18,12 +18,6 @@ cset="${2}"
 basename="${3}"
 output="${4}"
 
-repodir="${basename}.tmp-hg-checkout"
-
-cd "${BUILD_DIR}"
-# Remove leftovers from a previous failed run
-rm -rf "${repodir}"
-
 # Play tic-tac-toe with temp files
 # - first, we download to a trashable location (the build-dir)
 # - then we create a temporary tarball in the final location, so it is
@@ -31,6 +25,7 @@ rm -rf "${repodir}"
 # - finally, we atomically rename to the final file
 
 ret=1
+repodir="$( mktemp -d "${BUILD_DIR}/.${output##*/}.XXXXXX" )"
 if ${HG} clone --noupdate --rev "${cset}" "${repo}" "${repodir}"; then
     tmp_output="$( mktemp "${output}.XXXXXX" )"
     if ${HG} archive --repository "${repodir}" --type tgz \
diff --git a/support/download/scp b/support/download/scp
index 1cc18de..192508b 100755
--- a/support/download/scp
+++ b/support/download/scp
@@ -12,9 +12,11 @@ set -e
 
 url="${1}"
 output="${2}"
-tmp_dl="$( mktemp "${BUILD_DIR}/.XXXXXX" )"
+tmp_dl="$( mktemp "${BUILD_DIR}/.${output##*/}..XXXXXX" )"
 
 ret=1
+# Note: it looks like scp does not unlink the destination,
+# which is what we want (observed by stracing scp.)
 if ${SCP} "${url}" "${tmp_dl}"; then
     tmp_output="$( mktemp "${output}.XXXXXX" )"
     if cat "${tmp_dl}" >"${tmp_output}"; then
diff --git a/support/download/svn b/support/download/svn
index 232d887..db98143 100755
--- a/support/download/svn
+++ b/support/download/svn
@@ -18,12 +18,6 @@ rev="${2}"
 basename="${3}"
 output="${4}"
 
-repodir="${basename}.tmp-svn-checkout"
-
-cd "${BUILD_DIR}"
-# Remove leftovers from a previous failed run
-rm -rf "${repodir}"
-
 # Play tic-tac-toe with temp files
 # - first, we download to a trashable location (the build-dir)
 # - then we create a temporary tarball in the final location, so it is
@@ -31,7 +25,9 @@ rm -rf "${repodir}"
 # - finally, we atomically rename to the final file
 
 ret=1
-if ${SVN} export "${repo}@${rev}" "${repodir}"; then
+cd "${BUILD_DIR}"
+repodir="$( mktemp -d ".${output##*/}.XXXXXX" )"
+if ${SVN} export --force "${repo}@${rev}" "${repodir}"; then
     tmp_output="$( mktemp "${output}.XXXXXX" )"
     if tar czf - "${repodir}" >"${tmp_output}"; then
         mv "${tmp_output}" "${output}"
diff --git a/support/download/wget b/support/download/wget
index e961d71..9bb6700 100755
--- a/support/download/wget
+++ b/support/download/wget
@@ -14,8 +14,6 @@ set -e
 url="${1}"
 output="${2}"
 
-tmp_dl="$( mktemp "${BUILD_DIR}/.XXXXXX" )"
-
 # Play tic-tac-toe with temp files
 # - first, we download to a trashable location (the build-dir)
 # - then we copy to a temporary tarball in the final location, so it is
@@ -23,6 +21,7 @@ tmp_dl="$( mktemp "${BUILD_DIR}/.XXXXXX" )"
 # - finally, we atomically rename to the final file
 
 ret=1
+tmp_dl="$( mktemp "${BUILD_DIR}/.${output##*/}.XXXXXX" )"
 if ${WGET} -O "${tmp_dl}" "${url}"; then
     tmp_output="$( mktemp "${output}.XXXXXX" )"
     if cat "${tmp_dl}" >"${tmp_output}"; then
-- 
1.9.1

[Buildroot] [PATCH 5/5] support/download: rationalise naming and use of the temporary files

[Yann E. MORIN]

All,

Doing a review of my own series...

Damn, I'm feeling so schizophrenic, now... ;-]

On 2014-07-06 23:27 +0200, Yann E. MORIN spake thusly:
> Currently, we have temporary files to store 'downloaded' files or
> repositories, and they all are created using 'mktemp .XXXXXX'.
> Also, temporary repositories are named in a inconsistent manner.
> 
> This poses a problem in case something goes wrong: it is not possible
> to easily see what temp file corresponds to what attempted download.
> 
> Make all this a bit more homogeneous:
>   - name the temporary files and directories after the final file,
>     with this mktemp pattern:   ${BUILD_DIR}/.${output##*/}.XXXXXX
> 
> This ensures it is easy to correlate a temp file or dir to the
> associated download attempt.
[--SNIP--]
> --- a/support/download/cvs
> +++ b/support/download/cvs
> @@ -20,28 +20,31 @@ rawname="${3}"
>  basename="${4}"
>  output="${5}"
>  
> -repodir="${basename}.tmp-cvs-checkout"
> -
> -cd "${BUILD_DIR}"
> -# Remove leftovers from a previous failed run
> -rm -rf "${repodir}"
> -
>  # Play tic-tac-toe with temp files
>  # - first, we download to a trashable location (the build-dir)
>  # - then we create a temporary tarball in the final location, so it is
>  #   on the same filesystem as the final file
>  # - finally, we atomically rename to the final file
>  
> +# Since cvs by itself is not capable of generating archives, we use
> +# a little trick of tar, to transform the filenames as they are being
> +# added to the archive, by replacing the leadin '.' with the basename
> +# of the package. Note: basename is just that, a basename, so it does
> +# not contain any '/', so we need not escape them for the transform
> +# expression.
> +
>  ret=1
> +repodir="$( mktemp -d "${BUILD_DIR}/.${output##*/}.XXXXXX" )"
>  if ${CVS} -z3 -d":pserver:anonymous@${repo}" \
>             co -d "${repodir}" -r ":${rev}" -P "${rawname}"; then
>      tmp_output="$( mktemp "${output}.XXXXXX" )"
> -    if tar czf - "${repodir}" >"${tmp_output}"; then
> +    if tar czf - -C "${repodir}" --transform "s/^\./${basename}/;" . \
> +           >"${tmp_output}"; then
>          mv "${tmp_output}" "${output}"
>          ret=0
>      fi
>  fi
>  
>  # Cleanup
> -rm -rf "${repodir}" "${tmp_output}"
> +rm -rf "${tmpdir}" "${tmp_output}"

This hunk is a left-over from a previous tentative. I forgot to remove
that.

/me reaches for his flame-thrower...

>  exit ${ret}
> diff --git a/support/download/git b/support/download/git
> index d3fcdaf..011c055 100755
> --- a/support/download/git
> +++ b/support/download/git
> @@ -18,8 +18,6 @@ cset="${2}"
>  basename="${3}"
>  output="${4}"
>  
> -repodir="${basename}.tmp-git-checkout"
> -
>  # Play tic-tac-toe with temp files
>  # - first, we download to a trashable location (the build-dir)
>  # - then we create the uncomporessed tarball in tht same trashable location
> @@ -27,12 +25,11 @@ repodir="${basename}.tmp-git-checkout"
>  #   it is on the same filesystem as the final file
>  # - finally, we atomically rename to the final file
>  
> +# Upon failure, git cleans behind itself, so no need to catch failures
> +# here. The only case when git would not clean up, is if it gets killed
> +# with SIGKILL, which is user-initiated, so we let the user handle that.
>  cd "${BUILD_DIR}"
> -# Remove leftovers from a previous failed run
> -rm -rf "${repodir}"
> -
> -# Upon failure, git cleans behind itself, so no need to catch failures here.
> -# The only case when git would not clean up, is if it gets killed with SIGKILL.

And the rewrite of this comment should have been folded in the
corresponding changeset... :-(

> diff --git a/support/download/scp b/support/download/scp
> index 1cc18de..192508b 100755
> --- a/support/download/scp
> +++ b/support/download/scp
> @@ -12,9 +12,11 @@ set -e
>  
>  url="${1}"
>  output="${2}"
> -tmp_dl="$( mktemp "${BUILD_DIR}/.XXXXXX" )"
> +tmp_dl="$( mktemp "${BUILD_DIR}/.${output##*/}..XXXXXX" )"

And there is a double-dot here... :-(

> diff --git a/support/download/svn b/support/download/svn
> index 232d887..db98143 100755
> --- a/support/download/svn
> +++ b/support/download/svn
> @@ -31,7 +25,9 @@ rm -rf "${repodir}"
>  # - finally, we atomically rename to the final file
>  
>  ret=1
> -if ${SVN} export "${repo}@${rev}" "${repodir}"; then
> +cd "${BUILD_DIR}"
> +repodir="$( mktemp -d ".${output##*/}.XXXXXX" )"
> +if ${SVN} export --force "${repo}@${rev}" "${repodir}"; then
>      tmp_output="$( mktemp "${output}.XXXXXX" )"
>      if tar czf - "${repodir}" >"${tmp_output}"; then

And here, we should use the same trick as is used for cvs.

Sigh...

I'll wait for more comments before re-spinning the series...

Regards,
Yann E. MORIN.

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

[Buildroot] [PATCH 1/5] support/download: fix the bzr helper

[Arnout Vandecappelle]

On 06/07/14 23:27, Yann E. MORIN wrote:
> bzr uses the name of the extension of the output file to known what
> output format to use: tar, tgz, tar.bz2... If no extension is
> recognised, bzr will output to a directory.
> 
> Since we use 'mktemp .XXXXXX' to generate temporary files, it obviously
> never ends with a recognised extension. Thus, bzr expects the output to
> be a directory, and fails since it is a file.

 I think it would be much simpler and more natural to use

tmp_dl="$( mktemp "${BUILD_DIR}/.XXXXXX.tar.gz" )"
tmp_output="$( mktemp "${output}.XXXXXX.tar.gz" )"

> 
> Fix that by forcing the output format.

 But of course, forcing it explicitly never hurts.

 So even with my earlier comment, this one gets my

Reviewed-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>


 Regards,
 Arnout

> 
> Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
> ---
>  support/download/bzr | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/support/download/bzr b/support/download/bzr
> index 3f52ee9..19d837d 100755
> --- a/support/download/bzr
> +++ b/support/download/bzr
> @@ -26,7 +26,7 @@ tmp_output="$( mktemp "${output}.XXXXXX" )"
>  # - finally, we atomically rename to the final file
>  
>  ret=1
> -if ${BZR} export "${tmp_dl}" "${repo}" -r "${rev}"; then
> +if ${BZR} export --format=tgz "${tmp_dl}" "${repo}" -r "${rev}"; then
>      if mv "${tmp_dl}" "${tmp_output}"; then
>          mv "${tmp_output}" "${output}"
>          ret=0
> 


-- 
Arnout Vandecappelle                          arnout at mind be
Senior Embedded Software Architect            +32-16-286500
Essensium/Mind                                http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint:  7CB5 E4CC 6C2E EFD4 6E3D A754 F963 ECAB 2450 2F1F

[Buildroot] [PATCH 2/5] support/download: properly use temp files

[Arnout Vandecappelle]

On 06/07/14 23:27, Yann E. MORIN wrote:
> When using 'mv' to move files to temp files, the existing temp file is
> forcibly removed by 'mv', and a new file is created.
> 
> Although this should have little impact to us, there is still a race
> conditions in case two parallel downloads would step on each other's
> toes, and it goes like that:
> 
>     Process 1                       Process 2
>     mktemp tmp_output
>     mv tmp_dl tmp_output
>         removes tmp_output
>                                     mktemp tmp_ouput
>         writes to tmp_output
>                                     mv tmp_dl tmp_output
>                                         removes tmp_output
>                                         writes to tmp_output
>     mv tmp_output output
>                                     mv tmp_output output
> 
> In this case, mktemp has the opportunity to create the same tmp_output
> temporary file, and we trash the content from one with the content
> from the other, and the last to do the final 'mv' breaks horibly.
> 
> Instead, use 'cat', which guarantees that tmp_output is not removed
> before writing to it.

 Not that it makes a real difference, but I think that 'cp' is a more natural
way to do this.

> 
> This complexifies a bit the local-files (cp) helper, but better safe
> than sorry.
> 
> (Note: this is a purely theoretical issue, I did not observe it yet, but
> it is slated to happen one day or the other, and will be very hard to
> debug then.)

 Actually, since the mktemp string is based on time and PID, the chance of this
happening is really vanishingly small. That said, better safe than sorry.


> 
> Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
> ---
>  support/download/bzr  | 2 +-
>  support/download/cp   | 9 ++++++---
>  support/download/cvs  | 2 +-
>  support/download/git  | 4 ++--
>  support/download/hg   | 2 +-
>  support/download/scp  | 2 +-
>  support/download/svn  | 2 +-
>  support/download/wget | 2 +-
>  8 files changed, 14 insertions(+), 11 deletions(-)
> 
> diff --git a/support/download/bzr b/support/download/bzr
> index 19d837d..f86fa82 100755
> --- a/support/download/bzr
> +++ b/support/download/bzr
> @@ -27,7 +27,7 @@ tmp_output="$( mktemp "${output}.XXXXXX" )"
>  
>  ret=1
>  if ${BZR} export --format=tgz "${tmp_dl}" "${repo}" -r "${rev}"; then
> -    if mv "${tmp_dl}" "${tmp_output}"; then
> +    if cat "${tmp_dl}" >"${tmp_output}"; then
>          mv "${tmp_output}" "${output}"
>          ret=0
>      fi

 Not really related to this patch, but why do we need this ${tmp_dl} to begin
with? Especially since we're already "occupying" a tempfile in DL_DIR anyway.

 Also, with this added complexity, the download helper scripts are becoming
quite similar. So wouldn't it be a good idea to refactor them? I'm thinking of
something like this:

support/download/helper:

#!/bin/bash

# We want to catch any command failure, and exit immediately
set -e

# Generic download helper
# Call it with:
#   $1: specific download helper
#   $2: output file
#   ...: arguments for the specific download helper

download="${1}"
output="${2}"
shift 2

tmp_output="$( mktemp ... )"
if "${download}" "${tmp_output}" "$@"; then
	# Blah blah need things to be atomic blah blah
	mv "${tmp_output}" "${output}"
fi



 I expect there will be even more common stuff between the download helpers in
the future, so it makes sense to have this.


> diff --git a/support/download/cp b/support/download/cp
> index 8f6bc06..e73159b 100755
> --- a/support/download/cp
> +++ b/support/download/cp
> @@ -13,12 +13,15 @@ set -e
>  source="${1}"
>  output="${2}"
>  
> +tmp_dl="$( mktemp "${BUILD_DIR}/.XXXXXX" )"
>  tmp_output="$( mktemp "${output}.XXXXXX" )"
>  
>  ret=1
> -if ${LOCALFILES} "${source}" "${tmp_output}"; then
> -    mv "${tmp_output}" "${output}"
> -    ret=0
> +if ${LOCALFILES} "${source}" "${tmp_dl}"; then
> +    if cat "${tmp_dl}" >"${tmp_output}"; then

 Same here, what's the point of ${tmp_dl}?

> +        mv "${tmp_output}" "${output}"
> +        ret=0
> +    fi
>  fi
>  
>  # Cleanup
> diff --git a/support/download/cvs b/support/download/cvs
> index 9aeed79..a8ab080 100755
> --- a/support/download/cvs
> +++ b/support/download/cvs
> @@ -36,7 +36,7 @@ rm -rf "${repodir}"
>  ret=1
>  if ${CVS} -z3 -d":pserver:anonymous@${repo}" \
>             co -d "${repodir}" -r ":${rev}" -P "${rawname}"; then
> -    if tar czf "${tmp_output}" "${repodir}"; then
> +    if tar czf - "${repodir}" >"${tmp_output}"; then
>          mv "${tmp_output}" "${output}"
>          ret=0
>      fi
> diff --git a/support/download/git b/support/download/git
> index badb491..b0031e5 100755
> --- a/support/download/git
> +++ b/support/download/git
> @@ -43,8 +43,8 @@ fi
>  
>  ret=1
>  pushd "${repodir}" >/dev/null
> -if ${GIT} archive --prefix="${basename}/" -o "${tmp_tar}" \
> -                  --format=tar "${cset}"; then
> +if ${GIT} archive --prefix="${basename}/" --format=tar "${cset}" \
> +                  >"${tmp_tar}"; then

 What's the reason for this change? I've checked with strace, and git archive
doesn't seem to unlink, it just does open(O_TRUNC) like cp.


>      if gzip -c "${tmp_tar}" >"${tmp_output}"; then
>          mv "${tmp_output}" "${output}"
>          ret=0
> diff --git a/support/download/hg b/support/download/hg
> index 6e9e26b..8b36db9 100755
> --- a/support/download/hg
> +++ b/support/download/hg
> @@ -35,7 +35,7 @@ ret=1
>  if ${HG} clone --noupdate --rev "${cset}" "${repo}" "${repodir}"; then
>      if ${HG} archive --repository "${repodir}" --type tgz \
>                       --prefix "${basename}" --rev "${cset}" \
> -                     "${tmp_output}"; then
> +                     - >"${tmp_output}"; then

 I didn't check for hg, but I also don't expect it to unlink() first. In fact,
it's mv's behaviour of unlinking first that is surprising.

>          mv "${tmp_output}" "${output}"
>          ret=0
>      fi
> diff --git a/support/download/scp b/support/download/scp
> index d3aad43..e16ece5 100755
> --- a/support/download/scp
> +++ b/support/download/scp
> @@ -17,7 +17,7 @@ tmp_output="$( mktemp "${output}.XXXXXX" )"
>  
>  ret=1
>  if ${SCP} "${url}" "${tmp_dl}"; then
> -    if mv "${tmp_dl}" "${tmp_output}"; then
> +    if cat "${tmp_dl}" >"${tmp_output}"; then
>          mv "${tmp_output}" "${output}"
>          ret=0
>      fi
> diff --git a/support/download/svn b/support/download/svn
> index 39cbbcb..259d3ed 100755
> --- a/support/download/svn
> +++ b/support/download/svn
> @@ -33,7 +33,7 @@ rm -rf "${repodir}"
>  
>  ret=1
>  if ${SVN} export "${repo}@${rev}" "${repodir}"; then
> -    if tar czf "${tmp_output}" "${repodir}"; then
> +    if tar czf - "${repodir}" >"${tmp_output}"; then

 Again, tar doesn't unlink so this isn't needed.



 Regards,
 Arnout

>          mv "${tmp_output}" "${output}"
>          ret=0
>      fi
> diff --git a/support/download/wget b/support/download/wget
> index cbeca3b..0f71108 100755
> --- a/support/download/wget
> +++ b/support/download/wget
> @@ -25,7 +25,7 @@ tmp_output="$( mktemp "${output}.XXXXXX" )"
>  
>  ret=1
>  if ${WGET} -O "${tmp_dl}" "${url}"; then
> -    if mv "${tmp_dl}" "${tmp_output}"; then
> +    if cat "${tmp_dl}" >"${tmp_output}"; then
>          mv "${tmp_output}" "${output}"
>          ret=0
>      fi
> 


-- 
Arnout Vandecappelle                          arnout at mind be
Senior Embedded Software Architect            +32-16-286500
Essensium/Mind                                http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint:  7CB5 E4CC 6C2E EFD4 6E3D A754 F963 ECAB 2450 2F1F

[Buildroot] [PATCH 4/5] support/download: only create final temp file when needed

[Arnout Vandecappelle]

On 06/07/14 23:27, Yann E. MORIN wrote:
> Create the temp file in the final location only when it is needed.
> 
> This avoids the nasty experience of seeing lots of temp files in
> BR2_DL_DIR, that would linger around in case the downloads fails.

 It would also help to add a

trap "rm -f ${tmp_dl}" EXIT HUP INT QUIT TERM

which also removes the need of doing the rm at the end. Of course, that trap is
still racy (interrupt between mktemp and trap).

 But with the two temporary variables, adding a trap becomes even more
difficult. Unless that part is refactored into a separate helper script of
course :-)

> 
> Add a comment on why we don;t clean-up after git.

 Typo in don;t

> 
> Reported-by: Peter Korsgaard <jacmet@uclibc.org>
> Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
> ---
>  support/download/bzr  | 2 +-
>  support/download/cvs  | 2 +-
>  support/download/git  | 6 ++++--
>  support/download/hg   | 2 +-
>  support/download/scp  | 2 +-
>  support/download/svn  | 2 +-
>  support/download/wget | 2 +-
>  7 files changed, 10 insertions(+), 8 deletions(-)
> 
> diff --git a/support/download/bzr b/support/download/bzr
> index f86fa82..a2cb440 100755
> --- a/support/download/bzr
> +++ b/support/download/bzr
> @@ -17,7 +17,6 @@ rev="${2}"
>  output="${3}"
>  
>  tmp_dl="$( mktemp "${BUILD_DIR}/.XXXXXX" )"
> -tmp_output="$( mktemp "${output}.XXXXXX" )"
>  
>  # Play tic-tac-toe with temp files
>  # - first, we download to a trashable location (the build-dir)
> @@ -27,6 +26,7 @@ tmp_output="$( mktemp "${output}.XXXXXX" )"
>  
>  ret=1
>  if ${BZR} export --format=tgz "${tmp_dl}" "${repo}" -r "${rev}"; then
> +    tmp_output="$( mktemp "${output}.XXXXXX" )"
>      if cat "${tmp_dl}" >"${tmp_output}"; then
>          mv "${tmp_output}" "${output}"
>          ret=0
> diff --git a/support/download/cvs b/support/download/cvs
> index a8ab080..22863d8 100755
> --- a/support/download/cvs
> +++ b/support/download/cvs
> @@ -21,7 +21,6 @@ basename="${4}"
>  output="${5}"
>  
>  repodir="${basename}.tmp-cvs-checkout"
> -tmp_output="$( mktemp "${output}.XXXXXX" )"
>  
>  cd "${BUILD_DIR}"
>  # Remove leftovers from a previous failed run
> @@ -36,6 +35,7 @@ rm -rf "${repodir}"
>  ret=1
>  if ${CVS} -z3 -d":pserver:anonymous@${repo}" \
>             co -d "${repodir}" -r ":${rev}" -P "${rawname}"; then
> +    tmp_output="$( mktemp "${output}.XXXXXX" )"
>      if tar czf - "${repodir}" >"${tmp_output}"; then
>          mv "${tmp_output}" "${output}"
>          ret=0
> diff --git a/support/download/git b/support/download/git
> index b0031e5..d3fcdaf 100755
> --- a/support/download/git
> +++ b/support/download/git
> @@ -19,8 +19,6 @@ basename="${3}"
>  output="${4}"
>  
>  repodir="${basename}.tmp-git-checkout"

 (not related to this patch) It's actually not a checkout, it's a bare clone.

> -tmp_tar="$( mktemp "${BUILD_DIR}/.XXXXXX" )"
> -tmp_output="$( mktemp "${output}.XXXXXX" )"
>  
>  # Play tic-tac-toe with temp files
>  # - first, we download to a trashable location (the build-dir)
> @@ -33,6 +31,8 @@ cd "${BUILD_DIR}"
>  # Remove leftovers from a previous failed run
>  rm -rf "${repodir}"
>  
> +# Upon failure, git cleans behind itself, so no need to catch failures here.
> +# The only case when git would not clean up, is if it gets killed with SIGKILL.

 I think the SIGKILL is reason enough to do the rm explicitly in the script. Of
course, this is only valid if you use the trap, otherwise you never reach the rm
(due to 'set -e').


 Regards,
 Arnout

>  if [ -n "$(${GIT} ls-remote "${repo}" "${cset}" 2>&1)" ]; then
>      printf "Doing shallow clone\n"
>      ${GIT} clone --depth 1 -b "${cset}" --bare "${repo}" "${repodir}"
> @@ -43,8 +43,10 @@ fi
>  
>  ret=1
>  pushd "${repodir}" >/dev/null
> +tmp_tar="$( mktemp "${BUILD_DIR}/.XXXXXX" )"
>  if ${GIT} archive --prefix="${basename}/" --format=tar "${cset}" \
>                    >"${tmp_tar}"; then
> +    tmp_output="$( mktemp "${output}.XXXXXX" )"
>      if gzip -c "${tmp_tar}" >"${tmp_output}"; then
>          mv "${tmp_output}" "${output}"
>          ret=0
[snip]

-- 
Arnout Vandecappelle                          arnout at mind be
Senior Embedded Software Architect            +32-16-286500
Essensium/Mind                                http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint:  7CB5 E4CC 6C2E EFD4 6E3D A754 F963 ECAB 2450 2F1F

[Buildroot] [PATCH 2/5] support/download: properly use temp files

[Yann E. MORIN]

Arnout, All,

On 2014-07-07 08:11 +0200, Arnout Vandecappelle spake thusly:
> On 06/07/14 23:27, Yann E. MORIN wrote:
> > When using 'mv' to move files to temp files, the existing temp file is
> > forcibly removed by 'mv', and a new file is created.
> > 
> > Although this should have little impact to us, there is still a race
> > conditions in case two parallel downloads would step on each other's
> > toes, and it goes like that:
> > 
> >     Process 1                       Process 2
> >     mktemp tmp_output
> >     mv tmp_dl tmp_output
> >         removes tmp_output
> >                                     mktemp tmp_ouput
> >         writes to tmp_output
> >                                     mv tmp_dl tmp_output
> >                                         removes tmp_output
> >                                         writes to tmp_output
> >     mv tmp_output output
> >                                     mv tmp_output output
> > 
> > In this case, mktemp has the opportunity to create the same tmp_output
> > temporary file, and we trash the content from one with the content
> > from the other, and the last to do the final 'mv' breaks horibly.
> > 
> > Instead, use 'cat', which guarantees that tmp_output is not removed
> > before writing to it.
> 
>  Not that it makes a real difference, but I think that 'cp' is a more natural
> way to do this.

I am not sure how cp handles copying over an existing file. I'll
check...

> > This complexifies a bit the local-files (cp) helper, but better safe
> > than sorry.
> > 
> > (Note: this is a purely theoretical issue, I did not observe it yet, but
> > it is slated to happen one day or the other, and will be very hard to
> > debug then.)
> 
>  Actually, since the mktemp string is based on time and PID, the chance of this
> happening is really vanishingly small. That said, better safe than sorry.

Yes, the opportunity for a collision is very low, but it's not
impossible. After all, that's the problem with race conditions: given
sufficient time and trials, you'll hit it, and it is very hard to debug,
especially in this case, since the condition is not reproducible (random
names.)

> > Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
> > ---
> >  support/download/bzr  | 2 +-
> >  support/download/cp   | 9 ++++++---
> >  support/download/cvs  | 2 +-
> >  support/download/git  | 4 ++--
> >  support/download/hg   | 2 +-
> >  support/download/scp  | 2 +-
> >  support/download/svn  | 2 +-
> >  support/download/wget | 2 +-
> >  8 files changed, 14 insertions(+), 11 deletions(-)
> > 
> > diff --git a/support/download/bzr b/support/download/bzr
> > index 19d837d..f86fa82 100755
> > --- a/support/download/bzr
> > +++ b/support/download/bzr
> > @@ -27,7 +27,7 @@ tmp_output="$( mktemp "${output}.XXXXXX" )"
> >  
> >  ret=1
> >  if ${BZR} export --format=tgz "${tmp_dl}" "${repo}" -r "${rev}"; then
> > -    if mv "${tmp_dl}" "${tmp_output}"; then
> > +    if cat "${tmp_dl}" >"${tmp_output}"; then
> >          mv "${tmp_output}" "${output}"
> >          ret=0
> >      fi
> 
>  Not really related to this patch, but why do we need this ${tmp_dl} to begin
> with? Especially since we're already "occupying" a tempfile in DL_DIR anyway.

The idea was not to pollute BR2_DL_DIR, in case the download fails.
Hence this dance:
  - download to a disposable area (BUILD_DIR);
  - move to a temp file in BR2_DL_DIR;
  - atomically rename to the final file.

But granted, since  we remove failed downloads, we can skip the
intermediary temp file in BUILD_DIR (although I still do not like much
the idea of polluting BR2_DL_DIR.)

I think there is still a case for which we can't remove the temp file...
Ah yes, if the user cancels a build with Ctrl-C, the script is aborted,
so we leave a .XXXXXX file in BR2_DL_DIR.

But even with the three-step dance above, there is still a small window
for leaving out cruft in BR2_DL_DIR; but that window is much, much
smaller than the download window.

>  Also, with this added complexity, the download helper scripts are becoming
> quite similar. So wouldn't it be a good idea to refactor them?

I think we already had this discussion back when I introduced the series:
    http://lists.busybox.net/pipermail/buildroot/2014-January/086826.html

But for now, there are a few important fixes I'd like be applied before
going further.

> I'm thinking of
> something like this:
> 
> support/download/helper:
> 
> #!/bin/bash
> 
> # We want to catch any command failure, and exit immediately
> set -e
> 
> # Generic download helper
> # Call it with:
> #   $1: specific download helper
> #   $2: output file
> #   ...: arguments for the specific download helper
> 
> download="${1}"
> output="${2}"
> shift 2
> 
> tmp_output="$( mktemp ... )"
> if "${download}" "${tmp_output}" "$@"; then
> 	# Blah blah need things to be atomic blah blah
> 	mv "${tmp_output}" "${output}"
> fi

Oh, you meant having a wrapper script above all the other helpers, and
delegate to the helpers only the download, and handle the atomicity
dance to the wrapper. Hmmm... Might be worth looking at, probably, yes.

>  I expect there will be even more common stuff between the download helpers in
> the future, so it makes sense to have this.

Well, for one, moving the hash check in that wrapper, for example...

> > diff --git a/support/download/cp b/support/download/cp
> > index 8f6bc06..e73159b 100755
> > --- a/support/download/cp
> > +++ b/support/download/cp
> > @@ -13,12 +13,15 @@ set -e
> >  source="${1}"
> >  output="${2}"
> >  
> > +tmp_dl="$( mktemp "${BUILD_DIR}/.XXXXXX" )"
> >  tmp_output="$( mktemp "${output}.XXXXXX" )"
> >  
> >  ret=1
> > -if ${LOCALFILES} "${source}" "${tmp_output}"; then
> > -    mv "${tmp_output}" "${output}"
> > -    ret=0
> > +if ${LOCALFILES} "${source}" "${tmp_dl}"; then
> > +    if cat "${tmp_dl}" >"${tmp_output}"; then
> 
>  Same here, what's the point of ${tmp_dl}?

Same answer.

> > +        mv "${tmp_output}" "${output}"
> > +        ret=0
> > +    fi
> >  fi
> >  
> >  # Cleanup
> > diff --git a/support/download/cvs b/support/download/cvs
> > index 9aeed79..a8ab080 100755
> > --- a/support/download/cvs
> > +++ b/support/download/cvs
> > @@ -36,7 +36,7 @@ rm -rf "${repodir}"
> >  ret=1
> >  if ${CVS} -z3 -d":pserver:anonymous@${repo}" \
> >             co -d "${repodir}" -r ":${rev}" -P "${rawname}"; then
> > -    if tar czf "${tmp_output}" "${repodir}"; then
> > +    if tar czf - "${repodir}" >"${tmp_output}"; then
> >          mv "${tmp_output}" "${output}"
> >          ret=0
> >      fi
> > diff --git a/support/download/git b/support/download/git
> > index badb491..b0031e5 100755
> > --- a/support/download/git
> > +++ b/support/download/git
> > @@ -43,8 +43,8 @@ fi
> >  
> >  ret=1
> >  pushd "${repodir}" >/dev/null
> > -if ${GIT} archive --prefix="${basename}/" -o "${tmp_tar}" \
> > -                  --format=tar "${cset}"; then
> > +if ${GIT} archive --prefix="${basename}/" --format=tar "${cset}" \
> > +                  >"${tmp_tar}"; then
> 
>  What's the reason for this change? I've checked with strace, and git archive
> doesn't seem to unlink, it just does open(O_TRUNC) like cp.

Ah, so you did strace both? I straced scp, and it does unlink.
Are we sure all cp and all tar we can encounter will all use
open(O_TRUNC) ?

It'd rather go on the safe side, and not assume much about this,
especially since that behaviour may change in the future, or may not
show in older versions, or different versions.

> >      if gzip -c "${tmp_tar}" >"${tmp_output}"; then
> >          mv "${tmp_output}" "${output}"
> >          ret=0
> > diff --git a/support/download/hg b/support/download/hg
> > index 6e9e26b..8b36db9 100755
> > --- a/support/download/hg
> > +++ b/support/download/hg
> > @@ -35,7 +35,7 @@ ret=1
> >  if ${HG} clone --noupdate --rev "${cset}" "${repo}" "${repodir}"; then
> >      if ${HG} archive --repository "${repodir}" --type tgz \
> >                       --prefix "${basename}" --rev "${cset}" \
> > -                     "${tmp_output}"; then
> > +                     - >"${tmp_output}"; then
> 
>  I didn't check for hg, but I also don't expect it to unlink() first. In fact,
> it's mv's behaviour of unlinking first that is surprising.

Well, scp also unlinks first.

And same answer, I don;t want to rely on such a behaviour. Redirecting
works the same everywhere, though.

> >          mv "${tmp_output}" "${output}"
> >          ret=0
> >      fi
> > diff --git a/support/download/scp b/support/download/scp
> > index d3aad43..e16ece5 100755
> > --- a/support/download/scp
> > +++ b/support/download/scp
> > @@ -17,7 +17,7 @@ tmp_output="$( mktemp "${output}.XXXXXX" )"
> >  
> >  ret=1
> >  if ${SCP} "${url}" "${tmp_dl}"; then
> > -    if mv "${tmp_dl}" "${tmp_output}"; then
> > +    if cat "${tmp_dl}" >"${tmp_output}"; then
> >          mv "${tmp_output}" "${output}"
> >          ret=0
> >      fi
> > diff --git a/support/download/svn b/support/download/svn
> > index 39cbbcb..259d3ed 100755
> > --- a/support/download/svn
> > +++ b/support/download/svn
> > @@ -33,7 +33,7 @@ rm -rf "${repodir}"
> >  
> >  ret=1
> >  if ${SVN} export "${repo}@${rev}" "${repodir}"; then
> > -    if tar czf "${tmp_output}" "${repodir}"; then
> > +    if tar czf - "${repodir}" >"${tmp_output}"; then
> 
>  Again, tar doesn't unlink so this isn't needed.

Ditto.

Regards,
Yann E. MORIN.

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

[Buildroot] [PATCH 4/5] support/download: only create final temp file when needed

[Yann E. MORIN]

Arnout, All,

On 2014-07-07 18:08 +0200, Arnout Vandecappelle spake thusly:
> On 06/07/14 23:27, Yann E. MORIN wrote:
> > Create the temp file in the final location only when it is needed.
> > 
> > This avoids the nasty experience of seeing lots of temp files in
> > BR2_DL_DIR, that would linger around in case the downloads fails.
> 
>  It would also help to add a
> 
> trap "rm -f ${tmp_dl}" EXIT HUP INT QUIT TERM
> 
> which also removes the need of doing the rm at the end. Of course, that trap is
> still racy (interrupt between mktemp and trap).
> 
>  But with the two temporary variables, adding a trap becomes even more
> difficult. Unless that part is refactored into a separate helper script of
> course :-)

Yep, added to the list of TODO. ;-)

> > Add a comment on why we don;t clean-up after git.
> 
>  Typo in don;t

Yep, already fixed.

[--SNIP--]
> > diff --git a/support/download/git b/support/download/git
> > index b0031e5..d3fcdaf 100755
> > --- a/support/download/git
> > +++ b/support/download/git
> > @@ -19,8 +19,6 @@ basename="${3}"
> >  output="${4}"
> >  
> >  repodir="${basename}.tmp-git-checkout"
> 
>  (not related to this patch) It's actually not a checkout, it's a bare clone.

And it's being renamed in a later patch.

> > -tmp_tar="$( mktemp "${BUILD_DIR}/.XXXXXX" )"
> > -tmp_output="$( mktemp "${output}.XXXXXX" )"
> >  
> >  # Play tic-tac-toe with temp files
> >  # - first, we download to a trashable location (the build-dir)
> > @@ -33,6 +31,8 @@ cd "${BUILD_DIR}"
> >  # Remove leftovers from a previous failed run
> >  rm -rf "${repodir}"
> >  
> > +# Upon failure, git cleans behind itself, so no need to catch failures here.
> > +# The only case when git would not clean up, is if it gets killed with SIGKILL.
> 
>  I think the SIGKILL is reason enough to do the rm explicitly in the script. Of
> course, this is only valid if you use the trap, otherwise you never reach the rm

Well, SIGKILL is not catchable. That's the main selling point of SIGKILL.
;-)

> (due to 'set -e').

Well, 'set -e' only kicks in for the final 'mv', since all other
commands and conditions in an if statement.

Also, I'm not a big fan of traps, although I can be abusing them from
time to time.

But OK, I'll add this to the TODO...

Regards,
Yann E. MORIN.

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

[Buildroot] [PATCH 3/5] support/download: simplify the local-files helper

[Peter Korsgaard]

>>>>> "Yann" == Yann E MORIN <yann.morin.1998@free.fr> writes:

 > Currently, the local-files download helper behaves like all the other
 > download helpers, by first copying into the BUILD_DIR, then to
 > BR2_DL_DIR, and then rename the final file.

 > This does two copies, for the sake of using the LOCALFILES command.

 > Just get rid of the intermediate copy to BUILD_DIR. Instead, directly
 > copy to the final temp file and rename that.

 > This does a single copy, but we lose the file access mode, so we just
 > reinstate them (in case it's a self-extracting executable used in a
 > br2-external package, for example.)

 > Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>


 > +++ b/support/download/cp
 > @@ -8,20 +8,19 @@ set -e
 >  #   $1: source file
 >  #   $2: output file
 >  # And this environment:
 > -#   LOCALFILES: the cp command to call
 > +#   (nothing special)
 
 >  source="${1}"
 >  output="${2}"
 
 > -tmp_dl="$( mktemp "${BUILD_DIR}/.XXXXXX" )"
 >  tmp_output="$( mktemp "${output}.XXXXXX" )"
 
 >  ret=1
 > -if ${LOCALFILES} "${source}" "${tmp_dl}"; then
 > -    if cat "${tmp_dl}" >"${tmp_output}"; then
 > -        mv "${tmp_output}" "${output}"
 > -        ret=0
 > -    fi
 > +if cat "${source}" >"${tmp_output}"; then
 > +    mode="$( stat -c '%a' "${source}" )"
 > +    chmod "${mode}" "${tmp_output}"
 > +    mv "${tmp_output}" "${output}"
 > +    ret=0

Similar to Arnouts comments, wouldn't cp -a be easier?


-- 
Bye, Peter Korsgaard

[Buildroot] [PATCH 4/5] support/download: only create final temp file when needed

[Arnout Vandecappelle]

On 07/07/14 23:53, Yann E. MORIN wrote:
> Arnout, All,
> 
> On 2014-07-07 18:08 +0200, Arnout Vandecappelle spake thusly:
>> On 06/07/14 23:27, Yann E. MORIN wrote:
[snip]
>>> +# Upon failure, git cleans behind itself, so no need to catch failures here.
>>> +# The only case when git would not clean up, is if it gets killed with SIGKILL.
>>
>>  I think the SIGKILL is reason enough to do the rm explicitly in the script. Of
>> course, this is only valid if you use the trap, otherwise you never reach the rm
> 
> Well, SIGKILL is not catchable. That's the main selling point of SIGKILL.
> ;-)

 If git gets SIGKILLed, the shell script is still running.

> 
>> (due to 'set -e').
> 
> Well, 'set -e' only kicks in for the final 'mv', since all other
> commands and conditions in an if statement.

 The call to git clone isn't, though.

> 
> Also, I'm not a big fan of traps, although I can be abusing them from
> time to time.

 I do agree that trap is mightily confusing: it has obscure syntax, and you can
easily accidentally replace an earlier trap. So in fact, it would be better not
to use 'set -e' but to put everything in conditions. But that just moves the
problem elsewhere...


 Regards,
 Arnout

> 
> But OK, I'll add this to the TODO...
> 
> Regards,
> Yann E. MORIN.
> 


-- 
Arnout Vandecappelle                          arnout at mind be
Senior Embedded Software Architect            +32-16-286500
Essensium/Mind                                http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint:  7CB5 E4CC 6C2E EFD4 6E3D A754 F963 ECAB 2450 2F1F

[Buildroot] [PATCH 2/5] support/download: properly use temp files

[Arnout Vandecappelle]

On 07/07/14 23:38, Yann E. MORIN wrote:
> Arnout, All,
> 
> On 2014-07-07 08:11 +0200, Arnout Vandecappelle spake thusly:
>> On 06/07/14 23:27, Yann E. MORIN wrote:
>>> When using 'mv' to move files to temp files, the existing temp file is
>>> forcibly removed by 'mv', and a new file is created.
>>>
>>> Although this should have little impact to us, there is still a race
>>> conditions in case two parallel downloads would step on each other's
>>> toes, and it goes like that:
>>>
>>>     Process 1                       Process 2
>>>     mktemp tmp_output
>>>     mv tmp_dl tmp_output
>>>         removes tmp_output
>>>                                     mktemp tmp_ouput
>>>         writes to tmp_output
>>>                                     mv tmp_dl tmp_output
>>>                                         removes tmp_output
>>>                                         writes to tmp_output
>>>     mv tmp_output output
>>>                                     mv tmp_output output
>>>
>>> In this case, mktemp has the opportunity to create the same tmp_output
>>> temporary file, and we trash the content from one with the content
>>> from the other, and the last to do the final 'mv' breaks horibly.
>>>
>>> Instead, use 'cat', which guarantees that tmp_output is not removed
>>> before writing to it.
>>
>>  Not that it makes a real difference, but I think that 'cp' is a more natural
>> way to do this.
> 
> I am not sure how cp handles copying over an existing file. I'll
> check...

 It does. It only unlinks if open(O_TRUNC) fails. (Checked with strace for
coreutils and my reading the source for busybox.)


> 
>>> This complexifies a bit the local-files (cp) helper, but better safe
>>> than sorry.
>>>
>>> (Note: this is a purely theoretical issue, I did not observe it yet, but
>>> it is slated to happen one day or the other, and will be very hard to
>>> debug then.)
>>
>>  Actually, since the mktemp string is based on time and PID, the chance of this
>> happening is really vanishingly small. That said, better safe than sorry.
> 
> Yes, the opportunity for a collision is very low, but it's not
> impossible. After all, that's the problem with race conditions: given
> sufficient time and trials, you'll hit it, and it is very hard to debug,
> especially in this case, since the condition is not reproducible (random
> names.)

 Well, since mktemp uses /dev/urandom to generate the file pattern (I was wrong
about time/pid, that's only if urandom is not available), the 6-character random
number has a collision frequency of about 10^10. The chance that such a
collision occurs at exactly the moment that we're doing two parallel downloads
of the same source is pretty darn small - much smaller than the chance of a sha1
collision I think.

> 
>>> Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
>>> ---
>>>  support/download/bzr  | 2 +-
>>>  support/download/cp   | 9 ++++++---
>>>  support/download/cvs  | 2 +-
>>>  support/download/git  | 4 ++--
>>>  support/download/hg   | 2 +-
>>>  support/download/scp  | 2 +-
>>>  support/download/svn  | 2 +-
>>>  support/download/wget | 2 +-
>>>  8 files changed, 14 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/support/download/bzr b/support/download/bzr
>>> index 19d837d..f86fa82 100755
>>> --- a/support/download/bzr
>>> +++ b/support/download/bzr
>>> @@ -27,7 +27,7 @@ tmp_output="$( mktemp "${output}.XXXXXX" )"
>>>  
>>>  ret=1
>>>  if ${BZR} export --format=tgz "${tmp_dl}" "${repo}" -r "${rev}"; then
>>> -    if mv "${tmp_dl}" "${tmp_output}"; then
>>> +    if cat "${tmp_dl}" >"${tmp_output}"; then
>>>          mv "${tmp_output}" "${output}"
>>>          ret=0
>>>      fi
>>
>>  Not really related to this patch, but why do we need this ${tmp_dl} to begin
>> with? Especially since we're already "occupying" a tempfile in DL_DIR anyway.
> 
> The idea was not to pollute BR2_DL_DIR, in case the download fails.
> Hence this dance:
>   - download to a disposable area (BUILD_DIR);
>   - move to a temp file in BR2_DL_DIR;
>   - atomically rename to the final file.
> 
> But granted, since  we remove failed downloads, we can skip the
> intermediary temp file in BUILD_DIR (although I still do not like much
> the idea of polluting BR2_DL_DIR.)

 Well, now you pollute BR2_DL_DIR with ${tmp_output} instead of ${tmp_dl}. Oh,
now I get it: if a download is interrupted, you'll leave the partial download in
BUILD_DIR instead of DL_DIR. OK, sorry about the noise.

> 
> I think there is still a case for which we can't remove the temp file...
> Ah yes, if the user cancels a build with Ctrl-C, the script is aborted,
> so we leave a .XXXXXX file in BR2_DL_DIR.

 Ctrl-C is SIGINT so it can be trap'ped.

> 
> But even with the three-step dance above, there is still a small window
> for leaving out cruft in BR2_DL_DIR; but that window is much, much
> smaller than the download window.

 Ack.

> 
>>  Also, with this added complexity, the download helper scripts are becoming
>> quite similar. So wouldn't it be a good idea to refactor them?
> 
> I think we already had this discussion back when I introduced the series:
>     http://lists.busybox.net/pipermail/buildroot/2014-January/086826.html
> 
> But for now, there are a few important fixes I'd like be applied before
> going further.

 OK.


> 
>> I'm thinking of
>> something like this:
>>
>> support/download/helper:
>>
>> #!/bin/bash
>>
>> # We want to catch any command failure, and exit immediately
>> set -e
>>
>> # Generic download helper
>> # Call it with:
>> #   $1: specific download helper
>> #   $2: output file
>> #   ...: arguments for the specific download helper
>>
>> download="${1}"
>> output="${2}"
>> shift 2
>>
>> tmp_output="$( mktemp ... )"
>> if "${download}" "${tmp_output}" "$@"; then
>> 	# Blah blah need things to be atomic blah blah
>> 	mv "${tmp_output}" "${output}"
>> fi
> 
> Oh, you meant having a wrapper script above all the other helpers, and
> delegate to the helpers only the download, and handle the atomicity
> dance to the wrapper. Hmmm... Might be worth looking at, probably, yes.

 But if you have more urgent fixes, that's OK.

> 
>>  I expect there will be even more common stuff between the download helpers in
>> the future, so it makes sense to have this.
> 
> Well, for one, moving the hash check in that wrapper, for example...
> 
[snip]
>>> diff --git a/support/download/git b/support/download/git
>>> index badb491..b0031e5 100755
>>> --- a/support/download/git
>>> +++ b/support/download/git
>>> @@ -43,8 +43,8 @@ fi
>>>  
>>>  ret=1
>>>  pushd "${repodir}" >/dev/null
>>> -if ${GIT} archive --prefix="${basename}/" -o "${tmp_tar}" \
>>> -                  --format=tar "${cset}"; then
>>> +if ${GIT} archive --prefix="${basename}/" --format=tar "${cset}" \
>>> +                  >"${tmp_tar}"; then
>>
>>  What's the reason for this change? I've checked with strace, and git archive
>> doesn't seem to unlink, it just does open(O_TRUNC) like cp.
> 
> Ah, so you did strace both? I straced scp, and it does unlink.
> Are we sure all cp and all tar we can encounter will all use
> open(O_TRUNC) ?

 Well, the unlink behaviour of mv is rather surprising if you ask me. Normally
when you create a file that already exists, you'd want it to retain the
attributes it has (owner, group, mod, acl, symlink, hardlinks).

> 
> It'd rather go on the safe side, and not assume much about this,
> especially since that behaviour may change in the future, or may not
> show in older versions, or different versions.

 OK, fair enough.

> 
>>>      if gzip -c "${tmp_tar}" >"${tmp_output}"; then
>>>          mv "${tmp_output}" "${output}"
>>>          ret=0
>>> diff --git a/support/download/hg b/support/download/hg
>>> index 6e9e26b..8b36db9 100755
>>> --- a/support/download/hg
>>> +++ b/support/download/hg
>>> @@ -35,7 +35,7 @@ ret=1
>>>  if ${HG} clone --noupdate --rev "${cset}" "${repo}" "${repodir}"; then
>>>      if ${HG} archive --repository "${repodir}" --type tgz \
>>>                       --prefix "${basename}" --rev "${cset}" \
>>> -                     "${tmp_output}"; then
>>> +                     - >"${tmp_output}"; then
>>
>>  I didn't check for hg, but I also don't expect it to unlink() first. In fact,
>> it's mv's behaviour of unlinking first that is surprising.
> 
> Well, scp also unlinks first.

 No it doesn't... Or else I'm doing something really wrong with my strace...


 Regards,
 Arnout

> 
> And same answer, I don;t want to rely on such a behaviour. Redirecting
> works the same everywhere, though.
> 
>>>          mv "${tmp_output}" "${output}"
>>>          ret=0
>>>      fi
>>> diff --git a/support/download/scp b/support/download/scp
>>> index d3aad43..e16ece5 100755
>>> --- a/support/download/scp
>>> +++ b/support/download/scp
>>> @@ -17,7 +17,7 @@ tmp_output="$( mktemp "${output}.XXXXXX" )"
>>>  
>>>  ret=1
>>>  if ${SCP} "${url}" "${tmp_dl}"; then
>>> -    if mv "${tmp_dl}" "${tmp_output}"; then
>>> +    if cat "${tmp_dl}" >"${tmp_output}"; then
>>>          mv "${tmp_output}" "${output}"
>>>          ret=0
>>>      fi
>>> diff --git a/support/download/svn b/support/download/svn
>>> index 39cbbcb..259d3ed 100755
>>> --- a/support/download/svn
>>> +++ b/support/download/svn
>>> @@ -33,7 +33,7 @@ rm -rf "${repodir}"
>>>  
>>>  ret=1
>>>  if ${SVN} export "${repo}@${rev}" "${repodir}"; then
>>> -    if tar czf "${tmp_output}" "${repodir}"; then
>>> +    if tar czf - "${repodir}" >"${tmp_output}"; then
>>
>>  Again, tar doesn't unlink so this isn't needed.
> 
> Ditto.
> 
> Regards,
> Yann E. MORIN.
> 


-- 
Arnout Vandecappelle                          arnout at mind be
Senior Embedded Software Architect            +32-16-286500
Essensium/Mind                                http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint:  7CB5 E4CC 6C2E EFD4 6E3D A754 F963 ECAB 2450 2F1F

[Buildroot] [PATCH 2/5] support/download: properly use temp files

[Yann E. MORIN]

Arnout, All,

On 2014-07-08 18:42 +0200, Arnout Vandecappelle spake thusly:
> On 07/07/14 23:38, Yann E. MORIN wrote:
> > On 2014-07-07 08:11 +0200, Arnout Vandecappelle spake thusly:
[--SNIP--]
> >>  Not that it makes a real difference, but I think that 'cp' is a more natural
> >> way to do this.
> > 
> > I am not sure how cp handles copying over an existing file. I'll
> > check...
> 
>  It does. It only unlinks if open(O_TRUNC) fails. (Checked with strace for
> coreutils and my reading the source for busybox.)

I'll rework the entire series to take your and Jacmet's comments in
consideration.

Thanks for the reviews! :-)

Regards,
Yann E. MORIN.

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

[Buildroot] [PATCH 2/5] support/download: properly use temp files

[Thomas Petazzoni]

Yann, Arnout,

On Mon, 7 Jul 2014 23:38:02 +0200, Yann E. MORIN wrote:

> >  Not really related to this patch, but why do we need this ${tmp_dl} to begin
> > with? Especially since we're already "occupying" a tempfile in DL_DIR anyway.
> 
> The idea was not to pollute BR2_DL_DIR, in case the download fails.
> Hence this dance:
>   - download to a disposable area (BUILD_DIR);
>   - move to a temp file in BR2_DL_DIR;
>   - atomically rename to the final file.

And also because $(DL_DIR) and $(BUILD_DIR) might be on different
filesystems, so the rename/move of the file from $(BUILD_DIR) (where it
was downloaded) to $(DL_DIR) may not be atomic. Hence the idea is to:

 - Download in $(BUILD_DIR)

 - Move to a temporary file in $(DL_DIR). This operation may not be
   atomic if $(DL_DIR) is not on the same filesystem as $(BUILD_DIR)

 - Finally rename the temporary file to the expected file name in
   $(DL_DIR). Since we're in the same directory, it's guaranteed to be
   atomic.

This allows to ensure that when the final file appears in $(DL_DIR),
we're sure the download is finished, and that therefore concurrently
executing Buildroot instances will either not see the downloaded file,
or see a fully completed downloaded file.

Or am I missing the point of the discussion here?

Thanks,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[Buildroot] [PATCH 2/5] support/download: properly use temp files

[Yann E. MORIN]

Thomas, All,

On 2014-07-09 09:45 +0200, Thomas Petazzoni spake thusly:
> On Mon, 7 Jul 2014 23:38:02 +0200, Yann E. MORIN wrote:
> 
> > >  Not really related to this patch, but why do we need this ${tmp_dl} to begin
> > > with? Especially since we're already "occupying" a tempfile in DL_DIR anyway.
> > 
> > The idea was not to pollute BR2_DL_DIR, in case the download fails.
> > Hence this dance:
> >   - download to a disposable area (BUILD_DIR);
> >   - move to a temp file in BR2_DL_DIR;
> >   - atomically rename to the final file.
> 
> And also because $(DL_DIR) and $(BUILD_DIR) might be on different
> filesystems, so the rename/move of the file from $(BUILD_DIR) (where it
> was downloaded) to $(DL_DIR) may not be atomic. Hence the idea is to:
> 
>  - Download in $(BUILD_DIR)
> 
>  - Move to a temporary file in $(DL_DIR). This operation may not be
>    atomic if $(DL_DIR) is not on the same filesystem as $(BUILD_DIR)
> 
>  - Finally rename the temporary file to the expected file name in
>    $(DL_DIR). Since we're in the same directory, it's guaranteed to be
>    atomic.
> 
> This allows to ensure that when the final file appears in $(DL_DIR),
> we're sure the download is finished, and that therefore concurrently
> executing Buildroot instances will either not see the downloaded file,
> or see a fully completed downloaded file.
> 
> Or am I missing the point of the discussion here?

Not as far as I'm concerned.

Except that Arnout's (and Peter's) concerns where that the way I
implemented that was not optimal, and I used convoluted constructs to
achieve this non-cluttering and atomicity.

I have to re-write my copy! ;-)

Regards,
Yann E. MORIN.

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'