Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RFC: propose helper script interface for writing rules #70

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Ella-0
Copy link

@Ella-0 Ella-0 commented Jul 10, 2024

Proposes the use of a helper script to allow setting udev properties as a replacement for udev rules. For example setting ACP_IGNORE=1 on the visense pcm in apple silicon macs.

#!/bin/sh -e

if printf '%s\n' "$SYSNAME" | grep '^pcmC.*D2c$'
then
	printf 'ACP_IGNORE=1\n'
fi

udev_device.c Fixed Show fixed Hide fixed
Copy link
Owner

@illiliti illiliti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this! I like the idea of passing control of properties to a helper script. It's much better option than adding special handling for properties.

udev_device.c Outdated Show resolved Hide resolved
udev_device.c Outdated Show resolved Hide resolved
@xplshn
Copy link

xplshn commented Jul 11, 2024

@illiliti Will this not be merged into master/main?

@illiliti
Copy link
Owner

@illiliti Will this not be merged into master/main?

I like the idea, so it is highly likely I'll merge this. Just need to sort out issues with code.

@Ella-0 Ella-0 force-pushed the master branch 3 times, most recently from 5ec5355 to 537ce84 Compare July 11, 2024 17:57
@Ella-0
Copy link
Author

Ella-0 commented Jul 11, 2024

Addressed all comments and added an example rules script to contrib with the SOUND_INITIALIZED and ACP_IGNORE stuff. Note: this on its own will not fix pipewire since pipewire still depends on the enumeration of parent devices

@illiliti
Copy link
Owner

Note: this on its own will not fix pipewire since pipewire still depends on the enumeration of parent devices

One could access parent device via $SYSPATH/../ or $SYSPATH/device/, so unless I'm missing something it is indeed possible to fix pipewire.

@Ella-0
Copy link
Author

Ella-0 commented Jul 11, 2024

Note: this on its own will not fix pipewire since pipewire still depends on the enumeration of parent devices

One could access parent device via $SYSPATH/../ or $SYSPATH/device/, so unless I'm missing something it is indeed possible to fix pipewire.

The problem with pipewire is it enumerates the following devices

/sys/devices/platform/sound/sound/card0/pcmC0D0p
/sys/devices/platform/sound/sound/card0/pcmC0D1p
/sys/devices/virtual/sound/timer
/sys/devices/platform/sound/sound/card0/pcmC0D0c
/sys/devices/platform/sound/sound/card0/pcmC0D2c
/sys/devices/platform/sound/sound/card0/controlC0

however the following is intended. card0 is missing. this is due to scan_devices working on /sys/dev/{char,block} but card? does not have an associated character or block device so isn't enumerated.

/sys/devices/platform/sound/sound/card0
/sys/devices/platform/sound/sound/card0/pcmC0D0p
/sys/devices/platform/sound/sound/card0/pcmC0D1p
/sys/devices/virtual/sound/timer
/sys/devices/platform/sound/sound/card0/pcmC0D0c
/sys/devices/platform/sound/sound/card0/pcmC0D2c
/sys/devices/platform/sound/sound/card0/controlC0

@illiliti
Copy link
Owner

Ah! Right. Forgot about this one. Well, we could switch our enumeration approach to appease pipewire. Let's keep it for later.

udev_device.c Outdated Show resolved Hide resolved
udev_device.c Outdated Show resolved Hide resolved
udev_device.c Outdated Show resolved Hide resolved
udev_device.c Outdated Show resolved Hide resolved
udev_device.c Outdated Show resolved Hide resolved
udev_device.c Outdated Show resolved Hide resolved
udev_device.c Outdated Show resolved Hide resolved
udev_device.c Outdated Show resolved Hide resolved
udev_device.c Outdated Show resolved Hide resolved
@Ella-0 Ella-0 force-pushed the master branch 3 times, most recently from 43383b5 to 4f12d0a Compare July 12, 2024 00:33
udev_device.c Outdated Show resolved Hide resolved
udev_device.c Show resolved Hide resolved
udev_device.c Show resolved Hide resolved
udev_device.c Outdated Show resolved Hide resolved
udev_device.c Outdated Show resolved Hide resolved
udev_device.c Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
@Ella-0 Ella-0 force-pushed the master branch 3 times, most recently from 41a653f to 5e8772f Compare July 13, 2024 08:36
Makefile Outdated Show resolved Hide resolved
udev_device.c Outdated Show resolved Hide resolved
char *argv[] = { argv0, NULL };

if (posix_spawn(&pid, argv0, &actions, NULL, argv, envp)) {
return -1;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

close out_pipe and destroy actions

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

int ret = posix_spawn(...);
posix_spawn_file_actions_destroy(&actions);
close(out_pipe[1]);
if (ret != 0) {
    close(out_pipe[0]);
    return -1;
}

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are still leaking out_pipe and actions here. Consider adopting the above code to fix the leak.

udev_device.c Show resolved Hide resolved
udev_device.c Fixed Show resolved Hide resolved

int out_pipe[2];

if (pipe2(out_pipe, O_CLOEXEC)) {

Check warning

Code scanning / CodeQL

Implicit function declaration

Function call implicitly declares 'pipe2'.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants