Fixes for packaging #172

jgunthorpe · 2017-07-24T19:51:56Z

These are various problems I found while working on the boot scripts.

bvanassche · 2017-07-26T17:52:56Z

srp_daemon/srp_daemon.c

@@ -1619,7 +1644,8 @@ static int get_config(struct config_t *conf, int argc, char *argv[])
 	while (1) {
 		int c;

-		c = getopt(argc, argv, "caveod:i:j:p:t:r:R:T:l:Vhnf:");
+		optind = 1;


Shouldn't this assignment occur outside the while-loop?

Yep, I'm going to move these two patches to another series and post them to the list. This has gone beyond packaging changes

bvanassche · 2017-07-26T17:57:05Z

srp_daemon/srp_daemon.c

+	cleanup_wakeup_fd();
+	return -1;
+}
+


Nit: a single blank line here should be sufficient.

bvanassche · 2017-07-26T17:58:09Z

srp_daemon/srp_daemon.c

@@ -1991,12 +2045,16 @@ static int ibsrpdm(int argc, char *argv[])
 		}
 	}

+	ret = setup_wakeup_fd();


Please don't add a call to setup_wakeup_fd() here. ibsrpdm doesn't call get_received_signal() and hence doesn't need wakeup_pipe[] to be initialized.

I did this only because the original code setup wakeup_fd before calling ibsrpdm, and I saw stuff in ibsrpdm that looks like it was using threads. If you are certain then I will drop it.

Yes, I'm sure that ibsrpdm doesn't need wakeup_pipe[] to be initialized and also that it would be an improvement if the signal actions are left unchanged for ibsrpdm. There are four calls to get_received_signal() in main() but these are all in code that cannot be reached when executed as ibsrpdm.

bvanassche · 2017-07-26T17:58:54Z

srp_daemon/srp_daemon.c

 umad_done:
 	umad_done();
-
+cleanup_wakeup:


Since ibsrpdm doesn't use wakeup_pipe[], please also remove this cleanup call.

jgunthorpe · 2017-07-26T18:35:07Z

Branch updated:

The srp_daemon LOG_PERROR and followup patches are dropped (will be in a new PR)
'Call systemctl properly from udev' also fixes start_on_all_ports
New trivial patch 'srp_daemon: start_on_all_ports uses a bashism'

This is up to date with all requested changes on github and the mailing list for this series.

bvanassche · 2017-07-26T19:58:38Z

srp_daemon/start_on_all_ports

@@ -1,4 +1,4 @@
-#!/bin/sh
+#!/bin/bash



In the description, please consider changing "tart_on_all_ports" into "start_on_all_ports" and "compatability" into "compatibility".

bvanassche · 2017-07-26T20:01:15Z

srp_daemon/srp_daemon.rules

@@ -1 +1 @@
-ACTION=="add", SUBSYSTEM=="infiniband_mad", KERNEL=="*umad*", PROGRAM:="/usr/bin/systemctl show srp_daemon -p ActiveState", RESULT=="ActiveState=active", ENV{SYSTEMD_WANTS}+="srp_daemon_port@$attr{ibdev}:$attr{port}.service"


Please change "Reviwed-by" into "Reviewed-by".

got it thanks

bdrung · 2017-07-27T14:06:32Z

debian/rules

@@ -34,6 +34,9 @@ override_dh_auto_test:

 override_dh_auto_install:
 	DESTDIR=$(CURDIR)/debian/tmp ninja -C build-deb install
+	# Debian kmod has a conf file called mlx4.conf that unnecessarily
+	# tries to autoload mlx4_en, call our conf file rdma-mlx4.conf instead
+	mv $(CURDIR)/debian/tmp/etc/modprobe.d/mlx4.conf $(CURDIR)/debian/tmp/etc/modprobe.d/rdma-mlx4.conf


mlx4.conf is only part of Ubuntu's kmod package. See https://launchpad.net/ubuntu/+source/kmod/24-1ubuntu1 and https://bugs.launchpad.net/maas/+bug/1115710

I am against renaming the file. kmod should stop providing that file and rely on rdma-core.

We already open bug [1] on this issue in kmod package, and i'll ask them to fix it.

https://bugs.launchpad.net/ubuntu/+source/kmod/+bug/1693503

Okay then.

@talatb To be clear, the mlx4.conf in kmod is not replaced by the one in rdma-core, they do totally different things.

The mlx4.conf in kmod is attempting to autoload ethernet support. I believe that the kernel now does that automatically via request_module.

Also, I haven't moved redhat/rdma.mlx4.sys.modprobe (which does the mlx4-setup thing) into common code because Mellanox said they prefer to do this setup another way. If so I recommend removing it from redhat as well.

