From 8c61324b6b60a7a7f27b4b81d8a9209127e43666 Mon Sep 17 00:00:00 2001 From: Will Thompson Date: Fri, 18 Sep 2020 11:09:35 +0100 Subject: [PATCH 1/6] sideload-repo-systemd: Set 'nullglob' option Previously, if there were no existing symlinks, this script would fail: wjt@camille:~$ LANG=en_GB.utf8 bash -x /usr/libexec/flatpak-create-sideload-symlinks.sh /run/media/wjt + test 1 -eq 1 + test -d /run/media/wjt + for f in "$1"/* + test -d '/run/media/wjt/*' + continue + for f in /run/flatpak/sideload-repos/automount-* + test -e '/run/flatpak/sideload-repos/automount-*' + rm '/run/flatpak/sideload-repos/automount-*' rm: cannot remove '/run/flatpak/sideload-repos/automount-*': No such file or directory This is due to the surprising POSIX shell behaviour that a glob that matches no files expands to itself, rather than to the empty list. http://mywiki.wooledge.org/BashFAQ/004 The POSIX solution is to add 'test -L $f' inside the loop to check if the variable actually exists. The first loop in this script uses this technique: it has a 'test -d "$f"', seen in the trace above. Bash implements a 'nullglob' feature which gives globs the behaviour you might hope for. Set it in this script. --- sideload-repos-systemd/flatpak-create-sideload-symlinks.sh | 1 + 1 file changed, 1 insertion(+) diff --git a/sideload-repos-systemd/flatpak-create-sideload-symlinks.sh b/sideload-repos-systemd/flatpak-create-sideload-symlinks.sh index 0e4c57f0..ca599b99 100755 --- a/sideload-repos-systemd/flatpak-create-sideload-symlinks.sh +++ b/sideload-repos-systemd/flatpak-create-sideload-symlinks.sh @@ -1,6 +1,7 @@ #!/bin/bash # This script is intended to be run by flatpak-sideload-usb-repo.service +shopt -s nullglob if ! test $# -eq 1 || ! test -d "$1"; then echo "Error: first argument must be a directory" From c43e8e1331ce8d1886ba03c81503d8b6e82269b5 Mon Sep 17 00:00:00 2001 From: Will Thompson Date: Fri, 18 Sep 2020 11:57:38 +0100 Subject: [PATCH 2/6] sideload-repos-systemd: Don't launch service in a tight loop man systemd.path has the following to say: > When a service unit triggered by a path unit terminates (regardless > whether it exited successfully or failed), monitored paths are checked > immediately again, and the service accordingly restarted instantly. On my system, I observe that once /run/media/wjt is created, flatpak-sideload-usb-repo.service is invoked in a tight loop. I think what's happening is that PathExists=/run/media/wjt continues to be true, so the service keeps getting restarted. What we instead want is to activate the .service: - On login - When the media directory *changes* (because mount points beneath it appear or disappear) Remove PathExists from the .path, and instead mark the .service as WantedBy default.target so it is launched on login. --- sideload-repos-systemd/flatpak-sideload-usb-repo.path.in | 1 - sideload-repos-systemd/flatpak-sideload-usb-repo.service.in | 3 +++ 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/sideload-repos-systemd/flatpak-sideload-usb-repo.path.in b/sideload-repos-systemd/flatpak-sideload-usb-repo.path.in index 095b7102..be63cccf 100644 --- a/sideload-repos-systemd/flatpak-sideload-usb-repo.path.in +++ b/sideload-repos-systemd/flatpak-sideload-usb-repo.path.in @@ -5,7 +5,6 @@ # have flatpaks on them, both when a new drive is inserted and at the start of # the user session. [Path] -PathExists=@media_dir@/%u PathChanged=@media_dir@/%u [Install] diff --git a/sideload-repos-systemd/flatpak-sideload-usb-repo.service.in b/sideload-repos-systemd/flatpak-sideload-usb-repo.service.in index 8f8895da..a3f4c93a 100644 --- a/sideload-repos-systemd/flatpak-sideload-usb-repo.service.in +++ b/sideload-repos-systemd/flatpak-sideload-usb-repo.service.in @@ -3,3 +3,6 @@ [Service] Type=oneshot ExecStart=@libexecdir@/flatpak-create-sideload-symlinks.sh @media_dir@/%u + +[Install] +WantedBy=default.target From 9b82fbfabbba98fec68940ffdaad4a7324c41593 Mon Sep 17 00:00:00 2001 From: Phaedrus Leeds Date: Mon, 21 Sep 2020 18:14:38 -0700 Subject: [PATCH 3/6] sideload-repos-systemd: Adjust whitespace --- sideload-repos-systemd/flatpak-create-sideload-symlinks.sh | 1 + 1 file changed, 1 insertion(+) diff --git a/sideload-repos-systemd/flatpak-create-sideload-symlinks.sh b/sideload-repos-systemd/flatpak-create-sideload-symlinks.sh index ca599b99..9b8c8705 100755 --- a/sideload-repos-systemd/flatpak-create-sideload-symlinks.sh +++ b/sideload-repos-systemd/flatpak-create-sideload-symlinks.sh @@ -1,6 +1,7 @@ #!/bin/bash # This script is intended to be run by flatpak-sideload-usb-repo.service + shopt -s nullglob if ! test $# -eq 1 || ! test -d "$1"; then From 49096521fd4676f1b8573e36298eca2b6ac68651 Mon Sep 17 00:00:00 2001 From: Phaedrus Leeds Date: Mon, 21 Sep 2020 18:16:26 -0700 Subject: [PATCH 4/6] sideload-repos-systemd: Make sym link more human friendly Use escaping rather than checksumming to generate a unique name, as discussed here: https://github.com/flatpak/flatpak/pull/3866#issuecomment-694784359 --- sideload-repos-systemd/flatpak-create-sideload-symlinks.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/sideload-repos-systemd/flatpak-create-sideload-symlinks.sh b/sideload-repos-systemd/flatpak-create-sideload-symlinks.sh index 9b8c8705..0fbd03c1 100755 --- a/sideload-repos-systemd/flatpak-create-sideload-symlinks.sh +++ b/sideload-repos-systemd/flatpak-create-sideload-symlinks.sh @@ -17,7 +17,7 @@ for f in "$1"/*; do if ! test -d "$f"; then continue fi - unique_name=automount-$(echo "$f" | sha256sum | cut -f 1 -d ' ') + unique_name=automount$(systemd-escape "$f") if test -e "/run/flatpak/sideload-repos/$unique_name"; then continue fi @@ -25,7 +25,7 @@ for f in "$1"/*; do done # Remove any broken symlinks e.g. from drives that were removed -for f in /run/flatpak/sideload-repos/automount-*; do +for f in /run/flatpak/sideload-repos/automount*; do if ! test -e "$f"; then rm "$f" fi From 093b22f5a395e573e6ea24ebf116f5a280212c6d Mon Sep 17 00:00:00 2001 From: Phaedrus Leeds Date: Mon, 21 Sep 2020 18:25:08 -0700 Subject: [PATCH 5/6] sideload-repo-systemd: Only remove owned sym links Check file ownership to ensure flatpak-create-sideload-symlinks.sh only cleans up links it created. This could be relevant on multi-user systems with fast user switching. --- sideload-repos-systemd/flatpak-create-sideload-symlinks.sh | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/sideload-repos-systemd/flatpak-create-sideload-symlinks.sh b/sideload-repos-systemd/flatpak-create-sideload-symlinks.sh index 0fbd03c1..4b3aabd6 100755 --- a/sideload-repos-systemd/flatpak-create-sideload-symlinks.sh +++ b/sideload-repos-systemd/flatpak-create-sideload-symlinks.sh @@ -26,6 +26,10 @@ done # Remove any broken symlinks e.g. from drives that were removed for f in /run/flatpak/sideload-repos/automount*; do + OWNER=$(stat -c '%u' "$f") + if [ "$UID" != "$OWNER" ]; then + continue + fi if ! test -e "$f"; then rm "$f" fi From 192da15f6017c2bb6640437f49f62530614451a5 Mon Sep 17 00:00:00 2001 From: Phaedrus Leeds Date: Mon, 21 Sep 2020 20:17:42 -0700 Subject: [PATCH 6/6] sideload-repos-systemd: Update a comment --- sideload-repos-systemd/flatpak-sideload-usb-repo.path.in | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/sideload-repos-systemd/flatpak-sideload-usb-repo.path.in b/sideload-repos-systemd/flatpak-sideload-usb-repo.path.in index be63cccf..3e7d26ad 100644 --- a/sideload-repos-systemd/flatpak-sideload-usb-repo.path.in +++ b/sideload-repos-systemd/flatpak-sideload-usb-repo.path.in @@ -1,6 +1,6 @@ # This unit is intended to be installed in the systemd user instance, and -# depends on flatpak-sideload-repos-dir.service being in the system instance -# and running first. The idea here is that we add any USB drive mounts to the +# depends on /run/flatpak/sideload-repos having been created via +# systemd-tmpfiles. The idea here is that we add any USB drive mounts to the # appropriate directory so Flatpak can find and pull from them in case they # have flatpaks on them, both when a new drive is inserted and at the start of # the user session.