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

Implement OpenRC service #227

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

BalkanMadman
Copy link
Contributor

This is still a draft.
This PR adds two flags to configure script:

  • --with-openrc: toggles on or off (default — off) OpenRC service installation and udev activation via that. Conflicts with systemd, so the latter must be disabled explicitly — --with-openrc --without-systemd
  • --with-rcservicedir: specifies directory where to install service file. Defaults to $sysconfdir/init.d (which is /etc/init.d)

I need the most feedback on which options should be used with usbmuxd. The only one used here is --user usbmux. Maybe --udev should be added? I don't know usbmuxd architecture that well, so a few advises on this matter won't hurt much :)

@BalkanMadman
Copy link
Contributor Author

Issue: #210

@BalkanMadman BalkanMadman force-pushed the service-openrc branch 2 times, most recently from 893c9ca to 881f59a Compare August 29, 2023 19:41
@nikias
Copy link
Member

nikias commented Sep 18, 2023

What's the status of this?

@BalkanMadman
Copy link
Contributor Author

Oh, I am sorry it took me so long, still being on vacation I haven't got the chance to actually test the commit. I will be able to fully work on this in two weeks.

The reason why it is still a draft in the lack of proper testing. This is based on master, which requires libimobiledevire-glue that is not yet packaged by Gentoo (which itself depends on libplist 2.3.0 that also happens to not be packaged), so you know. But it does look good when is installed to a DESTDIR (the fact on itself may not be enough to merge the PR).

One thing left, which does not depend on me is the flags used in the service script. Could you confirm/verify I used the correct ones?

command_args="--user usbmux"

In addition to that I would also like to get some feedback on code styling and change what is deemed necessary

@nikias
Copy link
Member

nikias commented Sep 19, 2023

One thing left, which does not depend on me is the flags used in the service script. Could you confirm/verify I used the correct ones?

command_args="--user usbmux"

Yes, that is correct. It's -U or --user.

In addition to that I would also like to get some feedback on code styling and change what is deemed necessary

Looks good to me.

@blshkv
Copy link

blshkv commented Jan 16, 2024

The reason why it is still a draft in the lack of proper testing. This is based on master, which requires libimobiledevire-glue that is not yet packaged by Gentoo (which itself depends on libplist 2.3.0 that also happens to not be packaged), so you know. But it does look good when is installed to a DESTDIR (the fact on itself may not be enough to merge the PR).
med necessary

FYI, libimobiledevice-glue is here:
pentoo/pentoo-overlay@f5164d2
libplist 2.3.0 is in the main tree.

Hope to see some progress soon.

@blshkv
Copy link

blshkv commented Jan 16, 2024

I checked the code I realised that you run "/etc/init.d/usbmuxd start" from udev rule? This is just wow ...
Please don't do it. Learn how to do it properly instead.

@BalkanMadman
Copy link
Contributor Author

I checked the code I realised that you run "/etc/init.d/usbmuxd start" from udev rule? This is just wow ... Please don't do it. Learn how to do it properly instead.

udev(7) specifies the only way to run daemons from udev and that is using SYSTEMD_WANTS property, which, logically, has no effect on OpenRC system (or any non-systemd init system).
Quoting:

Starting daemons or other long-running processes is not
allowed; the forked processes, detached or not, will be
unconditionally killed after the event handling has finished.
In order to activate long-running processes from udev rules,
provide a service unit and pull it in from a udev device
using the SYSTEMD_WANTS device property. See
systemd.device(5) for details.

/etc/init.d/usbmuxd is a short-running process that immediately exits after starting the daemon (the daemon itself is not child of the udev process but of the OpenRC service system). I might be mistaken somewhere, so feel free to correct me if it is the case.
A similar, albeit slightly different in implementation, method of launching daemons by udev is even present on the Gentoo wiki here.

From my testing with this patch applied to usbmuxd-1.1.1, it seems to have no effect on the occurrence of the bug. Yet, this seems to be much more compliant way to execute daemons with udev and should theoretically be merged.
The bug most likely will not be fixed by this, so the commit should be regarded as an enhancement, not a bug fix.
I will work on isolating and locating the root cause.

@BalkanMadman
Copy link
Contributor Author

And the commit itself should be cleaned up a bit...

@BalkanMadman BalkanMadman force-pushed the service-openrc branch 2 times, most recently from 98601f5 to 7fbeafe Compare January 16, 2024 21:17
@BalkanMadman
Copy link
Contributor Author

I suppose this is ready to be reviewed

@BalkanMadman BalkanMadman marked this pull request as ready for review January 16, 2024 21:37
@blshkv
Copy link

blshkv commented Jan 17, 2024

ok, maybe I'm wrong about running openrc. I was sure that udev script should execute its own /lib/udev/ only. But I found sys-apps/pcsc-lite, as an example, which installs /lib/udev/pcscd.sh and runs the following:

# make sure openrc is managing services
if [ ! -d /run/openrc ]; then
        exit 0
fi

IN_HOTPLUG=1 /etc/init.d/pcscd --quiet start

You might want to check what IN_HOTPLUG does, and add --quiet

@BalkanMadman
Copy link
Contributor Author

ok, maybe I'm wrong about running openrc. I was sure that udev script should execute its own /lib/udev/ only. But I found sys-apps/pcsc-lite, as an example, which installs /lib/udev/pcscd.sh and runs the following:

# make sure openrc is managing services
if [ ! -d /run/openrc ]; then
        exit 0
fi

IN_HOTPLUG=1 /etc/init.d/pcscd --quiet start

You might want to check what IN_HOTPLUG does, and add --quiet

I have already added the --quiet option:

usbmuxd/configure.ac

Lines 118 to 119 in 507cf94

udev_activation_rule="RUN+=\"@rcservicedir@/usbmuxd start --quiet\""
udev_deactivation_rule="RUN+=\"@rcservicedir@/usbmuxd stop --quiet\""

For a service marked with the IN_HOTPLUG to be displayed as, well, hotplugged, its name must be specified manually in the main OpenRC config /etc/rc.conf (AFAIK, it can not be set per-service in /etc/conf.d), which is rather uncomfortable and cumbersome for most users. I suppose, it is practically useless to install a whole shell script with no effect in 99% cases.

This commit adds a usbmuxd service script for the OpenRC init system.
It intends to conform more closely to the way services are started by
udev. Previously, on non-systemd systems, udev would start the `usbmuxd`
binary directly. This commits makes udev start `usbmuxd` in a much
cleaner way, by using the OpenRC service manager.

This adds two new `configure` flags:
* `--with-openrc`: toggles on or off (default - off) OpenRC service
  installation and udev activation via that. Conflicts with systemd, so
  the latter must be disabled explicitly:
  `--with-openrc --without-systemd`
* `--with-rcservicedir=DIR`: specifies directory where to install service
  file. Defaults to `$sysconfdir/init.d` (which is `/etc/init.d`)

Issue: libimobiledevice#210
Signed-off-by: BalkanMadman <[email protected]>
@BalkanMadman
Copy link
Contributor Author

I have slightly reworded the commit message to better reflect what the change actually does.
@nikias, could you have a look and merge this PR?

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