[PATCH] alsactl: Skip restore during the lock

[Takashi Iwai]

On Fri, 11 Dec 2020 18:23:03 +0100,
Jaroslav Kysela wrote:
> 
> Dne 11. 12. 20 v 18:06 Takashi Iwai napsal(a):
> > On Fri, 11 Dec 2020 17:59:05 +0100,
> > Takashi Iwai wrote:
> >>
> >> On Fri, 11 Dec 2020 17:45:45 +0100,
> >> Jaroslav Kysela wrote:
> >>>
> >>> Dne 11. 12. 20 v 9:38 Takashi Iwai napsal(a):
> >>>> Currently alsactl-restore tries to initialize the device when an error
> >>>> is found for restore action.  But this isn't the right behavior in the
> >>>> case where the lock is held; it implies that another alsactl is
> >>>> running concurrently, hence you shouldn't initialize the card at the
> >>>> same time.  The situation is found easily when two alsactls get
> >>>> started by both udev and systemd (note that those two invocations are
> >>>> the designed behavior, see /usr/lib/udev/rules.d/78-sound-cards.rules
> >>>> for details).
> >>>>
> >>>> This patch changes load_state() not to handle the initialization if
> >>>> the locking fails.
> >>>
> >>> The operation should serialize in this case (there's limit of 10 seconds which
> >>> should be enough to finish the initialization). The state_lock() function
> >>> should return -EBUSY when the file is locked (and I'm fine to change the
> >>> behaviour from 'init' to 'skip' for this lock state).
> >>>
> >>> It seems that -EEXIST is returned when the lock file exists, but the
> >>> open(file, O_CREAT|O_EXCL, 0644) caller has not enough priviledges to access
> >>> this file when another user owns the file.
> >>>
> >>> But alsactl from /lib/udev/rules.d/90-alsa-restore.rules and
> >>> /lib/systemd/system/alsa-restore.service should be run as root, right?
> >>
> >> Yes, it should be root.
> >>
> >> I also wondered how EEXIST comes, too.  Maybe it's also the race
> >> between the first open(O_RDWR) and the second
> >> open(O_RDWR|O_CREAT|O_EXCL)?  If so, it'd be better to go back again
> >> to the normal open(O_RDWR)?
> > 
> > ... something like below
> > 
> > 
> > diff --git a/alsactl/lock.c b/alsactl/lock.c
> > index 4a485392b3bd..c1c30f0c5eee 100644
> > --- a/alsactl/lock.c
> > +++ b/alsactl/lock.c
> > @@ -64,6 +64,9 @@ static int state_lock_(const char *file, int lock, int timeout, int _fd)
> >  				if (errno == EBUSY || errno == EAGAIN) {
> >  					sleep(1);
> >  					timeout--;
> > +				} if (errno == EEXIST){
> > +					/* race at creating a lock, try again */
> > +					continue;
> >  				} else {
> >  					err = -errno;
> >  					goto out;
> 
> If we don't use the sleep call and the timeout counter, there's endless CPU
> busy loop when the root creates the lock file and user wants to access it for
> example. It's better to add EEXIST to the previous errno condition.

The timeout is decreased in the while condition.


Takashi