bdrung · 2017-07-27T14:35:44Z

Besides my two smaller comments on the mailing list and my comment about the rename of mlx4.conf, I am fine with the changes (I skipped the redhat-prefixed ones)

It was ending up in rdma-core.rpm and ibacm.rpm, it belongs in ibacm only. Fixes: 0576db2 ("redhat spec: Avoid conflict due to implicit directory add") Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>

/usr/lib/udev/rules.d/libibumad.rules was claimed by rdma-core and libibumad.rpm /usr/lib/udev/rules.d/srp_daemon.rules was claimed by rdma-core and srp_daemon.rpm Fixes: 5a6f287 ("libibumad: Introduce the /dev/infiniband/umad/${ibdev}:${port}.device alias") Fixes: d35e5c4 ("srp_daemon.service: Add support for hot-plugging") Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>

/usr/lib64/libmlx4.so and 5 were being claimed by both rdma-core-devel.rpm and libibverbs.rpm The *.so symlinks belong only in the -devel package. Fixes: a61f2a6 ("mlx4: Export mlx4 direct verbs interface") Fixes: 07764bc ("mlx5: Export mlx5 direct verbs interface") Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>

cmake defaults this to /usr/var/run if left alone. This causes srp_daemon to not work properly on Debian derived. Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>

This was a copy and paste error. Fixes: bba93a9 ("Debian Packaging") Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>

This matches what RedHat is doing and makes sense, the rdma-core package contains the kernel support elements. Also add a Debian dependency for dmidecode as the truescale.cmd script uses it. Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>

This follows the standard convention for udev. I could not find an outline of where different activities should fall within the numbering scheme, so these are a guess: - 90 for libibumad rules because it only sets SYSTEMD_ALIAS and the only other rules to do that are labeled 99 - 60 for srp_daemon/ndd because other rules dealing with matching hardware seem to be around there as well Fixes: 5a6f287 ("libibumad: Introduce the /dev/infiniband/umad/${ibdev}:${port}.device alias") Fixes: d35e5c4 ("srp_daemon.service: Add support for hot-plugging") Fixes: 4910f1e ("rdma-ndd: add rdma-ndd from infiniband-diags") Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>

The only other users of SYSTEMD_ALIAS I could find uses the /sys/subsystem/ prefix, so we should do the same instead of using a fake /dev/ prefix. Fixes: 5a6f287 ("libibumad: Introduce the /dev/infiniband/umad/${ibdev}:${port}.device alias") Fixes: d35e5c4 ("srp_daemon.service: Add support for hot-plugging") Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com> Reviwed-by: Bart Van Assche <Bart.VanAssche@wdc.com>

Otherwise udev matches issm and umad for each resulting in systemd warnings: systemd[1]: sys-subsystem-rdma-devices-mlx4_0:1-umad.device: Dev sys-subsystem-rdma-devices-mlx4_0:1-umad.device appeared twice with different sysfs paths /sys/devices/pci0000:00/0000:00:04.0/infiniband_mad/issm0 and /sys/devices/pci0000:00/0000:00:04.0/infiniband_mad/umad0 Fixes: d35e5c4 ("srp_daemon.service: Add support for hot-plugging") Fixes: 5a6f287 ("libibumad: Introduce the /dev/infiniband/umad/${ibdev}:${port}.device alias") Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com> Reviwed-by: Bart Van Assche <Bart.VanAssche@wdc.com>

No reason to format to a string buffer and then just print that string, use vsyslog and vfprintf directly instead. Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com> Reviewed-by: Bart Van Assche <Bart.VanAssche@wdc.com>

Use /bin/ as the call path for systemctl. For some reason Debian derived distros do not have /usr/bin/systemctl, only /bin/systemctl. Other distros seem to have both paths. Also invoke PROGRAM using == which matches what the udev test suite does. Fixes: d35e5c4 ("srp_daemon.service: Add support for hot-plugging") Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com> Reviwed-by: Bart Van Assche <Bart.VanAssche@wdc.com>

start_on_all_ports[1184]: /usr/lib/srp_daemon/start_on_all_ports: 6: /usr/lib/srp_daemon/start_on_all_ports: Bad substitution Explicitly use bash for compatibility with Debian. Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>

jgunthorpe · 2017-07-27T19:41:24Z

All comments have been addressed and the conflicts with the just merged branch resolved.

iwpmd is not user runnable, so it should be installed in sbin. Also wrap-and-sort iwpmd.install Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>

rx_cfg is used to setup the kernel rxe device, it belongs with the rest of the kernel support items. Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>

