On Fri, 11 Dec 2020 18:56:56 +0100,
Jaroslav Kysela wrote:
>
> Dne 11. 12. 20 v 18:37 Takashi Iwai napsal(a):
> > 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.
>
> It seems not correct. It decreases the wait time to half. My fault :-(
>
> What about this straight change:
>
> --- a/alsactl/lock.c
> +++ b/alsactl/lock.c
> @@ -63,11 +63,15 @@ static int state_lock_(const char *file, int lock, int
> timeout, int _fd)
> if (fd < 0) {
> if (errno == EBUSY || errno == EAGAIN) {
> sleep(1);
> - timeout--;
> - } else {
> - err = -errno;
> - goto out;
> + continue;
> }
> + if (errno == EEXIST) {
> + fd = open(nfile, O_RDWR);
> + if (fd >= 0)
> + break;
> + }
> + err = -errno;
> + goto out;
> }
> }
> }
Yes, this should work. Shall I resubmit? I'd split to two, one to
correct doubly timeout decreases and another to handle EEXIST.
thanks,
Takashi
