[Bug 1814682] Review Request: rshim - rshim driver for Mellanox BlueField SoC

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



https://bugzilla.redhat.com/show_bug.cgi?id=1814682



--- Comment #47 from Michal Schmidt <mschmidt@xxxxxxxxxx> ---
(In reply to lsun from comment #32)
> As for the "systemd-units", it appears to be required by koji. I got some
> failures like below once removed it. So I'll leave it for now (please
> correct me if I am incorrect).
> https://koji.fedoraproject.org/koji/taskinfo?taskID=43163313

OK, in that case leaving a BuildRequires there is acceptable.
Note that the "systemd-units" package was merged into the main "systemd"
package in 2012 (before Fedora 18). Fedora packaging guidelines removed the
last reference to "systemd-units" in 2016, keeping "BuildRequires: systemd" as
the preferred way.

There is another option. You can remove the BR and instead tell your configure
script to not autodetect the systemd units directory, using:
  %configure --with-systemdsystemunitdir=%{_unitdir}
The minor advantage of this would be a smaller buildroot.

Use whatever option you prefer there.

(In reply to lsun from comment #46)
> Spec URL: https://github.com/Mellanox/rshim-user-space/releases/download/rshim-2.0.3/rshim.spec

> Requires: psmisc

Why does the rshim.service use killall in the first place? There is:
> KillMode=process

Why this mode? Do you need child processes to be left running in the cgroup
after the service is stopped?
If yes, commenting on it in the unit file would be nice.

> ExecStop=/usr/bin/killall -SIGKILL rshim

Referencing processes to kill by name is not good. It would kill unrelated
processes with the same name.
Is none of systemd's kill modes suitable for stopping the service without
additional help?
And why SIGKILL? Does it not stop on SIGTERM?

> Requires: kernel-modules-extra

This is always going to be imperfect, because nothing guarantees that the
installed package corresponds to the actually running kernel (different
versions, variants like -debug, custom unpackaged kernels, ...).
There is precedent for depending on kernel-modules-extra in Fedora packages
(usbip, xl2tpd, ...) though.
I would just make it more explicit which module the package needs by instead
using this:

Requires: kmod(cuse.ko)
# Hint for dnf to prefer kernel-modules-extra over kernel-debug-modules-extra:
Suggests: kernel-modules-extra


BTW, is it necessary to call 'system("modprobe cuse");' in src/rshim.c? I would
expect the module to get autoloaded during the call to cuse_lowlevel_setup.


-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
_______________________________________________
package-review mailing list -- package-review@xxxxxxxxxxxxxxxxxxxxxxx
To unsubscribe send an email to package-review-leave@xxxxxxxxxxxxxxxxxxxxxxx
Fedora Code of Conduct: https://docs.fedoraproject.org/en-US/project/code-of-conduct/
List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines
List Archives: https://lists.fedoraproject.org/archives/list/package-review@xxxxxxxxxxxxxxxxxxxxxxx




[Index of Archives]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Yosemite Conditions]     [KDE Users]

  Powered by Linux