[PATCH] alsactl: Skip restore during the lock

[Jaroslav Kysela]

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 at perex.cz>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.