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;
}
}
}
Jaroslav
--
Jaroslav Kysela <perex@xxxxxxxx>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.
