Skip to content

Commit

Permalink
hid-xpadneo, udev: Move udev rules up in rules priority
Browse files Browse the repository at this point in the history 
If we let udev evaluate our rules at position 60 (where most other
input rules sit), we can actually make use of uaccess and seat tags.
This allows proper access to the hidraw device because udev will now
assign ACLs for the current seat user to the device nodes.

Signed-off-by: Kai Krakow <[email protected]>
  • Loading branch information
@kakra
kakra committed Mar 24, 2021
1 parent 5b83a12 commit eee6d5b
Showing 1 changed file with 1 addition and 1 deletion.
2 changes: 1 addition & 1 deletion ...xpadneo/etc-udev-rules.d/98-xpadneo.rules → ...xpadneo/etc-udev-rules.d/60-xpadneo.rules
View file
Original file line number Diff line number Diff line change
@@ -1,2 +1,2 @@
ACTION=="add", KERNEL=="0005:045E:02FD.*|0005:045E:02E0.*|0005:045E:0B05.*|0005:045E:0B13.*", SUBSYSTEM=="hid", DRIVER!="xpadneo", ATTR{driver/unbind}="%k", ATTR{[drivers/hid:xpadneo]bind}="%k"
ACTION=="add|change", DRIVERS=="xpadneo", SUBSYSTEM=="input", ENV{ID_INPUT_JOYSTICK}=="1", TAG+="uaccess", MODE="0664"
ACTION=="add|change", DRIVERS=="xpadneo", SUBSYSTEM=="hidraw|input", TAG+="uaccess"

9 comments on commit eee6d5b

@terencode
Copy link

Choose a reason for hiding this comment

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

You also need to update dkms.post_remove and dkms.post_install because they are still using the old filename.

@kakra
Copy link
Collaborator Author

@kakra kakra commented on eee6d5b Mar 24, 2021

Choose a reason for hiding this comment

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

You should get an xpadneo superhero badge

@kakra
Copy link
Collaborator Author

@kakra kakra commented on eee6d5b Mar 24, 2021

Choose a reason for hiding this comment

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

DKMS is really dumb: It doesn't detect errors in the post install script, so our CI is useless here... Even if I force abort the post install script with an error, it happily continues saying everything went okay. I'll deal with that later and will push an update now fixing what you found. Thanks again.

@terencode
Copy link

@terencode terencode commented on eee6d5b Mar 24, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

DKMS is really dumb: It doesn't detect errors in the post install script, so our CI is useless here... Even if I force abort the post install script with an error, it happily continues saying everything went okay.

That's annoying, it might be
worth to open an issue upstream.

Thanks again.

Always a pleasure ^^

@kakra
Copy link
Collaborator Author

@kakra kakra commented on eee6d5b Mar 24, 2021

Choose a reason for hiding this comment

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

worth to open an issue upstream.

Yeah maybe... I already had to dive into the dkms source code to find a way to dump make.log to the console in case of errors so I could see it in the CI pipeline - it's an ugly hack. It just gets swallowed by dkms, leaving you a message to read this file for more details - of course it's already gone at that point because the CI bot will kick the machine from memory after the test is done. It would be beneficial if dkms had a switch to log verbosely. Looking at the code gives me nightmares: It's purely a monolithic shell script, and it's not checking for errors at uncountable places - it just assumes everything works fine and happily continues, leaving you with some undefined and broken system state. Sometimes, shell script is just not suitable for a task... :-\

Well, here's the report: dell/dkms#148

@terencode
Copy link

Choose a reason for hiding this comment

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

Guess who also had an issue with dkms not allowing you to debug problems correctly? dell/dkms#94

@kakra
Copy link
Collaborator Author

@kakra kakra commented on eee6d5b Mar 26, 2021

Choose a reason for hiding this comment

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

This is why I added this

cat_dkms_make_log() {
	local last_error=$?
	if [ -n "${V[*]}" ]; then
		cat "/var/lib/dkms/hid-xpadneo/${VERSION}/build/make.log" || true
	fi
	exit ${last_error}
}

and this

dkms add "${V[*]}" "hid-xpadneo/${VERSION}" || cat_dkms_make_log

But your suggestion in that report is misleading: The error may not be specific to some rule or makefile location, it may simply be a build error that gets hidden away by dkms. It really should have a verbose mode where it doesn't hide any output into log files that get purged by auto-build scripts afterwards. Because CI captures screen output, not log files.

@terencode
Copy link

@terencode terencode commented on eee6d5b Mar 26, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I know the error is not specific to rule/makefile location but when it is, it's even harder to know the issue. With regular build errors I had no issue looking up the make.log and seeing where compilation failed.
My suggestion is just to warn about this possibility, not telling you this is for sure the problem, so I fail to see why it's misleading.

@kakra
Copy link
Collaborator Author

@kakra kakra commented on eee6d5b Mar 26, 2021

Choose a reason for hiding this comment

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

With regular build errors I had no issue looking up the make.log and seeing where compilation failed.

Unless you don't have the build log... I had a problem where building locally worked just fine but in CI it didn't. But of course when CI posts to Github, the machine that built the project is already torn down, leaving you only with the console log - which points you to a make.log that's long gone.

Please sign in to comment.