Move it out of the redhat directory and into providers/mlx4, this follows the existing pattern of truescale.conf. Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>

bdrung · 2017-07-28T10:36:33Z

You missed one, see pull request #175

jgunthorpe · 2017-07-28T15:59:27Z

@brung, I got that one, I just did it wrong :(

jgunthorpe force-pushed the packaging branch 4 times, most recently from edef6e3 to 4437f37 Compare July 26, 2017 17:45

bvanassche reviewed Jul 26, 2017

View reviewed changes

jgunthorpe force-pushed the packaging branch from 4437f37 to a8f036c Compare July 26, 2017 17:58

bvanassche reviewed Jul 26, 2017

View reviewed changes

jgunthorpe force-pushed the packaging branch from a8f036c to 204d28a Compare July 26, 2017 18:32

bvanassche reviewed Jul 26, 2017

View reviewed changes

jgunthorpe force-pushed the packaging branch from 204d28a to 0376304 Compare July 26, 2017 20:06

bdrung reviewed Jul 27, 2017

View reviewed changes

jgunthorpe added 12 commits July 27, 2017 12:38

redhat: Fix redhat packaging of /etc/rdma/ibacm_opts.cfg

24d1eb9

It was ending up in rdma-core.rpm and ibacm.rpm, it belongs in ibacm only. Fixes: 0576db2 ("redhat spec: Avoid conflict due to implicit directory add") Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>

debian: Set the run directory path to /run/

0445712

cmake defaults this to /usr/var/run if left alone. This causes srp_daemon to not work properly on Debian derived. Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>

debian: Remove infiniband-diags Depends from iwpmd

8410440

This was a copy and paste error. Fixes: bba93a9 ("Debian Packaging") Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>

srp_deamon: Using v formatters directly for pr_err

70ead47

No reason to format to a string buffer and then just print that string, use vsyslog and vfprintf directly instead. Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com> Reviewed-by: Bart Van Assche <Bart.VanAssche@wdc.com>

jgunthorpe force-pushed the packaging branch 2 times, most recently from 50dec04 to f85a450 Compare July 27, 2017 18:42

jgunthorpe added 3 commits July 27, 2017 13:47

iwpmd: Fix install path for iwpmd

f387964

iwpmd is not user runnable, so it should be installed in sbin. Also wrap-and-sort iwpmd.install Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>

Move rxe_cfg to the rdma-core package

5c6372e

rx_cfg is used to setup the kernel rxe device, it belongs with the rest of the kernel support items. Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>

Make /etc/modprobe.d/mlx4.conf common

c46fa45

Move it out of the redhat directory and into providers/mlx4, this follows the existing pattern of truescale.conf. Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>

jgunthorpe force-pushed the packaging branch from f85a450 to c46fa45 Compare July 27, 2017 19:52

dledford merged commit 8f3a218 into linux-rdma:master Jul 27, 2017

jgunthorpe deleted the packaging branch July 27, 2017 22:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes for packaging #172

Fixes for packaging #172

jgunthorpe commented Jul 24, 2017

bvanassche Jul 26, 2017

jgunthorpe Jul 26, 2017

bvanassche Jul 26, 2017

jgunthorpe Jul 26, 2017

bvanassche Jul 26, 2017

jgunthorpe Jul 26, 2017

bvanassche Jul 26, 2017

bvanassche Jul 26, 2017

jgunthorpe commented Jul 26, 2017

bvanassche Jul 26, 2017

jgunthorpe Jul 26, 2017

bvanassche Jul 26, 2017

jgunthorpe Jul 26, 2017

bdrung Jul 27, 2017

talatb Jul 27, 2017

jgunthorpe Jul 27, 2017

bdrung commented Jul 27, 2017

jgunthorpe commented Jul 27, 2017

bdrung commented Jul 28, 2017

jgunthorpe commented Jul 28, 2017

		@@ -1 +1 @@
		ACTION=="add", SUBSYSTEM=="infiniband_mad", KERNEL=="umad", PROGRAM:="/usr/bin/systemctl show srp_daemon -p ActiveState", RESULT=="ActiveState=active", ENV{SYSTEMD_WANTS}+="srp_daemon_port@$attr{ibdev}:$attr{port}.service"

		@@ -1,4 +1,4 @@
		#!/bin/sh
		#!/bin/bash

Fixes for packaging #172

Fixes for packaging #172

Conversation

jgunthorpe commented Jul 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jgunthorpe commented Jul 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bdrung commented Jul 27, 2017

jgunthorpe commented Jul 27, 2017

bdrung commented Jul 28, 2017

jgunthorpe commented Jul 28, 2017