Re: Don't lock package db if fw_update(8) is not installing firmware

[Andrew Hewus Fresh]

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.sh    14 Oct 2023 18:10:47 -0000      1.51
> > +++ fw_update.sh    14 Oct 2023 18:12:08 -0000
> > @@ -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.sh        14 Oct 2023 18:10:47 -0000      1.51
+++ fw_update.sh        14 Oct 2023 19:39:25 -0000
@@ -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" )"