On Sat, Oct 14, 2023 at 09:02:41PM +0200, Marc Espie wrote:
> On Sat, Oct 14, 2023 at 11:24:08AM -0700, Andrew Hewus Fresh wrote:
> > While testing another patch, I noticed that fw_update will lock the
> > package database even if it's just downloading and not touching the
> > installed packages.
> >
> > Currently we do _read_ the existing firmware as part of detecting what
> > we might need to download or upgrade without locking. I'm unsure if
> > that is an issue.
> >
> > Comments, OK?
> >
> >
> >
> > Index: fw_update.sh
> > ===
> > RCS file: /cvs/src/usr.sbin/fw_update/fw_update.sh,v
> > retrieving revision 1.51
> > diff -u -p -r1.51 fw_update.sh
> > --- fw_update.sh14 Oct 2023 18:10:47 - 1.51
> > +++ fw_update.sh14 Oct 2023 18:12:08 -
> > @@ -593,7 +593,7 @@ kept=''
> > unregister=''
> >
> > if [ "${devices[*]:-}" ]; then
> > - lock_db
> > + "$INSTALL" && lock_db
> > for f in "${devices[@]}"; do
> > d="$( firmware_devicename "$f" )"
> >
> >
> >
> the package tools have two lock modes: read-only and read-write.
> If you want to match them, you want lock read-only for this.
Everywhere but during detection "installed_firmware" is called with the
read-write lock.
I think using that read-only lock during detection would look like this.
Index: fw_update.sh
===
RCS file: /cvs/src/usr.sbin/fw_update/fw_update.sh,v
retrieving revision 1.51
diff -u -p -r1.51 fw_update.sh
--- fw_update.sh14 Oct 2023 18:10:47 - 1.51
+++ fw_update.sh14 Oct 2023 19:39:25 -
@@ -69,7 +69,7 @@ cleanup() {
fi
[ "${FTPPID:-}" ] && kill -TERM -"$FTPPID" 2>/dev/null
- [ "${LOCKPID:-}" ] && kill -TERM -"$LOCKPID" 2>/dev/null
+ unlock_db
[ "${FWPKGTMP:-}" ] && rm -rf "$FWPKGTMP"
"$REMOVE_LOCALSRC" && rm -rf "$LOCALSRC"
[ -e "$CFILE" ] && [ ! -s "$CFILE" ] && rm -f "$CFILE"
@@ -259,7 +259,7 @@ lock_db() {
[ -e /usr/bin/perl ] || return 0
set -o monitor
- perl <<-'EOL' |&
+ _shared=${1:-0} perl <<-'EOL' |&
no lib ('/usr/local/libdata/perl5/site_perl');
use v5.36;
use OpenBSD::PackageInfo qw< lock_db >;
@@ -267,7 +267,7 @@ lock_db() {
$|=1;
$0 = "fw_update: lock_db";
- lock_db(0);
+ lock_db($ENV{_shared});
say $$;
@@ -284,6 +284,20 @@ EOL
return 0
}
+unlock_db() {
+ [ "${LOCKPID:-}" ] || return 0
+ local _seconds=0
+
+ kill -TERM -"$LOCKPID"
+
+ while kill -0 -"$LOCKPID" 2>/dev/null; do
+ sleep 1
+ ((_seconds++ > 5)) && kill -KILL -"$LOCKPID"
+ done
+
+ unset LOCKPID
+}
+
installed_firmware() {
local _pre="$1" _match="$2" _post="$3" _firmware _fw
set -sA _firmware -- $(
@@ -581,7 +595,9 @@ if [ "${devices[*]:-}" ]; then
"$ALL" && warn "Cannot use -a and devices/files" && usage
else
((VERBOSE > 1)) && echo -n "Detect firmware ..."
+ lock_db 1
set -sA devices -- $( detect_firmware )
+ unlock_db
((VERBOSE > 1)) &&
{ [ "${devices[*]:-}" ] && echo " found." || echo " done." ; }
fi
@@ -593,7 +609,7 @@ kept=''
unregister=''
if [ "${devices[*]:-}" ]; then
- lock_db
+ "$INSTALL" && lock_db
for f in "${devices[@]}"; do
d="$( firmware_devicename "$f" )"
