Re: [PATCH] alsactl: Skip restore during the lock

[Jaroslav Kysela <perex@perex.cz>]

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.

					Jaroslav

-- 
Jaroslav Kysela <perex@perex.cz>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.